From 2ee4f1ff0e2175c54c44e6d82a4d77466794fe0f Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 11 Mar 2014 22:31:14 -0700 Subject: [PATCH 1/3] RFC: Rearchitect the attribute-usage lint --- 0000-attribute-usage.md | 87 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 0000-attribute-usage.md diff --git a/0000-attribute-usage.md b/0000-attribute-usage.md new file mode 100644 index 00000000000..11b734c897d --- /dev/null +++ b/0000-attribute-usage.md @@ -0,0 +1,87 @@ +# Summary + +Rust currently has an attribute usage lint but it does not work particularly +well. This RFC proposes a new implementation strategy that should make it +significantly more useful. + +# Motivation + +The current implementation has two major issues: + +1. There are very limited warnings for valid attributes that end up in the +wrong place. Something like this will be silently ignored: +```rust +#[deriving(Clone)]; // Shouldn't have put a ; here +struct Foo; + +#[ignore(attribute-usage)] // Should have used #[allow(attribute-usage)] instead! +mod bar { + //... +} +``` + +2. `ItemDecorators` can now be defined outside of the compiler, and there's no +way to tag them and associated attributes as valid. Something like this +requires an `#[allow(attribute-usage)]`: +```rust +#[feature(phase)]; +#[phase(syntax, link)] +extern crate some_orm; + +#[ormify] +pub struct Foo { + #[column(foo_)] + #[primary_key] + foo: int +} +``` + +# Detailed design + +The current implementation is implemented as a simple fold over the AST, +comparing attributes against a whitelist. Crate-level attributes use a separate +whitelist, but no other distinctions are made. + +This RFC would change the implementation to actually track which attributes are +used during the compilation process. `syntax::ast::Attribute_` would be +modified to add a new `used` field: +```rust +pub struct Attribute_ { + style: AttrStyle, + value: @MetaItem, + is_sugared_doc: bool, + used: bool, +} +``` + +It will be initialized to `false`, and code reading an attribute should set it +to `true`. The utility methods in `ast::attr` can be modified to automatically +set it to handle the simple cases automatically. + +The `attribute-usage` lint would run at the end of compilation and warn on all +attributes whose `used` field hasn't been set. + +One interesting edge case is attributes like `doc` that are used, but not in +the normal compilation process. There could either be a separate fold pass to +mark all `doc` attributes as used or `doc` could simply be whitelisted in the +`attribute-usage` lint. + +Attributes in code that has been eliminated with `#[cfg()]` will not be linted, +but I feel that this is consistent with the way `#[cfg()]` works in general +(e.g. the code won't be type-checked either). + +# Alternatives + +An alternative would be to rewrite `rustc::middle::lint` to robustly check +that attributes are used where they're supposed to be. This will be fairly +complex and be prone to failure if/when more nodes are added to the AST. This +also doesn't solve motivation #2, which would require externally loaded lint +support. + +# Unresolved questions + ++ This implementation doesn't allow for a distinction between "unused" and +"unknown" attributes. The `#[phase(syntax)]` crate loading infrastructure could +be extended to pull a list of attributes from crates to use in the lint pass, +but I'm not sure if the extra complexity is worth it. + From 19ed3281b2c55a08a2501df025b4544b14393099 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Wed, 12 Mar 2014 20:18:04 -0700 Subject: [PATCH 2/3] Switch from AST mutation to a side table --- 0000-attribute-usage.md | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/0000-attribute-usage.md b/0000-attribute-usage.md index 11b734c897d..38abb3b3533 100644 --- a/0000-attribute-usage.md +++ b/0000-attribute-usage.md @@ -8,7 +8,7 @@ significantly more useful. The current implementation has two major issues: -1. There are very limited warnings for valid attributes that end up in the ++ There are very limited warnings for valid attributes that end up in the wrong place. Something like this will be silently ignored: ```rust #[deriving(Clone)]; // Shouldn't have put a ; here @@ -19,8 +19,7 @@ mod bar { //... } ``` - -2. `ItemDecorators` can now be defined outside of the compiler, and there's no ++ `ItemDecorators` can now be defined outside of the compiler, and there's no way to tag them and associated attributes as valid. Something like this requires an `#[allow(attribute-usage)]`: ```rust @@ -44,22 +43,31 @@ whitelist, but no other distinctions are made. This RFC would change the implementation to actually track which attributes are used during the compilation process. `syntax::ast::Attribute_` would be -modified to add a new `used` field: +modified to add an ID field: ```rust +pub type AttrId = uint; + pub struct Attribute_ { + id: AttrId, style: AttrStyle, value: @MetaItem, is_sugared_doc: bool, - used: bool, } ``` -It will be initialized to `false`, and code reading an attribute should set it -to `true`. The utility methods in `ast::attr` can be modified to automatically -set it to handle the simple cases automatically. +`syntax::ast::parse::ParseSess` will generate new `AttrId`s on demand. I +believe that attributes will only be created during parsing and expansion, and +the `ParseSess` is accessible in both. + +The `AttrId`s will be used to create a side table of used attributes. This will +most likely be a thread local to make it easily accessible during all stages of +compilation by calling a function in `syntax::attr`: +```rust +fn mark_used(attr: &Attribute) { } +``` The `attribute-usage` lint would run at the end of compilation and warn on all -attributes whose `used` field hasn't been set. +attributes whose ID does not appear in the side table. One interesting edge case is attributes like `doc` that are used, but not in the normal compilation process. There could either be a separate fold pass to @@ -84,4 +92,10 @@ support. "unknown" attributes. The `#[phase(syntax)]` crate loading infrastructure could be extended to pull a list of attributes from crates to use in the lint pass, but I'm not sure if the extra complexity is worth it. ++ The side table could be threaded through all of the compilation stages that +need to use it instead of being a thread local. This would probably require +significantly more work than the thread local approach, however. The thread +local approach should not negatively impact any future parallelization work as +each thread can keep its own side table, which can be merged into one for the +lint pass. From b925e325993c33b041c112db5de7b1d5c9926977 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 18 Mar 2014 19:22:00 -0700 Subject: [PATCH 3/3] Switch `AttrId` to a newtype --- 0000-attribute-usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/0000-attribute-usage.md b/0000-attribute-usage.md index 38abb3b3533..d5158f4ac7a 100644 --- a/0000-attribute-usage.md +++ b/0000-attribute-usage.md @@ -45,7 +45,7 @@ This RFC would change the implementation to actually track which attributes are used during the compilation process. `syntax::ast::Attribute_` would be modified to add an ID field: ```rust -pub type AttrId = uint; +pub struct AttrId(uint); pub struct Attribute_ { id: AttrId,