Skip to content

Fix #1 #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we want to get super technical, what we really want to say is that there are no attributes past the derive macro that is implementing the Discriminant trait.

I actually think that's what this code accomplishes, since I think that the other attribute macros will be consumed in-order, so I don't think you can see the attributes after they've taken effect. But the chat logs that you pointed to seem to suggest otherwise and that you can see other attribute macros after they've been expanded. Am I reading this right?

Anyways, consider this an optional request, but it might be nice to try to create an attribute macro that isn't malicious and does nothing and see if it gets compiled out so that we don't see it as long as it's positioned before the derive attribute. And if so, we can test a non-malicious attribute macro to see that it produces what we expect (an error if placed after the derive attribute, success if placed before).

I'm not totally sure what would happen, but maybe worth playing with.

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")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional/Nit: Slight preference to put this in a separate module and just #[cfg(feature = "test-utils")] the module instead of doing it per testonly attribute.

I think it'd read a lot clearer in tests too:

use safe_discriminant::Discriminant;
use safe_discriminant_derive::testonly::repr;

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

fn main() {}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is unfortunately no possible, all proc macros must be defined in the top level lib.rs.

/// 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional/Nit: Some extra spacing and strange formatting in the file.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

righ, I forgot that cargo format doesn't not format those files, will format with rustfmt

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

fn main() {}
Loading