Warn on pointless #[derive] in more places #50092

abonander · 2018-04-19T23:56:35Z

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

abonander · 2018-04-20T00:26:30Z

The regression test won't be effective until #49826 is merged, however. It passes before and after even though compiling compile-fail/issue-49934.rs standalone does panic in the beta compiler. This is because compile-fail is eating ICEs because they're currently not reported as such (#49888).

pietroalbini · 2018-04-20T07:53:23Z

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.

petrochenkov · 2018-04-20T22:44:17Z

src/test/compile-fail/issue-49934.rs

+#![feature(stmt_expr_attributes)]
+
+fn main() {
+    #[derive(Debug)] //~ ERROR `derive`


//~ ERROR derive

That's very informative.
Also + to @pietroalbini's suggestion about UI tests.

petrochenkov · 2018-04-20T22:49:20Z

@abonander
There were few regressions, some of them have reverse dependencies, so it would indeed make sense to make this a deprecation lint instead of a hard error, if that's not too much hassle.

abonander · 2018-04-26T11:04:56Z

@petrochenkov I made it a hard warning instead of a deprecation lint (since there's nothing being deprecated here)

abonander · 2018-04-26T11:12:30Z

src/test/ui/issue-49934.stderr

@@ -0,0 +1,24 @@
+warning: `#[derive]` on non-item statements is ignored


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"?

Maybe "#[derive] can only be applied on items"?

That doesn't explain why it's a warning and not an error, though.

abonander · 2018-04-28T00:52:14Z

@pietroalbini can you mark this Waiting on Review please

pietroalbini · 2018-04-28T07:46:59Z

Sure. Also, @petrochenkov, ping! This fixes a beta regression.

petrochenkov · 2018-04-28T09:15:39Z

@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).

bors · 2018-04-28T09:15:40Z

📌 Commit 7070198 has been approved by petrochenkov

abonander · 2018-04-28T09:18:06Z

@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.

pietroalbini · 2018-04-28T09:18:59Z

@bors p=1

petrochenkov · 2018-04-29T18:48:35Z

src/test/ui/issue-49934.rs

+        // fold_opt_expr
+        #[derive(Debug)] //~ WARN unused attribute
+        "Hello, world!"
+    ];


Could you add test cases for #50092 (comment) as well?
I.e. something like

fn f<#[derive(Debug)] T>() { match 0 { #[derive(Debug)] _ => {} } }

@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.

Sure, the more bugs are fixed the better, especially with one-line changes.

abonander · 2018-04-29T22:54:31Z

@petrochenkov updated.

abonander · 2018-04-29T22:57:54Z

src/libsyntax/ext/derive.rs

+    });
+
+    (item, derive_spans)
+}


this method is unused but isn't being warned about because it's public

abonander · 2018-04-29T22:59:57Z

src/librustc/hir/intravisit.rs

@@ -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());


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.

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.

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.

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

abonander · 2018-04-29T23:03:59Z

src/libsyntax/ext/expand.rs

@@ -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() {


I think I could flatten these two conditionals plus the if let StmtKind::Mac(_) to a single match if it's preferred.

petrochenkov · 2018-04-29T23:32:18Z

Excellent.
@bors r+

bors · 2018-04-29T23:32:19Z

📌 Commit f16d2ff has been approved by petrochenkov

bors · 2018-04-30T00:18:57Z

⌛ Testing commit f16d2ff with merge 78bcd9b...

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

bors · 2018-04-30T02:32:46Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 78bcd9b to master...

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

@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

@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)

abonander changed the title ~~Don't panic on dummy expansions for statments/expressions~~ Don't panic #[derive] placed on statments/expressions Apr 19, 2018

abonander changed the title ~~Don't panic #[derive] placed on statments/expressions~~ Don't panic #[derive] placed on statements/expressions Apr 19, 2018

abonander force-pushed the issue-49934 branch from 9418c8b to 7656945 Compare April 19, 2018 23:58

abonander changed the title ~~Don't panic #[derive] placed on statements/expressions~~ Don't panic on #[derive] placed on statements/expressions Apr 20, 2018

abonander mentioned this pull request Apr 20, 2018

[WIP] Treat crates specially when expanding #50101

Closed

2 tasks

pietroalbini assigned petrochenkov Apr 20, 2018

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2018

petrochenkov reviewed Apr 20, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2018

alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Apr 24, 2018

abonander changed the title ~~Don't panic on #[derive] placed on statements/expressions~~ Warn on #[derive] placed on statements/expressions Apr 26, 2018

abonander force-pushed the issue-49934 branch from 7656945 to 7070198 Compare April 26, 2018 11:06

abonander commented Apr 26, 2018

View reviewed changes

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 28, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2018

pietroalbini mentioned this pull request Apr 28, 2018

[Beta] Process backports #50298

Merged

petrochenkov reviewed Apr 29, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2018

abonander changed the title ~~Warn on #[derive] placed on statements/expressions~~ Warn on pointless #[derive] in more places Apr 29, 2018

abonander force-pushed the issue-49934 branch from 181fcea to 5a74ca4 Compare April 29, 2018 22:54

abonander commented Apr 29, 2018

View reviewed changes

abonander force-pushed the issue-49934 branch from 5a74ca4 to f16d2ff Compare April 29, 2018 23:01

abonander commented Apr 29, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2018

bors merged commit f16d2ff into rust-lang:master Apr 30, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Apr 30, 2018

Remove new enum variant from rust-lang#50092 backport

dacebb8

ExpansionKind::ForeignItems was added in rust-lang#49350, which is not included in the 1.26 beta.

pietroalbini mentioned this pull request Apr 30, 2018

[beta] Yet another round of backports #50334

Merged

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Apr 30, 2018

Adapt ui test of rust-lang#50092 to beta

81c564e

* Changed `// compile-pass` to `// must-compile-successfully` * Removed checks on unstable features

pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2018

Aaron1011 mentioned this pull request Oct 24, 2020

Inconsistent handling of #[derive] on invalid targets #78331

Closed

petrochenkov mentioned this pull request Nov 21, 2020

expand/resolve: Turn #[derive] into a regular macro attribute #79078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn on pointless #[derive] in more places #50092

Warn on pointless #[derive] in more places #50092

abonander commented Apr 19, 2018 •

edited

Loading

abonander commented Apr 20, 2018 •

edited

Loading

pietroalbini commented Apr 20, 2018

petrochenkov Apr 20, 2018

petrochenkov commented Apr 20, 2018

abonander commented Apr 26, 2018

abonander Apr 26, 2018 •

edited

Loading

pietroalbini Apr 26, 2018 •

edited

Loading

abonander Apr 27, 2018

abonander commented Apr 28, 2018

pietroalbini commented Apr 28, 2018

petrochenkov commented Apr 28, 2018

bors commented Apr 28, 2018

abonander commented Apr 28, 2018

pietroalbini commented Apr 28, 2018

petrochenkov Apr 29, 2018

abonander Apr 29, 2018

petrochenkov Apr 29, 2018

abonander Apr 29, 2018

abonander commented Apr 29, 2018

abonander Apr 29, 2018

abonander Apr 29, 2018

abonander Apr 29, 2018 •

edited

Loading

abonander Apr 29, 2018

abonander Apr 29, 2018

abonander Apr 29, 2018

petrochenkov commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018

		@@ -0,0 +1,24 @@
		warning: `#[derive]` on non-item statements is ignored

Warn on pointless #[derive] in more places #50092

Warn on pointless #[derive] in more places #50092

Conversation

abonander commented Apr 19, 2018 • edited Loading

abonander commented Apr 20, 2018 • edited Loading

pietroalbini commented Apr 20, 2018

Choose a reason for hiding this comment

petrochenkov commented Apr 20, 2018

abonander commented Apr 26, 2018

abonander Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

pietroalbini Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Apr 28, 2018

pietroalbini commented Apr 28, 2018

petrochenkov commented Apr 28, 2018

bors commented Apr 28, 2018

abonander commented Apr 28, 2018

pietroalbini commented Apr 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Apr 29, 2018

bors commented Apr 29, 2018

bors commented Apr 30, 2018

bors commented Apr 30, 2018

abonander commented Apr 19, 2018 •

edited

Loading

abonander commented Apr 20, 2018 •

edited

Loading

abonander Apr 26, 2018 •

edited

Loading

pietroalbini Apr 26, 2018 •

edited

Loading

abonander Apr 29, 2018 •

edited

Loading