Skip to content

Lint precedence possible ambiguity between closure and method call #14421

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+104 −16
Open

Lint precedence possible ambiguity between closure and method call #14421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions clippy_lints/src/precedence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet_with_applicability;
use rustc_ast::ast::BinOpKind::{Add, BitAnd, BitOr, BitXor, Div, Mul, Rem, Shl, Shr, Sub};
use rustc_ast::ast::{BinOpKind, Expr, ExprKind};
Expand All @@ -10,7 +10,8 @@ use rustc_span::source_map::Spanned;
declare_clippy_lint! {
/// ### What it does
/// Checks for operations where precedence may be unclear and suggests to add parentheses.
/// It catches a mixed usage of arithmetic and bit shifting/combining operators without parentheses
/// It catches a mixed usage of arithmetic and bit shifting/combining operators,
/// as well as method calls applied to closures.
///
/// ### Why is this bad?
/// Not everyone knows the precedence of those operators by
Expand Down Expand Up @@ -109,6 +110,19 @@ impl EarlyLintPass for Precedence {
},
_ => (),
}
} else if let ExprKind::MethodCall(method_call) = &expr.kind
&& let ExprKind::Closure(closure) = &method_call.receiver.kind
{
span_lint_and_then(cx, PRECEDENCE, expr.span, "precedence might not be obvious", |diag| {
diag.multipart_suggestion(
"consider parenthesizing the closure",
vec![
(closure.fn_decl_span.shrink_to_lo(), String::from("(")),
(closure.body.span.shrink_to_hi(), String::from(")")),
],
Applicability::MachineApplicable,
);
});
}
}
}
Expand Down
32 changes: 29 additions & 3 deletions tests/ui/precedence.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
#![warn(clippy::precedence)]
#![allow(unused_must_use, clippy::no_effect, clippy::unnecessary_operation)]
#![allow(clippy::identity_op)]
#![allow(clippy::eq_op)]
#![allow(
unused_must_use,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::clone_on_copy,
clippy::identity_op,
clippy::eq_op
)]

macro_rules! trip {
($a:expr) => {
Expand Down Expand Up @@ -35,3 +40,24 @@ fn main() {
let b = 3;
trip!(b * 8);
}

struct W(u8);
impl Clone for W {
fn clone(&self) -> Self {
W(1)
}
}

fn closure_method_call() {
// Do not lint when the method call is applied to the block, both inside the closure
let f = |x: W| { x }.clone();
assert!(matches!(f(W(0)), W(1)));

let f = (|x: W| -> _ { x }).clone();
assert!(matches!(f(W(0)), W(0)));
//~^^ precedence

let f = (move |x: W| -> _ { x }).clone();
assert!(matches!(f(W(0)), W(0)));
//~^^ precedence
Comment on lines +60 to +62
Copy link

@traviscross traviscross Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let f = (move |x: W| -> _ { x }).clone();
assert!(matches!(f(W(0)), W(0)));
//~^^ precedence

I'd prefer we not do this one. In my model, the one that's "wrong" is the |x| { .. }.something() case, because it's reasonable to assume that opening brace triggers the parsing of a block, when it doesn't. This one, by contrast, does the "right" thing, and with the annotated return type there, which then requires those braces, I think it's reasonably clear to the reader that the closure body must terminate with the closing brace.

That is, by linting against the one that's "wrong", as above, and suggesting the return type be annotated to make it unambiguous, probably we've solved the problem.

There's also a practical reason I don't want us pushing the parens here. I'm interested in us later implementing IntoFuture for thunk async closures. This would then allow writing async || -> _ { .. }.into_future(), and I think it'd be better ergonomics for those to not require a set of parens around them to please clippy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one, by contrast, does the "right" thing

I find async || -> _ { .. }.into_future() quite ambiguous and would have a hard time parsing it if I saw it in code, so I am in favor of requiring more explicit parentheses here. Or rather, I'd probably write this as IntoFuture::into_future(async || { ... }).

Copy link

@traviscross traviscross Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I find that || -> _ { .. }(), which we accept without linting, feels the same to me, so I'd prefer to lint against both or neither.

And for both, I feel like they look worse in abstract form than they would often in practice, e.g. with the block split over multiple lines or the return type ascribed in other ways.

Copy link

@traviscross traviscross Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broader point is that || -> _ { .. }// anything here shouldn't feel ambiguous to us because the only thing that can follow the -> _ is a block, so it's completely unambiguous where that block and therefore that closure body ends.

The only reason that it might feel ambiguous to us (I claim) is because of what we did with || { .., where the opening brace doesn't force block parsing. I would like us to move away from that. Perhaps we could lint hard against anything that relies on it. Maybe over an edition we could change the rule.

My hope is that in pushing in that direction, || -> _ { .. }// anything here won't feel ambiguous to us any longer.

Copy link
Member

@RalfJung RalfJung Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| -> _ { .. }// anything here shouldn't feel ambiguous to us because the only thing that can follow the -> _ is a block

The fact that -> _ affects parsing like that is arcane knowledge most people don't have. I heard about it the first time ~2 weeks ago. The entire point of this PR is to make it so people can parse code correctly regardless of whether they possess this arcane knowledge or not.

The only reason that it might feel ambiguous to us (I claim) is because of what we did with || { .., where the opening brace doesn't force block parsing. I would like us to move away from that. Perhaps we could lint hard against anything that relies on it. Maybe over an edition we could change the rule.

Ah, so you are saying we could make || -> _ { actually consistent with || {? Yeah that seems nice, and should definitely be possible with an edition transition.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

On the other, all I'm saying is that people know that || -> u64 42 doesn't work, and I don't think people expect it to, and so I think their normal intuitions about blocks kick in. What messes this up and makes it arcane is the other one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normal intuitions about blocks kick in

My intuition is surprised whenever these end of block cases pop up, for example that the parens are required in

fn f() -> Y {
    (unsafe { x } as Y)
}

Similarly, the current parsing of |x: String| {x}.clone() matches my intuition

}
32 changes: 29 additions & 3 deletions tests/ui/precedence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
#![warn(clippy::precedence)]
#![allow(unused_must_use, clippy::no_effect, clippy::unnecessary_operation)]
#![allow(clippy::identity_op)]
#![allow(clippy::eq_op)]
#![allow(
unused_must_use,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::clone_on_copy,
clippy::identity_op,
clippy::eq_op
)]

macro_rules! trip {
($a:expr) => {
Expand Down Expand Up @@ -35,3 +40,24 @@ fn main() {
let b = 3;
trip!(b * 8);
}

struct W(u8);
impl Clone for W {
fn clone(&self) -> Self {
W(1)
}
}

fn closure_method_call() {
// Do not lint when the method call is applied to the block, both inside the closure
let f = |x: W| { x }.clone();
assert!(matches!(f(W(0)), W(1)));

let f = |x: W| -> _ { x }.clone();
assert!(matches!(f(W(0)), W(0)));
//~^^ precedence

let f = move |x: W| -> _ { x }.clone();
assert!(matches!(f(W(0)), W(0)));
//~^^ precedence
}
38 changes: 30 additions & 8 deletions tests/ui/precedence.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: operator precedence might not be obvious
--> tests/ui/precedence.rs:16:5
--> tests/ui/precedence.rs:21:5
|
LL | 1 << 2 + 3;
| ^^^^^^^^^^ help: consider parenthesizing your expression: `1 << (2 + 3)`
Expand All @@ -8,40 +8,62 @@ LL | 1 << 2 + 3;
= help: to override `-D warnings` add `#[allow(clippy::precedence)]`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:18:5
--> tests/ui/precedence.rs:23:5
|
LL | 1 + 2 << 3;
| ^^^^^^^^^^ help: consider parenthesizing your expression: `(1 + 2) << 3`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:20:5
--> tests/ui/precedence.rs:25:5
|
LL | 4 >> 1 + 1;
| ^^^^^^^^^^ help: consider parenthesizing your expression: `4 >> (1 + 1)`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:22:5
--> tests/ui/precedence.rs:27:5
|
LL | 1 + 3 >> 2;
| ^^^^^^^^^^ help: consider parenthesizing your expression: `(1 + 3) >> 2`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:24:5
--> tests/ui/precedence.rs:29:5
|
LL | 1 ^ 1 - 1;
| ^^^^^^^^^ help: consider parenthesizing your expression: `1 ^ (1 - 1)`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:26:5
--> tests/ui/precedence.rs:31:5
|
LL | 3 | 2 - 1;
| ^^^^^^^^^ help: consider parenthesizing your expression: `3 | (2 - 1)`

error: operator precedence might not be obvious
--> tests/ui/precedence.rs:28:5
--> tests/ui/precedence.rs:33:5
|
LL | 3 & 5 - 2;
| ^^^^^^^^^ help: consider parenthesizing your expression: `3 & (5 - 2)`

error: aborting due to 7 previous errors
error: precedence might not be obvious
--> tests/ui/precedence.rs:56:13
|
LL | let f = |x: W| -> _ { x }.clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: consider parenthesizing the closure
|
LL | let f = (|x: W| -> _ { x }).clone();
| + +

error: precedence might not be obvious
--> tests/ui/precedence.rs:60:13
|
LL | let f = move |x: W| -> _ { x }.clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: consider parenthesizing the closure
|
LL | let f = (move |x: W| -> _ { x }).clone();
| + +

error: aborting due to 9 previous errors