fix(double_parens): don't lint in proc-macros

[ada4a]

Fixes #15852

changelog: [double_parens]: don't lint in proc-macros

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[github-actions]

No changes for 02e4516

[ada4a]

uhh....

Oh, these are probably caused by the changes the expr_search_pat? I only removed the final )/}/] from the search patterns, as @Jarcho said that they get trimmed away and so shouldn't be included in the search patterns. I used the same endings in ast_expr_search_pat, so it'd be good to know which of the changes I should revert in both of these functions..

[samueltardieu]

I won't have time to review this right now, so let's reroll. r? clippy

[ada4a]

Ohh, apparently only ) are trimmed, not }/]. Let's see if this will work

[zihan0822]

#15940 might also relate to this?

[ada4a]

[...] might also relate to this?

It probably does, but it looks like the is_from_proc_macro check doesn't work in that case for some reason. I'll push the test case I've written; there you can see the derive_double_parens derive macro, which contains ((5)) -- apparently, the (5) and 5 have different SyntaxContexts, and so an early-return, analogous to the following one in expr_search_pat:

rust-clippy/clippy_utils/src/check_proc_macro.rs

Lines 149 to 156 in 8ff0cf8

	// The expression can have subexpressions in different contexts, in which case
	// building up a search pattern from the macro expansion would lead to false positives;
	// e.g. `return format!(..)` would be considered to be from a proc macro
	// if we build up a pattern for the macro expansion and compare it to the invocation `format!()`.
	// So instead we return an empty pattern such that `span_matches_pat` always returns true.
	if !e.span.eq_ctxt(outer_span) {
	return (Pat::Str(""), Pat::Str(""));
	}

is taken, which makes the Pat basically nothing, which makes span_matches_pat always true, which makes is_from_proc_macro always false, which makes us, false-positively, lint.

I must admit I have no idea how to fix this -- maybe it'd be the easiest to just not lint in any code coming from an expansion

[Jarcho]

This shouldn't be trying using is_from_proc_macro. What you'll want to do is:

• use get_source_range on the inner parenthesis,
• check that the first and last character are ( and ).
• check that the end of the text before the range is ( followed by any whitespace (text[..start].trim_end().ends_with('('))
• check that the start of the text after the range is any whitespace followed by ).

[ada4a]

Thank you very much @Jarcho, that worked!

[ada4a]

is this comment correct?

[Jarcho]

If it's an imported source file then it's definitely from a external macro, but that's not the only case that is. You'll still need an in_external_macro check.

You should just SourceFileRange::as_str since that will be changed to not loading external sources. At least on lint relies on this and nobody has thoroughly checked every use to see if there are others.

[ada4a]

You should just SourceFileRange::as_str

I think that wouldn't work because of the consideration on the next line of the comment -- since we want to look not only at the range in sfr but also at the text surrounding it, we'll need to do the slicing manually

You'll still need an in_external_macro check.

It looks like a test case using external! is already not getting linted as is -- do you maybe know why that would be / what other test will point out the need for in_external_macro?

[Jarcho]

It looks like a test case using external! is already not getting linted as is -- do you maybe know why that would be / what other test will point out the need for in_external_macro?

external! just sets all the spans to the proc macro's mixed site span which has a location encompassing the entire call. If it used Span::resolved_at or Span::located_at you could get something which could share the parenthesis' location while still being detected as from an external macro.

[Jarcho]

I think that wouldn't work because of the consideration on the next line of the comment -- since we want to look not only at the range in sfr but also at the text surrounding it, we'll need to do the slicing manually

Right. Ignore that comment then.

[ada4a]

It looks like a test case using external! is already not getting linted as is -- do you maybe know why that would be / what other test will point out the need for in_external_macro?

Sorry, just noticed this -- wouldn't the test case with macro_rules::double_parens be testing exactly that, an external macro? I even tried adding some expressions from the derive test:

#[macro_export]
macro_rules! double_parens {
    ($a:expr, $b:expr, $c:expr, $d:expr) => {{
        let a = ($a);
        let b = ((5));
        let c = std::convert::identity((5));
        InterruptMask((($a.union($b).union($c).union($d)).into_bits()) as u32)
    }};
}

but it sill won't lint...

[ada4a]

Meanwhile if I create and call the same macro in the test file, everything gets linted as expected. So the lint does seem to handle internal/external distinction correctly already -- the answer is how..

[Jarcho]

No. If the source is from a different crate then it's loaded via external_src. I don't think macro_rules macros on their own can cause it.

[Jarcho]

Can you remove the changes to check_proc_macro.rs from this PR. They're no longer needed.

[ada4a]

Done.

Also added in_external_macro checks, and some more external! tests

[Jarcho]

Thank you.

View changes since this review

[flip1995]

rust-lang/rust#149656