Skip to content

Commit

Permalink
Prevent top level nested attributes
Browse files Browse the repository at this point in the history
* Add remove_repr macro for testing: This macro removes `#[repr(_)]` from an enum, this is useful for testing
the bug described in #1. `#[remove_repr]` is only available behind the
feature flag test-utils.

* Add a fake `repr` attribute in test-utils: This patch adds a fake `repr` and add tests to try to confuse the
compiler into thinking that it is the real repr. If possible, that would
cause problems with safe-discriminant. That is because we assume that
there is always a repr attributed.

* Reject enums with attribute macros: This patch checks if there is any top level attribute macro expansion,
and if there it, we report it as an error.

* test marcro ordering

fixes: #1

---------

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
  • Loading branch information
oddcoder authored Aug 19, 2024
1 parent 6be62f8 commit 4b23fc3
Show file tree
Hide file tree
Showing 17 changed files with 187 additions and 0 deletions.
2 changes: 2 additions & 0 deletions safe-discriminant-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ proc-macro2 = { workspace = true }
quote = { workspace = true }
syn = { workspace = true }

[features]
test-utils = []
45 changes: 45 additions & 0 deletions safe-discriminant-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,28 @@ fn validate_all_variants(variants: impl Iterator<Item = Variant>) -> Result<()>
.unwrap_or(Ok(()))
}

/// Returns true if there is any #[x] where x is not `repr`
/// returns false otherwise.
fn contains_attribute_macros(attrs: &[Attribute]) -> bool {
attrs.iter().any(|attr| !attr.path().is_ident("repr"))
}
/// Constructs Discriminant trait implementation for given enum.
/// Returns error in one of two cases:
/// 1- No valid `#[repr(x)]` is found.
/// 2- Any of the enum variants is missing discriminant.
/// 3- contains any additional top level attribute macros.
fn derive_discriminant_inner(tagged_enum: ItemEnum) -> Result<TokenStream> {
let prim = get_enum_repr_prim(&tagged_enum.attrs, tagged_enum.ident.span())?;
validate_all_variants(tagged_enum.variants.into_iter())?;
if contains_attribute_macros(&tagged_enum.attrs) {
return Err(Error::new(
tagged_enum.ident.span(),
concat!(
"Discriminant is not compatiable with any top ",
"level `#[attr]` except `#[repr(_)]`."
),
));
}
let name = tagged_enum.ident;
let generics = tagged_enum.generics;
let derive = quote! {
Expand All @@ -110,3 +125,33 @@ pub fn derive_discriminant(item: TokenStream) -> TokenStream {
Ok(s) => s,
}
}

#[cfg(feature = "test-utils")]
/// This macro will remove `#[repr(_)]` from any given enum.
/// This is only used for testing.
#[proc_macro_attribute]
pub fn remove_repr(_: TokenStream, item: TokenStream) -> TokenStream {
let mut tagged_enum = parse_macro_input!(item as ItemEnum);
tagged_enum
.attrs
.retain(|attr| !attr.path().is_ident("repr"));
quote! {
#tagged_enum
}
.into()
}

#[cfg(feature = "test-utils")]
/// This macro is fake `#[repr(_)]` attribute, it will be a problem if we
/// can trick the macro system into thinking this is the real #[repr(_)]
#[proc_macro_attribute]
pub fn repr(_: TokenStream, item: TokenStream) -> TokenStream {
item
}

#[cfg(feature = "test-utils")]
/// exactly as the name suggests!
#[proc_macro_attribute]
pub fn do_nothing(_: TokenStream, item: TokenStream) -> TokenStream {
item
}
1 change: 1 addition & 0 deletions safe-discriminant/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ safe-discriminant-derive = {workspace = true}

[dev-dependencies]
trybuild = { workspace = true }
safe-discriminant-derive = {workspace = true, features = ["test-utils"]}
12 changes: 12 additions & 0 deletions safe-discriminant/tests/fail/do_nothing_last.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use safe_discriminant::Discriminant;
use safe_discriminant_derive::do_nothing;

#[repr(u8)]
#[derive(Discriminant)]
#[do_nothing]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
5 changes: 5 additions & 0 deletions safe-discriminant/tests/fail/do_nothing_last.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Discriminant is not compatiable with any top level `#[attr]` except `#[repr(_)]`.
--> tests/fail/do_nothing_last.rs:7:10
|
7 | pub enum Foo {
| ^^^
13 changes: 13 additions & 0 deletions safe-discriminant/tests/fail/fake_repr1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// attempting to confuse Discriminant into thinking
// the fake repr is the real repr
use safe_discriminant::Discriminant;
use safe_discriminant_derive::repr;

#[derive(Discriminant)]
#[repr(u8)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
14 changes: 14 additions & 0 deletions safe-discriminant/tests/fail/fake_repr1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0659]: `repr` is ambiguous
--> tests/fail/fake_repr1.rs:7:3
|
7 | #[repr(u8)]
| ^^^^ ambiguous name
|
= note: ambiguous because of a name conflict with a builtin attribute
= note: `repr` could refer to a built-in attribute
note: `repr` could also refer to the attribute macro imported here
--> tests/fail/fake_repr1.rs:4:5
|
4 | use safe_discriminant_derive::repr;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: use `crate::repr` to refer to this attribute macro unambiguously
13 changes: 13 additions & 0 deletions safe-discriminant/tests/fail/fake_repr2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// attempting to confuse Discriminant into thinking
// the fake repr is the real repr
use safe_discriminant::Discriminant;
use safe_discriminant_derive::repr;

#[derive(Discriminant)]
#[crate::repr(u8)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
5 changes: 5 additions & 0 deletions safe-discriminant/tests/fail/fake_repr2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Discriminant requires a `#[repr(x)] where x is one of u8, i8, u16, i16, u32, i32, u64, i64, u128, i128.
--> tests/fail/fake_repr2.rs:8:10
|
8 | pub enum Foo {
| ^^^
12 changes: 12 additions & 0 deletions safe-discriminant/tests/fail/fake_repr3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// attempting to confuse Discriminant into thinking
// the fake repr is the real repr
use safe_discriminant::Discriminant;

#[derive(Discriminant)]
#[safe_discriminant_derive::repr(u8)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
5 changes: 5 additions & 0 deletions safe-discriminant/tests/fail/fake_repr3.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Discriminant requires a `#[repr(x)] where x is one of u8, i8, u16, i16, u32, i32, u64, i64, u128, i128.
--> tests/fail/fake_repr3.rs:7:10
|
7 | pub enum Foo {
| ^^^
12 changes: 12 additions & 0 deletions safe-discriminant/tests/fail/remove_repr_disc1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use safe_discriminant::Discriminant;
use safe_discriminant_derive::remove_repr;

#[remove_repr]
#[derive(Discriminant)]
#[repr(u8)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
5 changes: 5 additions & 0 deletions safe-discriminant/tests/fail/remove_repr_disc1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Discriminant requires a `#[repr(x)] where x is one of u8, i8, u16, i16, u32, i32, u64, i64, u128, i128.
--> tests/fail/remove_repr_disc1.rs:7:10
|
7 | pub enum Foo {
| ^^^
12 changes: 12 additions & 0 deletions safe-discriminant/tests/fail/remove_repr_disc2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use safe_discriminant::Discriminant;
use safe_discriminant_derive::remove_repr;

#[derive(Discriminant)]
#[remove_repr]
#[repr(u8)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}
5 changes: 5 additions & 0 deletions safe-discriminant/tests/fail/remove_repr_disc2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: Discriminant is not compatiable with any top level `#[attr]` except `#[repr(_)]`.
--> tests/fail/remove_repr_disc2.rs:7:10
|
7 | pub enum Foo {
| ^^^
15 changes: 15 additions & 0 deletions safe-discriminant/tests/pass/do_nothing_first.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use safe_discriminant::Discriminant;
use safe_discriminant_derive::do_nothing;

#[do_nothing]
#[repr(u8)]
#[derive(Discriminant)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {
assert_eq!(Foo::A.discriminant(), 0);
assert_eq!(Foo::B.discriminant(), 1);
}
11 changes: 11 additions & 0 deletions safe-discriminant/tests/pass/remove_repr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// this test makes sure that remove repr actually removes repr
use safe_discriminant_derive::remove_repr;

#[remove_repr]
#[repr(FOO_BAR_TYPE_DOES_NOT_EXIST)]
pub enum Foo {
A = 0,
B = 1,
}

fn main() {}

0 comments on commit 4b23fc3

Please sign in to comment.