-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix #1 #2
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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! { | ||
|
@@ -110,3 +125,33 @@ pub fn derive_discriminant(item: TokenStream) -> TokenStream { | |
Ok(s) => s, | ||
} | ||
} | ||
|
||
#[cfg(feature = "test-utils")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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() {} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
/// 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 | ||
} |
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() {} |
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 { | ||
| ^^^ |
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() {} |
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 |
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() {} |
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 { | ||
| ^^^ |
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() {} |
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 { | ||
| ^^^ |
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() {} |
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 { | ||
| ^^^ |
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() {} |
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 { | ||
| ^^^ |
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); | ||
} |
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; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional/Nit: Some extra spacing and strange formatting in the file. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
#[remove_repr] | ||
#[repr(FOO_BAR_TYPE_DOES_NOT_EXIST)] | ||
pub enum Foo { | ||
A = 0, | ||
B = 1, | ||
} | ||
|
||
fn main() {} |
Uh oh!
There was an error while loading. Please reload this page.