-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Fix linting false positive when block used as value #141987
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -656,6 +656,25 @@ trait UnusedDelimLint { | |
| is_kw: bool, | ||
| ); | ||
|
|
||
| #[inline] | ||
| fn impacts_followed_by_block(e: &ast::Expr) -> bool { | ||
| return match e.kind { | ||
| ExprKind::Block(..) | ExprKind::Call(..) | ExprKind::MethodCall(..) => true, | ||
| _ => Self::is_followed_by_block(e), | ||
| }; | ||
| } | ||
|
|
||
| #[inline] | ||
| fn is_followed_by_block(e: &ast::Expr) -> bool { | ||
| return match e.kind { | ||
| ExprKind::If(ref cond, ..) if !matches!(cond.kind, ExprKind::Let(..)) => true, | ||
| ExprKind::While(ref cond, ..) if !matches!(cond.kind, ExprKind::Let(..)) => true, | ||
| ExprKind::ForLoop { .. } => true, | ||
| ExprKind::Match(_, _, ast::MatchKind::Prefix) => true, | ||
| _ => false, | ||
| }; | ||
| } | ||
|
|
||
| fn is_expr_delims_necessary( | ||
| inner: &ast::Expr, | ||
| ctx: UnusedDelimsCtx, | ||
|
|
@@ -1475,7 +1494,15 @@ declare_lint! { | |
| "unnecessary braces around an expression" | ||
| } | ||
|
|
||
| declare_lint_pass!(UnusedBraces => [UNUSED_BRACES]); | ||
| #[derive(Default)] | ||
| pub(crate) struct UnusedBraces { | ||
| // Used for tracking parent expressions that would immediately followed | ||
| // by block. Storing false here indicates that expression itself is Block | ||
| // expression. This is meant for to prevent report false positive cases. | ||
| followed_by_block: Vec<bool>, | ||
| } | ||
|
|
||
| impl_lint_pass!(UnusedBraces => [UNUSED_BRACES]); | ||
|
|
||
| impl UnusedDelimLint for UnusedBraces { | ||
| const DELIM_STR: &'static str = "braces"; | ||
|
|
@@ -1505,6 +1532,11 @@ impl UnusedDelimLint for UnusedBraces { | |
| // - the block does not have a label | ||
| // - the block is not `unsafe` | ||
| // - the block contains exactly one expression (do not lint `{ expr; }`) | ||
| // - however, this does not lint if block is immediately followed by parent | ||
| // expression's block, e.g. `if` and `match`, which may cause false positive. | ||
| // ``` | ||
| // if return { return } {} else {} | ||
| // ``` | ||
| // - `followed_by_block` is true and the internal expr may contain a `{` | ||
| // - the block is not multiline (do not lint multiline match arms) | ||
| // ``` | ||
|
|
@@ -1524,9 +1556,18 @@ impl UnusedDelimLint for UnusedBraces { | |
| // let _: A<{produces_literal!()}>; | ||
| // ``` | ||
| // FIXME(const_generics): handle paths when #67075 is fixed. | ||
| // if ctx == UnusedDelimsCtx::FunctionArg { | ||
| // println!("{:?}", self.followed_by_block.last().copied().unwrap_or(false)) | ||
| // } | ||
|
Comment on lines
+1559
to
+1561
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? 😁
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to remove 😅 |
||
| if let [stmt] = inner.stmts.as_slice() | ||
| && let ast::StmtKind::Expr(ref expr) = stmt.kind | ||
| && !Self::is_expr_delims_necessary(expr, ctx, followed_by_block) | ||
| && ((ctx == UnusedDelimsCtx::FunctionArg || ctx == UnusedDelimsCtx::MethodArg) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this check for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is meant to check
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, that sucks :/ in a sense the enum FollowedByBlock {
Yes,
Inherit,
No,
}and we only check the last
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or hmm, wait, couldn't we have a trait UnusedDelimLint {
fn followed_by_block(&mut self) -> &mut Vec<bool>;
}and also set + unset stuff in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kind of modification makes things messy in my opinion, I have to admit that this is a good idea though. Although I have noticed that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. function args always get checked with |
||
| || !Self::is_expr_delims_necessary( | ||
| expr, | ||
| ctx, | ||
| self.followed_by_block.last().copied().unwrap_or(false), | ||
| )) | ||
| && (ctx != UnusedDelimsCtx::AnonConst | ||
| || (matches!(expr.kind, ast::ExprKind::Lit(_)) | ||
| && !expr.span.from_expansion())) | ||
|
|
@@ -1564,6 +1605,10 @@ impl EarlyLintPass for UnusedBraces { | |
| fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) { | ||
| <Self as UnusedDelimLint>::check_expr(self, cx, e); | ||
|
|
||
| if <Self as UnusedDelimLint>::impacts_followed_by_block(e) { | ||
| self.followed_by_block.push(<Self as UnusedDelimLint>::is_followed_by_block(e)); | ||
| } | ||
|
|
||
| if let ExprKind::Repeat(_, ref anon_const) = e.kind { | ||
| self.check_unused_delims_expr( | ||
| cx, | ||
|
|
@@ -1577,6 +1622,18 @@ impl EarlyLintPass for UnusedBraces { | |
| } | ||
| } | ||
|
|
||
| fn check_expr_post(&mut self, _cx: &EarlyContext<'_>, e: &ast::Expr) { | ||
| if <Self as UnusedDelimLint>::impacts_followed_by_block(e) { | ||
| let followed_by_block = | ||
| self.followed_by_block.pop().expect("check_expr and check_expr_post must balance"); | ||
| assert_eq!( | ||
| followed_by_block, | ||
| Self::is_followed_by_block(e), | ||
| "check_expr, check_ty, and check_expr_post are called, in that order, by the visitor" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn check_generic_arg(&mut self, cx: &EarlyContext<'_>, arg: &ast::GenericArg) { | ||
| if let ast::GenericArg::Const(ct) = arg { | ||
| self.check_unused_delims_expr( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //@ run-rustfix | ||
| //@ check-pass | ||
| #[allow(unreachable_code)] | ||
| #[allow(unused)] | ||
| #[warn(unused_braces)] | ||
| #[warn(unused_parens)] | ||
|
|
||
| fn main() { | ||
| return return; //~ WARN: unnecessary braces | ||
| if f(return) {} else {} //~ WARN: unnecessary braces | ||
| if return { return } { return } else { return } | ||
| match return { return } { | ||
| _ => { return } | ||
| } | ||
| while return { return } {} | ||
| } | ||
|
|
||
| fn f(_a: ()) -> bool { | ||
| true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //@ run-rustfix | ||
| //@ check-pass | ||
| #[allow(unreachable_code)] | ||
| #[allow(unused)] | ||
| #[warn(unused_braces)] | ||
| #[warn(unused_parens)] | ||
|
|
||
| fn main() { | ||
| return { return }; //~ WARN: unnecessary braces | ||
| if f({ return }) {} else {} //~ WARN: unnecessary braces | ||
| if return { return } { return } else { return } | ||
| match return { return } { | ||
| _ => { return } | ||
| } | ||
| while return { return } {} | ||
| } | ||
|
|
||
| fn f(_a: ()) -> bool { | ||
| true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| warning: unnecessary braces around `return` value | ||
| --> $DIR/unused-braces-respect-parent-block-issue-141783.rs:9:12 | ||
| | | ||
| LL | return { return }; | ||
| | ^^ ^^ | ||
| | | ||
| note: the lint level is defined here | ||
| --> $DIR/unused-braces-respect-parent-block-issue-141783.rs:5:8 | ||
| | | ||
| LL | #[warn(unused_braces)] | ||
| | ^^^^^^^^^^^^^ | ||
| help: remove these braces | ||
| | | ||
| LL - return { return }; | ||
| LL + return return; | ||
| | | ||
|
|
||
| warning: unnecessary braces around function argument | ||
| --> $DIR/unused-braces-respect-parent-block-issue-141783.rs:10:10 | ||
| | | ||
| LL | if f({ return }) {} else {} | ||
| | ^^ ^^ | ||
| | | ||
| help: remove these braces | ||
| | | ||
| LL - if f({ return }) {} else {} | ||
| LL + if f(return) {} else {} | ||
| | | ||
|
|
||
| warning: 2 warnings emitted | ||
|
|
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.
these two bullet points should be unified. separately, why is your added comment indented?
I think it's
iformatch, and the internal expression may contain a{is that right?
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.
Yes, this should be the intended way to describe this scenario