Skip to content

Conversation

@asuto15
Copy link
Contributor

@asuto15 asuto15 commented Nov 21, 2025

close #21063

Summary

This PR refines the remove_parentheses assist’s handling of return-like control-flow expressions, based on the discussion in #21092.

The assist now applies the following rules:

  1. Prevent producing return || …

    • Parentheses around return are not removed when doing so would create return ||
  2. Allow removal under prefix operators

    • Parentheses around return, return <expr>, break, and continue are removed when used under prefix operators like !
      (e.g., !(return)!return, !(return false)!return false).

Tests

This PR adds comprehensive tests covering:

  • All prohibited cases that would form return || … (with and without values, with or without a prefix operator).
  • Valid prefix cases (!(return), !(return false), !(break), !(continue)).
  • Valid binary-operator cases (true || (return)true || return).

@asuto15 asuto15 requested a review from A4-Tacks November 22, 2025 13:20
@asuto15
Copy link
Contributor Author

asuto15 commented Nov 22, 2025

I tried to use BinExpr::can_cast(parent.kind()), but it break the case where !(return) must be not-applicable failed.
This suggests that relying on the parent kind is not appropriate for detecting the closure risk.

@A4-Tacks
Copy link
Member

This suggests that relying on the parent kind is not appropriate for detecting the closure risk.

Yes, this is not very good

@asuto15
Copy link
Contributor Author

asuto15 commented Nov 22, 2025

Based on your feedback, I will revise the approach as follows:

  1. Keep prec.rs responsible only for expression precedence.
    Ret-like expressions will simply return ExprPrecedence::Jump, without inspecting surrounding ParenExpr or next tokens.

  2. Move all syntactic “closure‐risk” checks (e.g., return followed by ||/&&) back into the remove_parentheses assist, where the actual ParenExpr and its neighboring tokens are reliably available.

  3. Avoid relying on the syntax parent inside needs_parens_in_expr, since self, parent, and place_of may not correspond to the concrete AST structure.

@A4-Tacks
Copy link
Member

A4-Tacks commented Nov 22, 2025

2. Move all syntactic “closure‐risk” checks (e.g., `return` followed by `||`/`&&`) back into the `remove_parentheses` assist, where the actual `ParenExpr` and its neighboring tokens are reliably available.

Maybe it can be resolved in the prec.rs

Try checking if self like ReturnExpr, if the parent is BinExpr and if place_of is a subtree of parent.lhs()?

Similar:

&& let Some(node) = place_of.ancestors().find(|it| it.parent().is_none_or(|it| it == parent))
&& let Some(last_token) = node.last_token()
&& let Some(next) = last_token.next_token().and_then(|t| skip_trivia_token(t, Direction::Next))
// ...

@A4-Tacks
Copy link
Member

A similar case:

fn foo() -> bool {
    // Inline variable
    let x$0: bool = return false;
    !x || true;
}

@A4-Tacks
Copy link
Member

Maybe it can be resolved in the prec.rs

This seems to have passed all the tests:

        if self.precedence() == ExprPrecedence::Jump
            && let Some(node) = place_of.ancestors().find(|it| it.parent().is_none_or(|it| &it == parent.syntax()))
            && let Some(next) = node.last_token().and_then(|t| skip_trivia_token(t.next_token()?, Direction::Next))
            && matches!(next.kind(), T![||] | T![&&])
        {
            return true;
        }

@asuto15
Copy link
Contributor Author

asuto15 commented Nov 22, 2025

       if self.precedence() == ExprPrecedence::Jump
            && let Some(node) = place_of.ancestors().find(|it| it.parent().is_none_or(|it| &it == parent.syntax()))
            && let Some(next) = node.last_token().and_then(|t| skip_trivia_token(t.next_token()?, Direction::Next))
            && matches!(next.kind(), T![||] | T![&&])
        {
            return true;
        }

This code works perfectly

@asuto15 asuto15 requested a review from A4-Tacks November 22, 2025 23:39
@asuto15
Copy link
Contributor Author

asuto15 commented Nov 23, 2025

Thank you for reviewing my code so actively!

@ShoyuVanilla ShoyuVanilla added this pull request to the merge queue Nov 23, 2025
Merged via the queue into rust-lang:master with commit e540a44 Nov 23, 2025
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2025
@asuto15 asuto15 deleted the fix/#21063 branch November 23, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive on return for remove_parentheses

4 participants