Fix really long single-dot methods being multilined

fables-tales · Aug 16, 2023 · 4cb900c · 4cb900c
1 parent a2fa7d4
commit 4cb900c
Show file tree

Hide file tree

Showing 10 changed files with 62 additions and 38 deletions.
diff --git a/fixtures/large/rspec_mocks_proxy_expected.rb b/fixtures/large/rspec_mocks_proxy_expected.rb
@@ -393,8 +393,9 @@ def initialize(source_space, *args)
       # That's what this method (together with `original_unbound_method_handle_from_ancestor_for`)
       # does.
       def original_method_handle_for(message)
-        unbound_method = superclass_proxy && superclass_proxy
-          .original_unbound_method_handle_from_ancestor_for(message.to_sym)
+        unbound_method = superclass_proxy && superclass_proxy.original_unbound_method_handle_from_ancestor_for(
+          message.to_sym
+        )
 
         return super unless unbound_method
         unbound_method.bind(object)

diff --git a/fixtures/small/block_local_variables_expected.rb b/fixtures/small/block_local_variables_expected.rb
@@ -4,7 +4,8 @@
   puts(y)
   y = 19
   puts(y)
-}.call(17)
+}
+  .call(17)
 puts(y)
 
 a do |;x|

diff --git a/fixtures/small/breakables_over_line_length_expected.rb b/fixtures/small/breakables_over_line_length_expected.rb
@@ -18,7 +18,9 @@
   ReallyLongThing
 ]
 
-if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample
-    .calls_a_thing(a, b)
+if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample.calls_a_thing(
+    a,
+    b
+  )
   puts("")
 end
diff --git a/fixtures/small/method_chains_expected.rb b/fixtures/small/method_chains_expected.rb
@@ -25,7 +25,8 @@
 # the first receiver, consider that "on one line"
 foo.bar.baz
 
-foo::bar&.nil?
+foo::bar
+  &.nil?
 
 foo::bar
   &.nil?::klass

diff --git a/fixtures/small/more_methods_expected.rb b/fixtures/small/more_methods_expected.rb
@@ -8,7 +8,8 @@
 a.b(1).c
 
 a do
-end.after_block
+end
+  .after_block
 
 a.<=(3)
 a&.<=(3)
diff --git a/fixtures/small/single_line_method_call_chain_actual.rb b/fixtures/small/single_line_method_call_chain_actual.rb
@@ -1,3 +1,7 @@
 def blorp
   method_call(arg_a, arg_b).chained_call
 end
+
+This::Is::Some::SuperDuperLongConstantThatOnlyHasOneCallInTheChain.build(
+  reason: Because::ThisCouldCauseUsSome::SeriousProblemsIfWeTriedToMultilineIt::EventThoughItActuallyDoesExtendBeyondTheLimit
+)
diff --git a/fixtures/small/single_line_method_call_chain_expected.rb b/fixtures/small/single_line_method_call_chain_expected.rb
@@ -1,3 +1,7 @@
 def blorp
   method_call(arg_a, arg_b).chained_call
 end
+
+This::Is::Some::SuperDuperLongConstantThatOnlyHasOneCallInTheChain.build(
+  reason: Because::ThisCouldCauseUsSome::SeriousProblemsIfWeTriedToMultilineIt::EventThoughItActuallyDoesExtendBeyondTheLimit
+)
diff --git a/librubyfmt/src/parser_state.rs b/librubyfmt/src/parser_state.rs
@@ -401,8 +401,9 @@ impl ConcreteParserState for BaseParserState {
         f: RenderFunc,
     ) {
         self.shift_comments();
-        let be =
+        let mut be =
             BreakableCallChainEntry::new(self.current_formatting_context(), call_chain_elements);
+        be.push_line_number(self.current_orig_line_number);
         self.breakable_entry_stack.push(Box::new(be));
 
         f(self);

diff --git a/librubyfmt/src/render_queue_writer.rs b/librubyfmt/src/render_queue_writer.rs
@@ -201,7 +201,8 @@ impl RenderQueueWriter {
     ) {
         let single_line_string_length = bcce.longest_multiline_string_length();
         let length = accum.current_line_length() + single_line_string_length;
-        if (length > MAX_LINE_LENGTH || bcce.is_multiline())
+        let must_be_single_line = bcce.should_force_single_line();
+        if (!must_be_single_line && (length > MAX_LINE_LENGTH || bcce.is_multiline()))
             && bcce.entry_formatting_context() != FormattingContext::StringEmbexpr
         {
             let tokens = bcce.into_tokens(ConvertType::MultiLine);

diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs
@@ -352,44 +352,24 @@ impl AbstractTokenTarget for BreakableCallChainEntry {
             })
             .collect::<Vec<StartEnd>>();
 
-        // don't multiline if there's only one call in the chain
-        if all_op_locations.len() < 2 {
-            return false;
-        }
-
         // Multiline the chain if all the operators (dots, double colons, etc.) are not on the same line
         if let Some(first_op_start_end) = all_op_locations.first() {
             let chain_is_user_multilined = !all_op_locations
                 .iter()
                 .all(|op_start_end| op_start_end == first_op_start_end);
             if chain_is_user_multilined {
                 return true;
-            }
-        }
-
-        // Ignore chains that are basically only method calls, e.g.
-        // ````ruby
-        // Thing.foo(args)
-        // Thing.foo(args) { block! }
-        // ```
-        // These should always stay inline
-        match call_chain_to_check.as_slice() {
-            [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..)]
-            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..)]
-            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::Block(..)]
-            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..), CallChainElement::Block(..)] =>
+            } else if self.line_numbers.len() == 2
+                && all_op_locations.len() == 1
+                && *self.line_numbers.iter().max().unwrap() == first_op_start_end.1
             {
-                return false;
-            }
-            [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..)]
-            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..)]
-            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::Block(..)]
-            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..), CallChainElement::Block(..)] => {
-                if matches!(maybe_const_ref.as_ref(), Expression::ConstPathRef(..)) {
-                    return false;
-                }
+                // This is a mega hack to support constructs like
+                // ```ruby
+                // params(foo: String)
+                //   .returns(Bar)
+                // ```
+                return true;
             }
-            _ => {}
         }
 
         // If the first item in the chain is a multiline expression (like a hash or array),
@@ -493,6 +473,34 @@ impl BreakableCallChainEntry {
             .unwrap()
     }
 
+    pub fn should_force_single_line(&self) -> bool {
+        // Ignore chains that are basically only method calls, e.g.
+        // ````ruby
+        // Thing.foo(args)
+        // Thing.foo(args) { block! }
+        // ```
+        // These should always stay inline
+        match self.call_chain.as_slice() {
+            [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..)]
+            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..)]
+            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::Block(..)]
+            | [CallChainElement::VarRef(..) | CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..), CallChainElement::Block(..)] =>
+            {
+                return true;
+            }
+            [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..)]
+            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..)]
+            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::Block(..)]
+            | [CallChainElement::Expression(maybe_const_ref), CallChainElement::DotTypeOrOp(..), CallChainElement::IdentOrOpOrKeywordOrConst(..), CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(..), CallChainElement::Block(..)] => {
+                if matches!(maybe_const_ref.as_ref(), Expression::ConstPathRef(..)) {
+                    return true;
+                }
+            }
+            _ => {}
+        }
+        false
+    }
+
     /// In practice, this generally means something like the call chain having something
     /// like a method call with args or a block, e.g.
     ///