Warn on pointless #[derive] in more places #50092
Conversation
|
Maybe you can use an ui test instead of a compile-fail test? Also yeah, if the extra derive on statements doesn't cause any trouble but it's just for correctness, I would prefer a warn-by-default lint. |
src/test/compile-fail/issue-49934.rs
Outdated
| #![feature(stmt_expr_attributes)] | ||
|
|
||
| fn main() { | ||
| #[derive(Debug)] //~ ERROR `derive` |
There was a problem hiding this comment.
//~ ERROR
derive
That's very informative.
Also + to @pietroalbini's suggestion about UI tests.
|
@abonander |
|
@petrochenkov I made it a hard warning instead of a deprecation lint (since there's nothing being deprecated here) |
src/test/ui/issue-49934.stderr
Outdated
| @@ -0,0 +1,24 @@ | |||
| warning: `#[derive]` on non-item statements is ignored | |||
There was a problem hiding this comment.
Bikeshedding: better term than "non-item statements"? (the fact that items are a subtype of statements in this context is an idiosyncrasy of the current AST, I think). Just "statements" maybe?
And: "is ignored", "does nothing", or "is meaningless"?
There was a problem hiding this comment.
Maybe "#[derive] can only be applied on items"?
There was a problem hiding this comment.
That doesn't explain why it's a warning and not an error, though.
|
@pietroalbini can you mark this |
|
Sure. Also, @petrochenkov, ping! This fixes a beta regression. |
|
@bors r+ because regression, but hardcoded warnings are bad mkay, this should be a deprecation lint ideally, unless linting can't be done during expansion (I don't remember). |
|
📌 Commit 7070198 has been approved by |
|
@petrochenkov Yeah it's difficult cause linting is done after the conversion to HIR. If I get around to addressing deprecation of macros I could convert this to a deprecation lint but at that point I would rather make it a hard error for the 2018 edition. |
|
@bors p=1 |
| // fold_opt_expr | ||
| #[derive(Debug)] //~ WARN unused attribute | ||
| "Hello, world!" | ||
| ]; |
There was a problem hiding this comment.
Could you add test cases for #50092 (comment) as well?
I.e. something like
fn f<#[derive(Debug)] T>() {
match 0 {
#[derive(Debug)]
_ => {}
}
}There was a problem hiding this comment.
@petrochenkov intravisit is already walking attributes on match arms, but it is not walking attributes on type parameters. Do you want me to implement that as well? It's a one-line change.
There was a problem hiding this comment.
Sure, the more bugs are fixed the better, especially with one-line changes.
|
@petrochenkov updated. |
src/libsyntax/ext/derive.rs
Outdated
| }); | ||
|
|
||
| (item, derive_spans) | ||
| } |
There was a problem hiding this comment.
this method is unused but isn't being warned about because it's public
| @@ -404,7 +404,7 @@ pub fn walk_local<'v, V: Visitor<'v>>(visitor: &mut V, local: &'v Local) { | |||
| // Intentionally visiting the expr first - the initialization expr | |||
| // dominates the local's definition. | |||
| walk_list!(visitor, visit_expr, &local.init); | |||
|
|
|||
| walk_list!(visitor, visit_attribute, local.attrs.iter()); | |||
There was a problem hiding this comment.
I would almost recommend an audit of the entire intravisit module to ensure that anywhere attributes can appear they're being visited. This missing line took me three hours to debug because I assumed it was something wrong with my code.
There was a problem hiding this comment.
It looks like every type in hir that has an attrs field is now being walked by intravisit. I think I've already fixed the last of the discrepancies.
There was a problem hiding this comment.
On a different note, this field and a couple others use ThinVec while the attrs fields on all the other types use HirVec. I'm not sure of the difference but I think it's just an alias anyway. However, I got an error when passing &local.attrs here when it works fine for invocations with HirVec.
There was a problem hiding this comment.
I see, HirVec is effectively Box<[T]> while ThinVec is Option<Box<Vec<T>>> (fat pointer vs thin pointer). Using ThinVec in these cases may have been a deliberate choice to reduce memory consumption.
This fixes the regression in rust-lang#49934 and ensures that unused `#[derive]`s on statements, expressions and generic type parameters survive to trip the `unused_attributes` lint. For `#[derive]` on macro invocations it has a hardcoded warning since linting occurs after expansion. This also adds regression testing for some nodes that were already warning properly. closes rust-lang#49934
| @@ -1001,7 +1040,14 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { | |||
|
|
|||
| // we'll expand attributes on expressions separately | |||
| if !stmt.is_expr() { | |||
| let (attr, derives, stmt_) = self.classify_item(stmt); | |||
| let (attr, derives, stmt_) = if stmt.is_item() { | |||
There was a problem hiding this comment.
I think I could flatten these two conditionals plus the if let StmtKind::Mac(_) to a single match if it's preferred.
|
Excellent. |
|
📌 Commit f16d2ff has been approved by |
Warn on pointless #[derive] in more places This fixes the regression in #49934 and ensures that unused `#[derive]` invocations on statements, expressions and generic type parameters survive to trip the `unused_attributes` lint. There is a separate warning hardcoded for `#[derive]` on macro invocations since linting (even the early-lint pass) occurs after expansion. This also adds regression tests for some nodes that were already warning properly. closes #49934
|
☀️ Test successful - status-appveyor, status-travis |
ExpansionKind::ForeignItems was added in rust-lang#49350, which is not included in the 1.26 beta.
* Changed `// compile-pass` to `// must-compile-successfully` * Removed checks on unstable features
[beta] Yet another round of backports * #50092: Warn on pointless `#[derive]` in more places * #50227: Fix ICE with erroneous `impl Trait` in a trait impl #50092 also needed some tweaks on the beta branch (see my own two commits). r? @alexcrichton
[beta] Yet another round of backports * #50092: Warn on pointless `#[derive]` in more places * #50227: Fix ICE with erroneous `impl Trait` in a trait impl #50092 also needed some tweaks on the beta branch (see my own two commits). r? @alexcrichton
expand/resolve: Turn `#[derive]` into a regular macro attribute This PR turns `#[derive]` into a regular attribute macro declared in libcore and defined in `rustc_builtin_macros`, like it was previously done with other "active" attributes in rust-lang#62086, rust-lang#62735 and other PRs. This PR is also a continuation of rust-lang#65252, rust-lang#69870 and other PRs linked from them, which layed the ground for converting `#[derive]` specifically. `#[derive]` still asks `rustc_resolve` to resolve paths inside `derive(...)`, and `rustc_expand` gets those resolution results through some backdoor (which I'll try to address later), but otherwise `#[derive]` is treated as any other macro attributes, which simplifies the resolution-expansion infra pretty significantly. The change has several observable effects on language and library. Some of the language changes are **feature-gated** by [`feature(macro_attributes_in_derive_output)`](rust-lang#81119). #### Library - `derive` is now available through standard library as `{core,std}::prelude::v1::derive`. #### Language - `derive` now goes through name resolution, so it can now be renamed - `use derive as my_derive; #[my_derive(Debug)] struct S;`. - `derive` now goes through name resolution, so this resolution can fail in corner cases. Crater found one such regression, where import `use foo as derive` goes into a cycle with `#[derive(Something)]`. - **[feature-gated]** `#[derive]` is now expanded as any other attributes in left-to-right order. This allows to remove the restriction on other macro attributes following `#[derive]` (rust-lang/reference#566). The following macro attributes become a part of the derive's input (this is not a change, non-macro attributes following `#[derive]` were treated in the same way previously). - `#[derive]` is now expanded as any other attributes in left-to-right order. This means two derive attributes `#[derive(Foo)] #[derive(Bar)]` are now expanded separately rather than together. It doesn't generally make difference, except for esoteric cases. For example `#[derive(Foo)]` can now produce an import bringing `Bar` into scope, but previously both `Foo` and `Bar` were required to be resolved before expanding any of them. - **[feature-gated]** `#[derive()]` (with empty list in parentheses) actually becomes useful. For historical reasons `#[derive]` *fully configures* its input, eagerly evaluating `cfg` everywhere in its target, for example on fields. Expansion infra doesn't do that for other attributes, but now when macro attributes attributes are allowed to be written after `#[derive]`, it means that derive can *fully configure* items for them. ```rust #[derive()] #[my_attr] struct S { #[cfg(FALSE)] // this field in removed by `#[derive()]` and not observed by `#[my_attr]` field: u8 } ``` - `#[derive]` on some non-item targets is now prohibited. This was accidentally allowed as noop in the past, but was warned about since early 2018 (rust-lang#50092), despite that crater found a few such cases in unmaintained crates. - Derive helper attributes used before their introduction are now reported with a deprecation lint. This change is long overdue (since macro modularization, rust-lang#52226 (comment)), but it was hard to do without fixing expansion order for derives. The deprecation is tracked by rust-lang#79202. ```rust #[trait_helper] // warning: derive helper attribute is used before it is introduced #[derive(Trait)] struct S {} ``` Crater analysis: rust-lang#79078 (comment)
This fixes the regression in #49934 and ensures that unused
#[derive]invocations on statements, expressions and generic type parameters survive to trip theunused_attributeslint. There is a separate warning hardcoded for#[derive]on macro invocations since linting (even the early-lint pass) occurs after expansion. This also adds regression tests for some nodes that were already warning properly.closes #49934