-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: never remove parens from prefix ops with valueless return/break/continue #21092
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
fix: never remove parens from prefix ops with valueless return/break/continue #21092
Conversation
|
@asuto15 actually I started working on the issue and completed earlier than you but could not open a pr due to time constraint. So, I opened it anyway. |
ShoyuVanilla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
This does not solve issue, such as this test: check_assist_not_applicable(
remove_parentheses,
r#"fn main() { let _x = true && $0(return) || true; }"#,
);
check_assist(
remove_parentheses,
r#"fn f() { cond && !$0(return) }"#,
r#"fn f() { cond && !return }"#,
); |
This comment was marked as resolved.
This comment was marked as resolved.
Yes, this is intended behavior. I missed typing a character |
|
Thank you! One more question for the maintainers @ShoyuVanilla and @A4-Tacks: Would you prefer that we treat all valueless control-flow expressions ( For example, allowing: fn f() { !($0(return)) } // -> !return
fn f() { !($0(break)) } // -> !break
fn f() { !($0(continue)) } // -> !continue(except the This would give a simple, consistent rule. |
Consider this test: check_assist_not_applicable(
remove_parentheses,
r#"fn main() { let _x = true && $0(return false) || true; }"#,
);
check_assist(
remove_parentheses,
r#"fn f() { cond && !$0(return false) }"#,
r#"fn f() { cond && !return false }"#,
); |
Oh, good catch |
|
Sorry I missed this discussion. Does this pr not fixes the issue or any edge case? |
The main issue is BinExpr instead of PrefixExpr |
fixes #21063
The
Remove redundant parentheseswas suggesting removal of parentheses in expressions like !(return), !(break), and !(continue), which would result in invalid syntax.So, added a case in
Expr::needs_parens_in()which checks for two things:1.) Prefix operator
2.) is_ret_like_with_no_value() which handles return, yield, break, continue, become
If both are present, the parenthesis are not removed.
Added test also.