fix: Add parenthesis around `..` closure if it's a method call receiver #5842

ytmimi · 2023-07-18T20:57:07Z

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?

It should be correct iirc. This is just one of the weird precedence things

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.

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)

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(); }

-Original file line number
+Diff line change
@@ Expand Up / @@ -253,11 +253,7 @@ impl ChainItemKind { @@
                     return (
                         ChainItemKind::Parent {
                             expr: expr.clone(),
-                            parens: is_method_call_receiver
-                                && matches!(
-                                    &expr.kind,
-                                    ast::ExprKind::Lit(lit) if crate::expr::lit_ends_in_dot(lit)
-                                ),
+                            parens: is_method_call_receiver && should_add_parens(expr),
                         },
                         expr.span,
                     );
@@ Expand Down Expand Up / @@ -982,3 +978,22 @@ fn trim_tries(s: &str) -> String { @@
         }
         result
     }
+    /// Whether a method call's receiver needs parenthesis, like
+    /// ```rust,ignore
+    /// || .. .method();
+    /// || 1.. .method();
+    /// 1. .method();
+    /// ```
+    /// Which all need parenthesis or a space before `.method()`.
+    fn should_add_parens(expr: &ast::Expr) -> bool {
+        match expr.kind {
+            ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit),
+            ast::ExprKind::Closure(ref cl) => match cl.body.kind {
+                ast::ExprKind::Range(_, _, ast::RangeLimits::HalfOpen) => true,
+                ast::ExprKind::Lit(ref lit) => crate::expr::lit_ends_in_dot(lit),
+                _ => false,
+            },
+            _ => false,
+        }
+    }

-Original file line number
+Diff line change
@@ -0,0 +1,13 @@
+    trait Trait {
+        fn method(&self) {}
+    }
+    impl<F: Fn() -> T, T> Trait for F {}
+    impl Trait for f32 {}
+    fn main() {
+        || (10.).method();
+        (|| ..).method();
+        (|| 1..).method();
+    }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Add parenthesis around `..` closure if it's a method call receiver #5842

Uh oh!

Diff view

Diff view

There are no files selected for viewing

ytmimi Jul 18, 2023

Uh oh!

fee1-dead Jul 19, 2023

Uh oh!

ytmimi Jul 23, 2023

Uh oh!

Centri3 Jul 23, 2023 •

edited

Loading

Uh oh!

ytmimi Aug 1, 2023

Uh oh!

fix: Add parenthesis around .. closure if it's a method call receiver #5842

Uh oh!

fix: Add parenthesis around .. closure if it's a method call receiver #5842

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

ytmimi Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

fee1-dead Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

ytmimi Jul 23, 2023

Choose a reason for hiding this comment

Uh oh!

Centri3 Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

fix: Add parenthesis around `..` closure if it's a method call receiver #5842

fix: Add parenthesis around `..` closure if it's a method call receiver #5842

Centri3 Jul 23, 2023 •

edited

Loading