handle paren in macro expand for let-init-else expr

[bvanjoi]

Fixes #131655

This PR modifies the codegen logic of the macro expansion within let-init-else expression:

• Before: The expression let xxx = (mac! {}) else {} expands to let xxx = (expanded_ast) else {}.
• After: The same expression expands to let xxx = expanded_ast else {}.

An alternative solution to this issue could involve handling the source code directly when encountering unused parentheses in let-init-else expressions. However, this approach might be more cumbersome due to the absence of the necessary data structure.

r? @petrochenkov

[cjgillot]

This seems like a far reaching change because of a buggy lint.
Would a targeted exception on the lint itself be simpler?

[bvanjoi]

Would a targeted exception on the lint itself be simpler?

If we follow this thought, we need to check the original AST for init_expr in let-init-else when encountering unused_paren. There are two possible approaches:

• Parse the expression again to check if it is a macro call with braces.
• Record the node_id if the paren expression contains a macro call with braces during expansion.

[petrochenkov]

This is an issue with any context that restricts braces, right?
Like plain if:

fn main() {
    macro_rules! x {
        () => { true }
    }

    if (x! {}) {} // warning: unnecessary parentheses around `if` condition
}

See NO_STRUCT_LITERAL in the parser for all such contexts.
IIRC, braced macro calls like mac! {} are currently prohibited in all such contexts for consistency with other braced syntaxes (even if it's not technically necessary) due to a relatively recent lang team decision.

[dtolnay]

if (x! {}) {} is actually unnecessary parentheses. if x! {} {} is valid syntax.

[petrochenkov]

Perhaps the lint should look at span of the expression inside the parentheses, check what macro expansion it comes from, and suppress the warning in suspicious cases.
The current implementation in this PR is certainly not the way to go.

IIRC @dtolnay was also recently working on pretty-printing / linting (un)necessary parentheses, maybe he has more suggestions.

[bvanjoi]

Update: It now stores the span of necessary parenthesis nodes and uses it when checking for unused_parens lint.

[bvanjoi]

@rustbot ready

[petrochenkov]

Sorry for the long delay.

I think the current version is still too much.
This can probably be fixed by a simpler heuristic local to compiler/rustc_lint/src/unused.rs - in if expr { ... } we look 1) at the span of expr and 2) at the span of the whole if expr { ... }, and if syntactic contexts of the spans do not match then we suppress the lint.
Something like Span::contains or similar could also be used instead of context comparison.
Other lints and diagnostics are full of checks like this.
@rustbot author

[alex-semenyuk]

@bvanjoi
Thanks for your contribution
From wg-triage. Any updates on this PR? Also could you please rebase

[bvanjoi]

This can probably be fixed by a simpler heuristic local

The root cause of this issue is that information gets lost during macro expansion. For example, the expression let Some(_) = (x! {}) else { return }; expands into let Some(_) = (None::<i32>) else { return }, making it impossible to determine whether None::<i32> originated from macro expansion or was written directly by the user. Is there any way to distinguish between the two? I’ll look into it.

[bvanjoi]

The logic is now straightforward: if both the parenthesized expression and its inner expression originate from the same context, we continue checking. Otherwise, we stop.

[bvanjoi]

@rustbot ready

[petrochenkov]

Thanks!
@bors r+

[bors]

📌 Commit e9d2fef has been approved by petrochenkov

It is now in the queue for this repository.