Lint precedence possible ambiguity between closure and method call #14421

samueltardieu · 2025-03-16T22:36:58Z

|x: String| {x}.clone() applies clone() to {x}, while |x: String| -> String {x}.clone() applies .clone() to the closure because a closure with an explicit return type requires a block as its body.

This extends the precedence lint to suggest using parentheses around the closure definition in the second case, when .clone() would apply to the whole closure.

changelog: [precedence]: warn about ambiguity when a closure is used as a method call receiver

Related discussion

rustbot · 2025-03-16T22:37:02Z

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

traviscross · 2025-03-17T05:51:36Z

tests/ui/precedence.fixed

+}
+
+fn closure_method_call() {
+    let f = move |x: W| { { x }.clone() };


This part of the lint would conflict with the following lint for which lang FCP has been proposed:

Add checking for unnecessary delims in closure body rust#136906

Probably what I'd suggest instead is annotating the return type to make the outer block braces required:

Suggested change

let f = move |x: W| { { x }.clone() };

let f = move |x: W| -> _ { { x }.clone() };

As rust-lang/rust#136906 (comment) mentions rustfmt will remove the extra braces from |x: W| { { x }.clone() } so we definitely can't suggest that

There's a proposal about how to handle that described here and here.

traviscross · 2025-03-17T06:02:10Z

tests/ui/precedence.fixed

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


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.

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 || { ... }).

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.

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.

|| -> _ { .. }// 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.

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.

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

A discrepancy between the `cmake` version available on the runners and the one required by the `aws-lc-sys` dependency prevents the crate from building.

`|x: String| {x}.clone()` applies `clone()` to {x}, while `|x: String| -> String {x}.clone()` applies `.clone()` to the closure because a closure with an explicit return type requires a block. This extends the `precedence` lint to suggest using parentheses around the closure definition when the closure would be cloned, as in `(|x: String| -> String {x}).clone()`.

samueltardieu · 2025-04-01T18:05:07Z

The current state of the proposed change right now is to suggests using parentheses when the method call would apply to the closure itself, and only in this case. That would be, in the top example, (|x: String| -> String { x }).clone().

traviscross · 2025-04-01T23:18:47Z

There's also a proposal on the table here for rustfmt to disambiguate closures that start with an inner block and then have an ambiguous end.

With that in place the following

fn main() {
    _ = || { { 0 }.clone() };
}

is formatted like this.

fn main() {
    _ = || {
        { 0 }.clone()
    };

RalfJung · 2025-04-02T06:01:19Z

There's also a proposal on the table rust-lang/rust#136906 (comment) for rustfmt to disambiguate closures that start with an inner block and then have an ambiguous end.

The original form there is already disambiguated, so I don't think this helps with the problem this lint is trying to address.

EDIT: Oh I see this is about now having rustfmt work against disambiguation here. We'd still need the lint, but the lint can now suggest to add braces since rustfmt will not remove them any more. Is that what you mean?

traviscross · 2025-04-02T06:45:25Z

We'd still need the lint, but the lint can now suggest to add braces since rustfmt will not remove them any more. Is that what you mean?

That's right.

samueltardieu · 2025-04-02T07:13:02Z

There's also a proposal on the table rust-lang/rust#136906 (comment) for rustfmt to disambiguate closures that start with an inner block and then have an ambiguous end.

The original form there is already disambiguated, so I don't think this helps with the problem this lint is trying to address.

EDIT: Oh I see this is about now having rustfmt work against disambiguation here. We'd still need the lint, but the lint can now suggest to add braces since rustfmt will not remove them any more. Is that what you mean?

samueltardieu · 2025-04-02T07:15:04Z

Sorry, closed by mistake and reopened.

I'll mark this PR as blocked until the rustfmt one is closed. If it's merged, I'll readd add-ing brackets around the block.

@rustbot label +S-blocked +S-waiting-on-author -S-waiting-on-review

Alexendoo · 2025-04-02T11:30:19Z

Doesn't need to be considered blocked, we can revisit the braces case at a later date separately

samueltardieu · 2025-04-02T16:05:19Z

Doesn't need to be considered blocked, we can revisit the braces case at a later date separately

Sure.
@rustbot label -S-blocked +S-waiting-on-review

Alexendoo · 2025-04-08T01:58:47Z

Did a small survey with some GitHub searches, I managed to find three cases in the wild:

Interestingly the last one has (|| {}).foo() and || -> _ {}.foo() in the same file

So an in context comparison would be

        |stream: ParseStream| -> syn::Result<Self> { Self::parse(stream) }.parse2(input)

        (|stream: ParseStream| -> syn::Result<Self> { Self::parse(stream) }).parse2(input)

|| -> _ { .. }() is much more common https://github.com/search?q=lang%3Ars+%2F%5C%7C+-%3E.*%5C%7B.*%5C%7D%5C%28%5B%5E%7B%7D%5Cn%5D*%24%2F&type=code

rustbot assigned Alexendoo Mar 16, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 16, 2025

traviscross reviewed Mar 17, 2025

View reviewed changes

Alexendoo added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Mar 30, 2025

samueltardieu added 2 commits April 1, 2025 19:58

Do not build tokio-rustls in the CI for the time being

57f4f5d

A discrepancy between the `cmake` version available on the runners and the one required by the `aws-lc-sys` dependency prevents the crate from building.

samueltardieu force-pushed the push-srutyqvvxsnn branch from 579b5d6 to c1afad3 Compare April 1, 2025 18:01

samueltardieu removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Apr 1, 2025

samueltardieu closed this Apr 2, 2025

samueltardieu reopened this Apr 2, 2025

samueltardieu removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 2, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels Apr 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint precedence possible ambiguity between closure and method call #14421

Lint precedence possible ambiguity between closure and method call #14421

samueltardieu commented Mar 16, 2025 •

edited

Loading

rustbot commented Mar 16, 2025

traviscross Mar 17, 2025

Alexendoo Mar 17, 2025

traviscross Apr 1, 2025

traviscross Mar 17, 2025 •

edited

Loading

RalfJung Mar 17, 2025

traviscross Mar 17, 2025 •

edited

Loading

traviscross Mar 17, 2025 •

edited

Loading

RalfJung Mar 17, 2025 •

edited

Loading

traviscross Mar 17, 2025

Alexendoo Mar 17, 2025

samueltardieu commented Apr 1, 2025

traviscross commented Apr 1, 2025

RalfJung commented Apr 2, 2025 •

edited

Loading

traviscross commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

Alexendoo commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

Alexendoo commented Apr 8, 2025

	let f = move \|x: W\| { { x }.clone() };
	let f = move \|x: W\| -> _ { { x }.clone() };

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

Lint precedence possible ambiguity between closure and method call #14421

Are you sure you want to change the base?

Lint precedence possible ambiguity between closure and method call #14421

Conversation

samueltardieu commented Mar 16, 2025 • edited Loading

rustbot commented Mar 16, 2025

traviscross Mar 17, 2025

Choose a reason for hiding this comment

Alexendoo Mar 17, 2025

Choose a reason for hiding this comment

traviscross Apr 1, 2025

Choose a reason for hiding this comment

traviscross Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung Mar 17, 2025

Choose a reason for hiding this comment

traviscross Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

traviscross Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

traviscross Mar 17, 2025

Choose a reason for hiding this comment

Alexendoo Mar 17, 2025

Choose a reason for hiding this comment

samueltardieu commented Apr 1, 2025

traviscross commented Apr 1, 2025

RalfJung commented Apr 2, 2025 • edited Loading

traviscross commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

Alexendoo commented Apr 2, 2025

samueltardieu commented Apr 2, 2025

Alexendoo commented Apr 8, 2025

samueltardieu commented Mar 16, 2025 •

edited

Loading

traviscross Mar 17, 2025 •

edited

Loading

traviscross Mar 17, 2025 •

edited

Loading

traviscross Mar 17, 2025 •

edited

Loading

RalfJung Mar 17, 2025 •

edited

Loading

RalfJung commented Apr 2, 2025 •

edited

Loading