More proc-macro detection

[Jarcho]

fixes #6514
fixes #8683
fixes #6858
fixes #6594

This is a more general way of checking if an expression comes from a macro and could be trivially applied to other lints. Ideally this would be fixed in rustc's proc-macro api, but I don't see that happening any time soon.

changelog: FPs: [unit_arg] [default_trait_access] [missing_docs_in_private_items]: No longer trigger in code generated from proc-macros.

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[Manishearth]

I don't understand: don't we already have utils functions that check for external macros? Spans do get augmented if they're from proc macros

Also this seems to rely on some very specific span behavior in rustc, I'd like to see a lot more tests specific to this.

cc @rust-lang/clippy

[Jarcho]

The proc-macro issue is from using something like quote_spanned with the span of an input token. When this is done the span looks like a regular span, but it's attached to the wrong token. Either rustc needs to tag all those spans as coming from a different token somehow (I don't know if this is possible given the current proc-macro interface), or we need to look at the actual text associated with the span and see if it could possibly match the expected item (e.g. an if expression always has if as it's first token, exempting macros).

[Manishearth]

Ah, I see!

[flip1995]

Do we need to separate the ìs_proc_macro and the in_external_macro functions? I would assume that if we want to prevent linting in proc macros we most likely also want to prevent linting in declarative external macros.

[Manishearth]

A thing to point out is in_external_macro is fast and should be the first thing we run, whereas is_proc_macro may be much slower and probably should be run later in the checks.

[Jarcho]

It shouldn't be so expensive that we can't run them at the same time. in_external_macro is a rustc function, so there's still going to be both functions.

[Manishearth]

@Jarcho right, but check_foo() for each lint is called for each item/expr/etc in the codebase, so it does matter for performance for the things you call first to be very fast operations that can quickly reject things, and heavier stuff goes later on. So while I do think it's a bit more error prone I do feel like this check kinda belongs after the initial AST matching has happened to reject most cases, since this check is doing a lot more in-depth comparisons

[Jarcho]

True. But, in_external_macro should also be called after the initial ast matching as well. It just matters significantly less in that case.

Ideally it would be run once for all lint passes, but that's a totally different problem.

[Manishearth]

Hm, fair!

[Manishearth]

Gonna unassign myself from complex PRs for a while since I'm a bit swamped.

[bors]

☔ The latest upstream changes (presumably #8563) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

Ideally it would be run once for all lint passes, but that's a totally different problem.

I mean we could just stop linting in external macros altogether: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/struct.Lint.html#structfield.report_in_external_macro

rust-clippy/clippy_lints/src/lib.rs

Lines 103 to 108 in c0fce5a

	macro_rules! declare_clippy_lint {
	{ $(#[$attr:meta])* pub $name:tt, style, $description:tt } => {
	declare_tool_lint! {
	$(#[$attr])* pub clippy::$name, Warn, $description, report_in_external_macro: true
	}
	};

I think last time we thought about doing this, we decided to not do it for some reason, I can't remember anymore. Maybe we can continue linting in external macros in correctness and sus lints and don't lint for all other lint groups?

[Manishearth]

@flip1995 that might be good! I think overall for things like, for example, the clippy unwrap lints, folks might want them to lint inside macros. It's also tricky because "external" may still mean "in same workspace".

[flip1995]

for example, the clippy unwrap lints, folks might want them to lint inside macros

Good point, but then external macros would be just like external function calls. Clippy also doesn't look into those. It may make sense then that in the declare_clippy_lint! macro we add an optional external-macros argument so lints can opt-in to external macro linting regardless of the lint group.

It's also tricky because "external" may still mean "in same workspace".

True, but we don't have a good way to check this AFAIK

[bors]

☔ The latest upstream changes (presumably #8918) made this pull request unmergeable. Please resolve the merge conflicts.

[daxpedda]

While fixing missing_const_for_fn, see #8854, I found that your PR basically does half of my work already. So I made Jarcho#1.

Does that make sense or is there no intention to merge this PR? Apart from the need to re-base of course.

[Jarcho]

r? @xFrednet

Turns out nobody is assigned here. Time to bother somebody without a long review queue.

[xFrednet]

Time to bother somebody without a long review queue.

Psst, before somebody else notices xD. Totally fine, I'll add this to my backlog. Would you mind rebasing? 🙃

[xFrednet]

Hey @Jarcho, sadly, I'm a bit swarmed with RL stuff at the moment. It will take a bit longer until I can do a full review, but it's still on my todo list :)

[Jarcho]

Feel free to pass it off to someone else if needed.

[bors]

☔ The latest upstream changes (presumably #9182) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

The implementation looks good to me, I have some smaller suggestions and questions just to make sure that everything is correct. If they have been solved, we can merge this.

Also, thank you for your patience. The next review should be much quicker!

Out of interest, have you done any kind of benchmarks of this implementation? I suspect that it shouldn't add much, if the check is directly before the lint emission. Still, it could be interesting 🙃

[xFrednet]

Looks good to me. Thank you for this implementation, I'm interested to see if we get any new FPs with this new checks 🙃

@bors r+

[bors]

📌 Commit 745b194 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 745b194 with merge 29f3dc3...

[xFrednet]

@bors r-

(The changelog entries are not yet up to day)

[xFrednet]

Updated the changelog entry.

@bors r+

[bors]

📌 Commit 745b194 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 745b194 with merge 10853f7...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 10853f7 to master...