Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable all linting in external macros #407

Open
jFransham opened this issue Oct 22, 2015 · 16 comments
Open

Add option to disable all linting in external macros #407

jFransham opened this issue Oct 22, 2015 · 16 comments
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started T-macros Type: Issues with macros and macro expansion

Comments

@jFransham
Copy link

I use Clippy a lot (even writing a Syntastic extension for it) but it seems overly harsh on code that is generated by macros. For example, the Oak parser generator uses closures to unroll tuple arguments into regular arguments, but if you have single-argument functions the linter will throw a redundant closure check - not something that could be fixed by the library author or the user (as far as I know). If there was an option to disable this, the pedant inside me who likes to get rid of all warnings would be happy and I could continue tapping keys and fighting crime.

@llogiq
Copy link
Contributor

llogiq commented Oct 22, 2015

I'm entirely OK with this proposal, it's not even that hard – only someone needs to find the time to look through all passes and introduce the macro checks wherever they're missing.

Keep in mind that we have some lints that should not trigger within macros at all (that's why we have two methods in utils). Should macros deactivate all readability-related lints regardless of origin?

@jFransham
Copy link
Author

Can we check the inputs to macros and see if the problem is in the input? This would, for example, catch println!("{}", (x | 1) > 2) and other problems that would be false negatives if all lints were disabled in macros. What I'm thinking is, parse each of the macro's variables as ASTs (sans type information etc.) and run the lints that only need an AST on those. If I get time over the next week I'll work on doing a pull request for this.

It won't catch problems in calls to macros that duplicate rust syntax - stuff like (fn $name($first, $second) => $expr) => {} (you see stuff like this a fair bit in DSL-type macros) or syntax extensions (maybe? I haven't written one so I don't know if they specify inputs like macros do) but it'd be a really nice feature to have.

EDIT: accidentally clicked "close and comment"

@jFransham jFransham reopened this Oct 23, 2015
@Manishearth
Copy link
Member

It's a bit hard to generalize that. Lints don't run pre-expansion

Currently we have a set of lints which are just turned off when in a macro. We could add a feature to turn off all lints in macros.

@llogiq
Copy link
Contributor

llogiq commented Oct 23, 2015

Somehow rustc can usually report which macros expanded what, so the information is clearly available. It's just not reachable from the lint for now. Perhaps we should lobby to get access to it?

@Manishearth
Copy link
Member

ExpnInfo knows about it. We have access, it's just hard to unroll and work with.

@leoyvens
Copy link

needless_return currently triggers on macro expansions, it shouldn't since it's readability related and "fixing it" can break code elsewhere.

@Manishearth
Copy link
Member

Yes, that should be unconditionally off in macros.

I wonder if we can be smarter about our macro checks (as well as providing more control to the user about tweaking them). For example, the return lint should only trigger if the return and the surrounding fn item are from different macro contexts.

@llogiq
Copy link
Contributor

llogiq commented Dec 13, 2015

Yeah, but even same_expansion_ctx(nodeid, nodeid) is a bit complex due to expansion chaining. It's not enough to have the same expansion somewhere (or everything in the same for loop would match).

@jFransham
Copy link
Author

It's worth noting that IIRC the compiler team are planning an overhaul of the macro semantics and syntax, if this is true then it might be worth waiting to implement this.

@leoyvens
Copy link

@jFransham True, however the existing macros will take a while to migrate, so the current macro system will still be relevant.

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label May 10, 2017
@rcoh
Copy link
Contributor

rcoh commented Apr 9, 2018

What's the status of this? I'm running into the same issue (although now with nom). Have the compiler internals caught up?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2018

It's being worked on in the compiler: rust-lang/rust#49755

@phansch phansch added the T-macros Type: Issues with macros and macro expansion label Apr 9, 2018
@sbeyer
Copy link

sbeyer commented Oct 11, 2021

Activity or mentions here in every year since 2015. Now let's add 2021 to the list: I stumbled over this issue yesterday and while checking if I could disable lints in macro expansions, I found this issue.

My case is very simple. The macro (from a crate that I use) calls into() on some input arguments. So cargo clippy brought up some useless conversion warnings. Because I did not want to disable that useful warning globally, I had to put #[allow(clippy::useless_conversion)] above every use of that macro.

I think this is a situation that is very natural to have in a macro and to make clear why it could be nicer for the users to ignore lints within macro expansions, at least if they come from a crate.

As a side note: having to add some #[allow(...)] lines was not the worst thing! The worst thing was the initial confusion the clippy warning gave me, because clippy did not show me the expanded macro... for me, the macro was simply a black-box. So, at first sight, I had no idea why there was a useless conversion. So providing a glance into the macro expansion could also have helped – maybe.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2021

We could have a crate wide attribute that silences all clippy lints with spans from external macros and add a help explaining the macro situation (and the crate wide attr) if the attribute is not set

@sbeyer
Copy link

sbeyer commented Oct 11, 2021

@oli-obk That would be the best solution (from my user perspective). I have no idea how hard that is to implement.

@jonas-schievink
Copy link
Contributor

rust-lang/rust#52467 has been merged a long time ago, but some Clippy lints still incorrectly fire for code generated by foreign macros (for example in #4861 and knurling-rs/defmt#642).

Indy2222 added a commit to DigitalExtinction/Game that referenced this issue Jul 1, 2022
We started to get a Clippy error in CI:

error: call to `std::mem::forget` with a value that does not implement `Drop`. Forgetting such a type is the same as dropping it
  --> crates/spawner/src/spawner.rs:19:10
   |
19 | #[derive(Bundle)]
   |          ^^^^^^
   |
   = note: `-D clippy::forget-non-drop` implied by `-D warnings`
note: argument has type `spawner::Spawn`
  --> crates/spawner/src/spawner.rs:19:10
   |
19 | #[derive(Bundle)]
   |          ^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#forget_non_drop
   = note: this error originates in the derive macro `Bundle` (in Nightly builds, run with -Z macro-backtrace for more info)

The failing lint was recently added in
rust-lang/rust-clippy#8630. Unfortunately, it is
not possible to disable linting of code generated by external macros
rust-lang/rust-clippy#407.

The issue was reported to Bevy team here
bevyengine/bevy#5166.
Indy2222 added a commit to DigitalExtinction/Game that referenced this issue Jul 1, 2022
We started to get a Clippy error in CI:

error: call to `std::mem::forget` with a value that does not implement `Drop`. Forgetting such a type is the same as dropping it
  --> crates/spawner/src/spawner.rs:19:10
   |
19 | #[derive(Bundle)]
   |          ^^^^^^
   |
   = note: `-D clippy::forget-non-drop` implied by `-D warnings`
note: argument has type `spawner::Spawn`
  --> crates/spawner/src/spawner.rs:19:10
   |
19 | #[derive(Bundle)]
   |          ^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#forget_non_drop
   = note: this error originates in the derive macro `Bundle` (in Nightly builds, run with -Z macro-backtrace for more info)

The failing lint was recently added in
rust-lang/rust-clippy#8630. Unfortunately, it is
not possible to disable linting of code generated by external macros
rust-lang/rust-clippy#407.

The issues on Bevy site is tracked here
bevyengine/bevy#4601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

No branches or pull requests

9 participants