Squash all lints tied to foreign macros by default #52467

alexcrichton · 2018-07-17T18:30:05Z

This PR is a continuation of #49755 (thanks for the initial jump-start @Dylan-DPC!) and is targeted at solving #48855. This change updates the lint infrastructure to, by default, ignore all lints emitted for code that originates in a foreign macro. For example if println!("...") injects some idiomatic warnings these are all ignored by default. The rationale here is that for almost all lints there's no action that can be taken if the code originates from a foreign lint.

Closes #48855
Closes #52483
Closes #52479

rust-highfive · 2018-07-17T18:30:09Z

r? @estebank

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

alexcrichton · 2018-07-17T18:31:52Z

r? @Manishearth

Manishearth

Looks good so far, but it should always output warnings for future incompatibility lints. Folks should know if their code will break, even if it's not something they can fix in their own crate.

alexcrichton · 2018-07-17T18:39:24Z

@Manishearth indeed! That's why this is in the else clause where the previous has to do with future incompatibility (this is also mentioned in the comment added)

oli-obk · 2018-07-17T19:02:48Z

Ideally we'd still emit lints if the macro originates from a crate within the workspace. But that probably requires significant changes to tell us which crates belong to the same workspace

oli-obk · 2018-07-17T19:04:45Z

src/librustc/lint/mod.rs

+/// Returns whether `span` originates in a foreign crate's external macro.
+///
+/// This is used to test whether a lint should be entirely aborted above.
+fn in_external_macro(sess: &Session, span: Span) -> bool {


Can you make this public so we can reuse it from clippy?

Manishearth

lol missed that (was reviewing from phone)

r=me, with the function public

This commit polishes off this new function to compile on newer rustc as well as update and add a suite of test cases to work with this new check for lints.

alexcrichton · 2018-07-17T19:21:10Z

@oli-obk I agree! I also agree though that unfortunately there's no great way to know that in the compiler right now :(

@bors: r=Manishearth

bors · 2018-07-17T19:21:11Z

📌 Commit 8adf08c has been approved by Manishearth

bors · 2018-07-20T08:54:21Z

⌛ Testing commit 8adf08c with merge 4260c8b...

@Dylan-DPC

Squash all lints tied to foreign macros by default This PR is a continuation of #49755 (thanks for the initial jump-start @Dylan-DPC!) and is targeted at solving #48855. This change updates the lint infrastructure to, by default, ignore all lints emitted for code that originates in a foreign macro. For example if `println!("...")` injects some idiomatic warnings these are all ignored by default. The rationale here is that for almost all lints there's no action that can be taken if the code originates from a foreign lint. Closes #48855 Closes #52483 Closes #52479

bors · 2018-07-20T11:10:53Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing 4260c8b to master...

Manishearth · 2018-07-20T20:23:10Z

This ends up breaking a bunch of clippy lints:

Sometimes we lint on an expression when not caring about the contents, it should be okay for that to be a macro -- e.g. if the expression is println!("foo"), the lint gets hidden, even though the lint is about the expression as a whole and not the macro origins
There should be an opt out, we have some lints that lint #[derive()]s
There's some funkyness around compiler desugarings, we're having for i in 1..3 where the lint is on the range getting squashed

overall ISTM it might be better to make this a per-lint thing like how clippy does. Either way, clippy is going to opt out I think.

alexcrichton · 2018-07-20T20:53:53Z

@Manishearth should this check perhaps become a runtime check against a set stored in the session or lint context? That way Clippy would be able to register lints as "please issue a warning despite the foreign-macro-ness"

oli-obk · 2018-07-20T20:57:11Z

I have made it a runtime check and added the two rustc exceptions as a proof of concept: https://github.com/rust-lang/rust/pull/52562/files#diff-837439bcaa26732bb48bcbb0c60716ccL586

they are no longer required due to rust-lang/rust#52467

added function to check if lints belong to an external macro

dd0808d

rust-highfive assigned estebank Jul 17, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2018

alexcrichton force-pushed the lints-and-macros branch from a8a5491 to e8bcc0c Compare July 17, 2018 18:31

rust-highfive assigned Manishearth and unassigned estebank Jul 17, 2018

Manishearth requested changes Jul 17, 2018

View reviewed changes

alexcrichton mentioned this pull request Jul 17, 2018

Add macro checks to lints #48855

Closed

oli-obk reviewed Jul 17, 2018

View reviewed changes

Manishearth approved these changes Jul 17, 2018

View reviewed changes

rustc: Polish off in_external_macro

8adf08c

This commit polishes off this new function to compile on newer rustc as well as update and add a suite of test cases to work with this new check for lints.

alexcrichton force-pushed the lints-and-macros branch from e8bcc0c to 8adf08c Compare July 17, 2018 19:20

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 Jul 17, 2018

alexcrichton added this to the Rust 2018 Preview 2 milestone Jul 17, 2018

alexcrichton added the A-edition-2018-lints Area: lints supporting the 2018 edition label Jul 17, 2018

oli-obk mentioned this pull request Jul 18, 2018

False positive for 2018 unused extern crate in global allocator #52483

Closed

alexcrichton mentioned this pull request Jul 18, 2018

2018 lints: false positive: "extern crate is not idiomatic" #52479

Closed

bors merged commit 8adf08c into rust-lang:master Jul 20, 2018

bors mentioned this pull request Jul 20, 2018

2018 edition ? Kleene operator #51587

Merged

alexcrichton deleted the lints-and-macros branch July 20, 2018 20:53

flip1995 mentioned this pull request Jul 21, 2018

Failing tests caused by rust-lang/rust#52467 rust-lang/rust-clippy#2947

Closed

alexcrichton mentioned this pull request Jul 24, 2018

Macro checks thwart unreachable_pub lint #52665

Open

alexcrichton mentioned this pull request Jul 31, 2018

Epoch lints don't handle macros very well #48704

Closed

alexcrichton mentioned this pull request Sep 5, 2018

#[deny] in expansion of proc macro is ignored #53975

Closed

japaric added a commit to rust-embedded/cortex-m-rt that referenced this pull request Sep 5, 2018

remove #[allow]s

de35ebb

they are no longer required due to rust-lang/rust#52467

ehuss mentioned this pull request Oct 30, 2018

write! is somehow masking the #[must_use] on its result #55516

Closed

This was referenced Oct 17, 2019

Cheat around the unsafe_code lint in the select macro crossbeam-rs/crossbeam#430

Closed

Remove #[allow(unsafe_code)] from select! macro crossbeam-rs/crossbeam#431

Merged

taiki-e mentioned this pull request Nov 21, 2020

DynFuture is unsound ardaku/pasts#3

Closed

andrewhickman mentioned this pull request Apr 21, 2021

With #[context], a function never gets marked unused andrewhickman/fn-error-context#3

Closed

taiki-e mentioned this pull request Oct 12, 2021

Add a strange test for unsafe_code lint. #89821

Merged

jonas-schievink mentioned this pull request Nov 29, 2021

Add option to disable all linting in external macros rust-lang/rust-clippy#407

Open

adamgreig pushed a commit to rust-embedded/cortex-m that referenced this pull request Jan 12, 2022

remove #[allow]s

2f162e2

they are no longer required due to rust-lang/rust#52467

ojeda mentioned this pull request Jan 25, 2022

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

47 tasks

taiki-e mentioned this pull request Nov 30, 2022

Move doc(hidden) items to __private module and make yielder::pair unsafe tokio-rs/async-stream#84

Merged

bjoernQ mentioned this pull request Mar 5, 2024

Runtime interrupt binding esp-rs/esp-hal#1231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Squash all lints tied to foreign macros by default #52467

Squash all lints tied to foreign macros by default #52467

alexcrichton commented Jul 17, 2018 •

edited

Loading

rust-highfive commented Jul 17, 2018

alexcrichton commented Jul 17, 2018

Manishearth left a comment

alexcrichton commented Jul 17, 2018

oli-obk commented Jul 17, 2018

oli-obk Jul 17, 2018

alexcrichton Jul 17, 2018

Manishearth left a comment

alexcrichton commented Jul 17, 2018

bors commented Jul 17, 2018

bors commented Jul 20, 2018

bors commented Jul 20, 2018

Manishearth commented Jul 20, 2018

alexcrichton commented Jul 20, 2018

oli-obk commented Jul 20, 2018

Squash all lints tied to foreign macros by default #52467

Squash all lints tied to foreign macros by default #52467

Conversation

alexcrichton commented Jul 17, 2018 • edited Loading

rust-highfive commented Jul 17, 2018

alexcrichton commented Jul 17, 2018

Manishearth left a comment

Choose a reason for hiding this comment

alexcrichton commented Jul 17, 2018

oli-obk commented Jul 17, 2018

oli-obk Jul 17, 2018

Choose a reason for hiding this comment

alexcrichton Jul 17, 2018

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

alexcrichton commented Jul 17, 2018

bors commented Jul 17, 2018

bors commented Jul 20, 2018

bors commented Jul 20, 2018

Manishearth commented Jul 20, 2018

alexcrichton commented Jul 20, 2018

oli-obk commented Jul 20, 2018

alexcrichton commented Jul 17, 2018 •

edited

Loading