Add lint for unused macros by est31 · Pull Request #41907 · rust-lang/rust

est31 · 2017-05-11T09:00:51Z

Addresses parts of #34938, to add a lint for unused macros.

We now output warnings by default when we encounter a macro that we didn't use for expansion.

Issues to be resolved before this PR is ready for merge:

fix the NodeId issue described above
remove all unused macros from rustc and the libraries or set #[allow(unused_macros)] next to them if they should be kept for some reason. This is needed for successful boostrap and bors to accept the PR. -> Remove unused macros from the codebase #41934
~~implement the full extent of Unused lint warnings for macros #34938, that means the macro match arm checking as well.~~let's not do this for now

rust-highfive · 2017-05-11T09:01:04Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

est31 · 2017-05-11T09:02:09Z

r? @jseyfried because you seem to have done many things with macros in the past, so might know the code best.

est31 · 2017-05-11T12:42:12Z

Hmmm, I suspect that the lint checker requires something to be present under the NodeId we give it, so we can't just bail out by creating some random one, but I guess we'll need to create our own special ExprKind like UnusedMac or something that then lives on until the linter runs.

est31 · 2017-05-12T06:34:10Z

Okay, apparently macro definitions do survive inside the AST (but not in the hir), only have to add handling for them in the lint code. Update incoming. I've also found some occurences of unused macros inside the codebase,

Remove unused macros from the codebase Thanks to the lint I've implemented in rust-lang#41907 I've found some unused macros inside the rustc codebase.

petrochenkov · 2017-05-13T12:45:40Z

If @jseyfried doesn't appear, then r=me after #41934 is merged.

This commit extends the current unused macro linter to support directives like #[allow(unused_macros)] or #[deny(unused_macros)] directly next to the macro definition, or in one of the modules the macro is inside. Before, we only supported such directives at a per crate level, due to the crate's NodeId being passed to session.add_lint. We also had to implement handling of the macro's NodeId in the lint visitor.

est31 · 2017-05-13T14:05:22Z

okay, ready for review. r? @jseyfried

jseyfried

Looks good! r=me modulo comments

jseyfried · 2017-05-15T00:51:10Z

+ let id_span = match *self.macro_map[did] {
+ SyntaxExtension::NormalTT(_, isp, _) => isp,
+ _ => None
+ };


nit: this is usually formatted
let id_span = match*self.macro_map[did]{SyntaxExtension::NormalTT(_, isp, _) => isp, _ => None};

jseyfried · 2017-05-15T00:54:43Z


 use ops::*;

+#[allow(unused_macros)]


Is there a reason we can't just remove the unused macro here?

There is a FIXME below to get the remaining impls uncommented, including invocations of the macro.

Makes sense.

jseyfried · 2017-05-15T01:01:38Z

+ exp,
+ Some((def.id, def.span)),
+ attr::contains_name(&def.attrs, "allow_internal_unstable")
+ )


nit: nonstandard formatting -- should be three lines with "visual indenting" or indented one block with four/five lines.

jseyfried · 2017-05-15T01:11:27Z

+ // List of macros that we need to warn about as being unused.
+ // The bool is true if the macro is unused, and false if its used.
+ // Setting a bool to false should be much faster than removing a single
+ // element from a FxHashSet.


Could you clarify that this is only for crate-local macro_rules!?

Okay, but hopefully the upcoming macros 2.0 macros could be included as well.

jseyfried · 2017-05-15T01:22:51Z

+ // The bool is true if the macro is unused, and false if its used.
+ // Setting a bool to false should be much faster than removing a single
+ // element from a FxHashSet.
+ unused_macros: FxHashMap<DefId, bool>,


I still think this should be an FxHashSet<DefId> -- I believe the perf difference is negligible here (removing a value from a hash set is already many orders of magnitude faster than the rest the processing we do per used crate-local macro_rules!).

Add lint for unused macros Addresses parts of rust-lang#34938, to add a lint for unused macros. We now output warnings by default when we encounter a macro that we didn't use for expansion. Issues to be resolved before this PR is ready for merge: - [x] fix the NodeId issue described above - [x] remove all unused macros from rustc and the libraries or set `#[allow(unused_macros)]` next to them if they should be kept for some reason. This is needed for successful boostrap and bors to accept the PR. -> rust-lang#41934 - [x] ~~implement the full extent of rust-lang#34938, that means the macro match arm checking as well.~~ *let's not do this for now*

bors · 2017-05-16T13:32:45Z

⌛ Testing commit 6dbd706 with merge 9ccc84d...

bors · 2017-05-16T15:06:14Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-05-16T15:09:36Z


[01:15:04] ---- concurrent_installs stdout ----
[01:15:04] thread 'concurrent_installs' panicked at '
[01:15:04] Expected: execs
[01:15:04] but: exited with exit code: 101
[01:15:04] --- stdout
[01:15:04] [01:15:04] --- stderr
[01:15:04] Updating registry `file:///checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t0/registry`
[01:15:04] Blocking waiting for file lock on the registry index
[01:15:04] error: could not find `foo` in `registry https://github.com/rust-lang/crates.io-index`
[01:15:04] ', /cargo/registry/src/github.com-1ecc6299db9ec823/hamcrest-0.1.1/src/core.rs:31
[01:15:04] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:15:04] [01:15:04] [01:15:04] failures:
[01:15:04] concurrent_installs

I think this was recently fixed, but the update to the cargo submodule hasn't been pulled in yet (if the PR was even merged)

@bors retry

alexcrichton · 2017-05-16T15:16:50Z

FWIW that was fixed by rust-lang/cargo#4051 and we'll get the update in #42039

bors · 2017-05-16T23:27:42Z

⌛ Testing commit 6dbd706 with merge 86319e4...

Add lint for unused macros Addresses parts of #34938, to add a lint for unused macros. We now output warnings by default when we encounter a macro that we didn't use for expansion. Issues to be resolved before this PR is ready for merge: - [x] fix the NodeId issue described above - [x] remove all unused macros from rustc and the libraries or set `#[allow(unused_macros)]` next to them if they should be kept for some reason. This is needed for successful boostrap and bors to accept the PR. -> #41934 - [x] ~~implement the full extent of #34938, that means the macro match arm checking as well.~~ *let's not do this for now*

bors · 2017-05-17T01:57:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 86319e4 to master...

@jseyfried

Extend the unused macro lint to macros 2.0 Extends the unused macro lint (added in PR #41907) to macros 2.0 (added in PR #40847). r? @jseyfried

Implement a lint to warn about unused macro rules This implements a new lint to warn about unused macro rules (arms/matchers), similar to the `unused_macros` lint added by rust-lang#41907 that warns about entire macros. ```rust macro_rules! unused_empty { (hello) => { println!("Hello, world!") }; () => { println!("empty") }; //~ ERROR: 1st rule of macro `unused_empty` is never used } fn main() { unused_empty!(hello); } ``` Builds upon ~~(and currently integrates)~~ rust-lang#96149 and rust-lang#96156. Fixesrust-lang#73576

…enkov Implement a lint to warn about unused macro rules This implements a new lint to warn about unused macro rules (arms/matchers), similar to the `unused_macros` lint added by rust-lang#41907 that warns about entire macros. ```rust macro_rules! unused_empty { (hello) => { println!("Hello, world!") }; () => { println!("empty") }; //~ ERROR: 1st rule of macro `unused_empty` is never used } fn main() { unused_empty!(hello); } ``` Builds upon rust-lang#96149 and rust-lang#96156. Fixesrust-lang#73576

Implement a lint to warn about unused macro rules This implements a new lint to warn about unused macro rules (arms/matchers), similar to the `unused_macros` lint added by rust-lang#41907 that warns about entire macros. ```rust macro_rules! unused_empty { (hello) => { println!("Hello, world!") }; () => { println!("empty") }; //~ ERROR: 1st rule of macro `unused_empty` is never used } fn main() { unused_empty!(hello); } ``` Builds upon rust-lang#96149 and rust-lang#96156. Fixesrust-lang#73576

rust-highfive assigned pnkfelix May 11, 2017

rust-highfive assigned jseyfried and unassigned pnkfelix May 11, 2017

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2017

est31 mentioned this pull request May 12, 2017
Remove unused macros from the codebase #41934
Merged

est31 force-pushed the macro_unused branch 5 times, most recently from 0c72cd2 to a97a1c5Compare May 12, 2017 10:13

est31 added 5 commits May 13, 2017 16:02

Add lint for unused macros
df188b8

Extend the libsyntax visitor to work over macro defs
db82c57

libcore: #[allow] some unused macros
ba0601d

Add test, and fix the other tests
b36d23c

est31 force-pushed the macro_unused branch from a97a1c5 to b36d23cCompare May 13, 2017 14:03

est31 changed the title ~~Add lint for unused macros [WIP]~~Add lint for unused macrosMay 13, 2017

librustc_incremental: remove unused macro in test case
0d0cb27

jseyfried reviewed May 15, 2017
View reviewed changes

Address review comments
25b7f10

Mark-Simulacrum mentioned this pull request May 16, 2017
Rollup of 8 pull requests #42032
Closed

arielb1 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 May 16, 2017

bors merged commit 6dbd706 into rust-lang:masterMay 17, 2017

This was referenced May 17, 2017
Change some notes into suggestions #42033
Merged
Initial implementation of declarative macros 2.0 #40847
Merged

est31 mentioned this pull request May 21, 2017
Unused lint warnings for macros #34938
Closed

malbarbo mentioned this pull request May 24, 2017
Add x86_64-linux-android build support cross-rs/cross#98
Merged

est31 mentioned this pull request May 31, 2017
Extend the unused macro lint to macros 2.0 #42334
Merged

bors added a commit that referenced this pull request Jun 3, 2017
Auto merge of#42334- est31:master, r=jseyfried
a8dc246
Extend the unused macro lint to macros 2.0 Extends the unused macro lint (added in PR #41907) to macros 2.0 (added in PR #40847). r? @jseyfried

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 6, 2017

hcpl mentioned this pull request Sep 6, 2017
"unused #[macro_use] import" false positive in libs and unit-tests #44342
Closed

est31 mentioned this pull request Jun 22, 2020
Warn about unused macro arms. #73576
Closed

est31 mentioned this pull request Apr 17, 2022
Implement a lint to warn about unused macro rules #96150
Merged

Uh oh!

Uh oh!

Conversation

est31 commented May 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 11, 2017

Uh oh!

est31 commented May 11, 2017

Uh oh!

est31 commented May 11, 2017

Uh oh!

est31 commented May 12, 2017

Uh oh!

petrochenkov commented May 13, 2017

Uh oh!

est31 commented May 13, 2017

Uh oh!

jseyfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 16, 2017

Uh oh!

bors commented May 16, 2017

Uh oh!

Mark-Simulacrum commented May 16, 2017

Uh oh!

alexcrichton commented May 16, 2017

Uh oh!

bors commented May 16, 2017

Uh oh!

bors commented May 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

est31 commented May 11, 2017•
edited
Loading