fix: Add parenthesis around .. closure if it's a method call receiver
#5842
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fee1-dead
reviewed
Jul 18, 2023
|
Also, I think a similar case should also be fixed: trait Trait {
fn method(&self) {}
}
impl Trait for f32 {}
pub fn x() {
|| 10. .method();
}and where the expression is a range that has a start: trait Trait {
fn method(&self);
}
impl<F: Fn() -> T, T> Trait for F {
fn method(&self) {}
}
pub fn x() {
|| 1.. .method();
}therefore this shouldn't just match on |
Centri3
force-pushed
the
4808
branch
2 times, most recently
from
7149aa8
to
4bd2dc2
Compare
ytmimi
reviewed
Jul 18, 2023
Comment on lines
+10
to
+12
| || (10.).method(); | ||
| (|| ..).method(); | ||
| (|| 1..).method(); |
There was a problem hiding this comment.
Are the parens being applied in the correct place in the first example? Shouldn't it be (|| 10.).method(); if we're applying the method to the closure?
There was a problem hiding this comment.
It should be correct iirc. This is just one of the weird precedence things
There was a problem hiding this comment.
I just checked and here are the ASTs that rustfmt sees when parsing
fn main() {
|| (10.).method();
}AST wrapping float in parens
Module {
ast_mod_kind: None,
items: [
Item {
attrs: [],
id: NodeId(4294967040),
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
36,
),
ctxt: #0,
},
vis: Visibility {
kind: Inherited,
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
0,
),
ctxt: #0,
},
tokens: None,
},
ident: main#0,
kind: Fn(
Fn {
defaultness: Final,
generics: Generics {
params: [],
where_clause: WhereClause {
has_where_token: false,
predicates: [],
span: Span {
lo: BytePos(
9,
),
hi: BytePos(
9,
),
ctxt: #0,
},
},
span: Span {
lo: BytePos(
7,
),
hi: BytePos(
7,
),
ctxt: #0,
},
},
sig: FnSig {
header: FnHeader {
unsafety: No,
asyncness: No,
constness: No,
ext: None,
},
decl: FnDecl {
inputs: [],
output: Default(
Span {
lo: BytePos(
10,
),
hi: BytePos(
10,
),
ctxt: #0,
},
),
},
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
9,
),
ctxt: #0,
},
},
body: Some(
Block {
stmts: [
Stmt {
id: NodeId(4294967040),
kind: Semi(
Expr {
id: NodeId(4294967040),
kind: Closure(
Closure {
binder: NotPresent,
capture_clause: Ref,
constness: No,
asyncness: No,
movability: Movable,
fn_decl: FnDecl {
inputs: [],
output: Default(
Span {
lo: BytePos(
19,
),
hi: BytePos(
19,
),
ctxt: #0,
},
),
},
body: Expr {
id: NodeId(4294967040),
kind: MethodCall(
MethodCall {
seg: PathSegment {
ident: method#0,
id: NodeId(4294967040),
args: None,
},
receiver: Expr {
id: NodeId(4294967040),
kind: Paren(
Expr {
id: NodeId(4294967040),
kind: Lit(
Lit {
kind: Float,
symbol: "10.",
suffix: None,
},
),
span: Span {
lo: BytePos(
20,
),
hi: BytePos(
23,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
),
span: Span {
lo: BytePos(
19,
),
hi: BytePos(
24,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
args: [],
span: Span {
lo: BytePos(
25,
),
hi: BytePos(
33,
),
ctxt: #0,
},
},
),
span: Span {
lo: BytePos(
19,
),
hi: BytePos(
33,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
fn_decl_span: Span {
lo: BytePos(
16,
),
hi: BytePos(
18,
),
ctxt: #0,
},
fn_arg_span: Span {
lo: BytePos(
16,
),
hi: BytePos(
18,
),
ctxt: #0,
},
},
),
span: Span {
lo: BytePos(
16,
),
hi: BytePos(
33,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
),
span: Span {
lo: BytePos(
16,
),
hi: BytePos(
34,
),
ctxt: #0,
},
},
],
id: NodeId(4294967040),
rules: Default,
span: Span {
lo: BytePos(
10,
),
hi: BytePos(
36,
),
ctxt: #0,
},
tokens: None,
could_be_bare_literal: false,
},
),
},
),
tokens: None,
},
],
inner_attr: [],
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
37,
),
ctxt: #0,
},
}
fn main() {
(|| 10.).method();
}AST wrapping closure in parens
Module {
ast_mod_kind: None,
items: [
Item {
attrs: [],
id: NodeId(4294967040),
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
36,
),
ctxt: #0,
},
vis: Visibility {
kind: Inherited,
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
0,
),
ctxt: #0,
},
tokens: None,
},
ident: main#0,
kind: Fn(
Fn {
defaultness: Final,
generics: Generics {
params: [],
where_clause: WhereClause {
has_where_token: false,
predicates: [],
span: Span {
lo: BytePos(
9,
),
hi: BytePos(
9,
),
ctxt: #0,
},
},
span: Span {
lo: BytePos(
7,
),
hi: BytePos(
7,
),
ctxt: #0,
},
},
sig: FnSig {
header: FnHeader {
unsafety: No,
asyncness: No,
constness: No,
ext: None,
},
decl: FnDecl {
inputs: [],
output: Default(
Span {
lo: BytePos(
10,
),
hi: BytePos(
10,
),
ctxt: #0,
},
),
},
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
9,
),
ctxt: #0,
},
},
body: Some(
Block {
stmts: [
Stmt {
id: NodeId(4294967040),
kind: Semi(
Expr {
id: NodeId(4294967040),
kind: MethodCall(
MethodCall {
seg: PathSegment {
ident: method#0,
id: NodeId(4294967040),
args: None,
},
receiver: Expr {
id: NodeId(4294967040),
kind: Paren(
Expr {
id: NodeId(4294967040),
kind: Closure(
Closure {
binder: NotPresent,
capture_clause: Ref,
constness: No,
asyncness: No,
movability: Movable,
fn_decl: FnDecl {
inputs: [],
output: Default(
Span {
lo: BytePos(
20,
),
hi: BytePos(
20,
),
ctxt: #0,
},
),
},
body: Expr {
id: NodeId(4294967040),
kind: Lit(
Lit {
kind: Float,
symbol: "10.",
suffix: None,
},
),
span: Span {
lo: BytePos(
20,
),
hi: BytePos(
23,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
fn_decl_span: Span {
lo: BytePos(
17,
),
hi: BytePos(
19,
),
ctxt: #0,
},
fn_arg_span: Span {
lo: BytePos(
17,
),
hi: BytePos(
19,
),
ctxt: #0,
},
},
),
span: Span {
lo: BytePos(
17,
),
hi: BytePos(
23,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
),
span: Span {
lo: BytePos(
16,
),
hi: BytePos(
24,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
args: [],
span: Span {
lo: BytePos(
25,
),
hi: BytePos(
33,
),
ctxt: #0,
},
},
),
span: Span {
lo: BytePos(
16,
),
hi: BytePos(
33,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
),
span: Span {
lo: BytePos(
16,
),
hi: BytePos(
34,
),
ctxt: #0,
},
},
],
id: NodeId(4294967040),
rules: Default,
span: Span {
lo: BytePos(
10,
),
hi: BytePos(
36,
),
ctxt: #0,
},
tokens: None,
could_be_bare_literal: false,
},
),
},
),
tokens: None,
},
],
inner_attr: [],
span: Span {
lo: BytePos(
0,
),
hi: BytePos(
37,
),
ctxt: #0,
},
}
And here's the diff when comparing wrapping the float vs wrapping the closure. For reference I saved both ASTs to files and ran git --no-pager diff --no-index wrap_float_with_parens_ast.txt wrap_closure_with_parens_ast.txt
From the AST level it seems that the first is treated as a closure that contains a method call, while the second is a method call on a tuple.
AST Diff
Module {
kind: Semi(
Expr {
id: NodeId(4294967040),
- kind: Closure(
- Closure {
- binder: NotPresent,
- capture_clause: Ref,
- constness: No,
- asyncness: No,
- movability: Movable,
- fn_decl: FnDecl {
- inputs: [],
- output: Default(
- Span {
- lo: BytePos(
- 19,
- ),
- hi: BytePos(
- 19,
- ),
- ctxt: #0,
- },
- ),
+ kind: MethodCall(
+ MethodCall {
+ seg: PathSegment {
+ ident: method#0,
+ id: NodeId(4294967040),
+ args: None,
},
- body: Expr {
+ receiver: Expr {
id: NodeId(4294967040),
- kind: MethodCall(
- MethodCall {
- seg: PathSegment {
- ident: method#0,
- id: NodeId(4294967040),
- args: None,
- },
- receiver: Expr {
- id: NodeId(4294967040),
- kind: Paren(
- Expr {
+ kind: Paren(
+ Expr {
+ id: NodeId(4294967040),
+ kind: Closure(
+ Closure {
+ binder: NotPresent,
+ capture_clause: Ref,
+ constness: No,
+ asyncness: No,
+ movability: Movable,
+ fn_decl: FnDecl {
+ inputs: [],
+ output: Default(
+ Span {
+ lo: BytePos(
+ 20,
+ ),
+ hi: BytePos(
+ 20,
+ ),
+ ctxt: #0,
+ },
+ ),
+ },
+ body: Expr {
id: NodeId(4294967040),
kind: Lit(
Lit {
@@ -148,58 +148,58 @@ Module {
attrs: [],
tokens: None,
},
- ),
- span: Span {
- lo: BytePos(
- 19,
- ),
- hi: BytePos(
- 24,
- ),
- ctxt: #0,
+ fn_decl_span: Span {
+ lo: BytePos(
+ 17,
+ ),
+ hi: BytePos(
+ 19,
+ ),
+ ctxt: #0,
+ },
+ fn_arg_span: Span {
+ lo: BytePos(
+ 17,
+ ),
+ hi: BytePos(
+ 19,
+ ),
+ ctxt: #0,
+ },
},
- attrs: [],
- tokens: None,
- },
- args: [],
+ ),
span: Span {
lo: BytePos(
- 25,
+ 17,
),
hi: BytePos(
- 33,
+ 23,
),
ctxt: #0,
},
+ attrs: [],
+ tokens: None,
},
),
span: Span {
lo: BytePos(
- 19,
+ 16,
),
hi: BytePos(
- 33,
+ 24,
),
ctxt: #0,
},
attrs: [],
tokens: None,
},
- fn_decl_span: Span {
- lo: BytePos(
- 16,
- ),
- hi: BytePos(
- 18,
- ),
- ctxt: #0,
- },
- fn_arg_span: Span {
+ args: [],
+ span: Span {
lo: BytePos(
- 16,
+ 25,
),
hi: BytePos(
- 18,
+ 33,
),
ctxt: #0,
},
If there are compilation steps that happen after parsing that ensures that || (10.).method(); means "apply the method call to the return value of the closure then I think we're good to go, otherwise I'd like to change the output to produce (|| 10.).method(); for consistency, and because I think that (|| 10.).method(); more clearly conveys that we're applying the method to the returned value of the closure.
There was a problem hiding this comment.
I believe we aren't applying it to the closure in specific, but the float, yet for some reason the previous lit_ends_in_dot check didn't apply here.
Changing it to (|| 10.).method() applies it to the closure as you noticed, which does cause differing behavior (and can cause compile errors if the trait isn't implemented for closures, which isn't good)
There was a problem hiding this comment.
Yeah, I just double checked and it turns out that the code highlighted in the first example of #5842 (comment) (inlined below for reference)
trait Trait {
fn method(&self) {}
}
impl Trait for f32 {}
pub fn x() {
|| 10. .method();
}parses as a closure and the the method is applied to the 10.
while the second example from that comment (inlined below for reference) parses as method call where the receiver is a closure.
trait Trait {
fn method(&self);
}
impl<F: Fn() -> T, T> Trait for F {
fn method(&self) {}
}
pub fn x() {
|| 1.. .method();
}
fee1-dead
approved these changes
Jul 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4808
I think this is (perhaps?) okay without a version gate as the formatted code would be invalid before now, might be wrong though ^^