From ed684f3b364b8b375d90c87c20f28ff3c6e7511f Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Sat, 3 Dec 2022 11:41:10 -0800 Subject: [PATCH 1/3] Fixtures --- fixtures/large/rspec_mocks_proxy_expected.rb | 5 ++++- ...omments_at_indentation_changes_expected.rb | 7 ++++-- fixtures/small/method_annotation_expected.rb | 4 +++- fixtures/small/method_chains_actual.rb | 22 +++++++++++++++++++ fixtures/small/method_chains_expected.rb | 22 +++++++++++++++++++ 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/fixtures/large/rspec_mocks_proxy_expected.rb b/fixtures/large/rspec_mocks_proxy_expected.rb index baee2ce2..b00e61ba 100644 --- a/fixtures/large/rspec_mocks_proxy_expected.rb +++ b/fixtures/large/rspec_mocks_proxy_expected.rb @@ -305,7 +305,10 @@ def reset class PartialDoubleProxy < Proxy def original_method_handle_for(message) if any_instance_class_recorder_observing_method?(@object.class, message) - message = ::RSpec::Mocks.space.any_instance_recorder_for(@object.class).build_alias_method_name(message) + message = ::RSpec::Mocks + .space + .any_instance_recorder_for(@object.class) + .build_alias_method_name(message) end ::RSpec::Support.method_handle_for(@object, message) diff --git a/fixtures/small/comments_at_indentation_changes_expected.rb b/fixtures/small/comments_at_indentation_changes_expected.rb index a448e4e2..1d0d01e8 100644 --- a/fixtures/small/comments_at_indentation_changes_expected.rb +++ b/fixtures/small/comments_at_indentation_changes_expected.rb @@ -1,6 +1,9 @@ it "" do - # Should only download the patches twice - Test::Mock.expects(Util, :download_file).twice.returns(true) + Test::Mock + .expects(Util, :download_file) + # Should only download the patches twice + .twice + .returns(true) end # Make some fake thing diff --git a/fixtures/small/method_annotation_expected.rb b/fixtures/small/method_annotation_expected.rb index 45db3d92..d521fc8a 100644 --- a/fixtures/small/method_annotation_expected.rb +++ b/fixtures/small/method_annotation_expected.rb @@ -3,7 +3,9 @@ def empty_example end sig do - params(a: T::Array[String], b: T::Hash[Symbol, String]).returns(T::Set[Symbol]).checked(:tests) + params(a: T::Array[String], b: T::Hash[Symbol, String]) + .returns(T::Set[Symbol]) + .checked(:tests) end def do_the_thing(a, b) puts(a) diff --git a/fixtures/small/method_chains_actual.rb b/fixtures/small/method_chains_actual.rb index aefe2e5c..ece2d64e 100644 --- a/fixtures/small/method_chains_actual.rb +++ b/fixtures/small/method_chains_actual.rb @@ -13,6 +13,28 @@ b: "" ) +foo.bar.baz + +foo.bar +.baz + +# If they're all on the same line but different from +# the first receiver, consider that "on one line" +foo +.bar.baz + +foo::bar +&.nil? + +foo::bar +&.nil?::klass +.true? + +Class +&.new +.call! + + def example things .map do |thing| diff --git a/fixtures/small/method_chains_expected.rb b/fixtures/small/method_chains_expected.rb index 9fa33ebc..dde20abf 100644 --- a/fixtures/small/method_chains_expected.rb +++ b/fixtures/small/method_chains_expected.rb @@ -15,6 +15,28 @@ b: "" ) +foo.bar.baz + +foo + .bar + .baz + +# If they're all on the same line but different from +# the first receiver, consider that "on one line" +foo.bar.baz + +foo::bar&.nil? + +foo + ::bar + &.nil? + ::klass + .true? + +Class + &.new + .call! + def example things .map do |thing| From 342043ef157b62c87ec069f438502fab3c57a729 Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Sat, 3 Dec 2022 11:43:16 -0800 Subject: [PATCH 2/3] Multiline call chains if user multilined it --- librubyfmt/rubyfmt_lib.rb | 4 +++ librubyfmt/src/format.rs | 45 ++++++++++++++++++++++++++--- librubyfmt/src/ripper_tree_types.rs | 8 ++--- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/librubyfmt/rubyfmt_lib.rb b/librubyfmt/rubyfmt_lib.rb index 682379e8..13f7dbee 100644 --- a/librubyfmt/rubyfmt_lib.rb +++ b/librubyfmt/rubyfmt_lib.rb @@ -169,6 +169,10 @@ def escape_percent_array_paren_content(part, pattern) end end + def on_op(*_args) + super + [[lineno, lineno]] + end + def on_next(*_args) super + [start_end_for_keyword('next')] end diff --git a/librubyfmt/src/format.rs b/librubyfmt/src/format.rs index 31a4a24e..1f97727c 100644 --- a/librubyfmt/src/format.rs +++ b/librubyfmt/src/format.rs @@ -2789,7 +2789,11 @@ fn format_call_chain_elements( ps.start_indent(); has_indented = true; } - if render_multiline_chain { + if render_multiline_chain + // Separating `::` calls with a newline + // isn't valid syntax + && !matches!(d, DotTypeOrOp::ColonColon(_)) + { ps.emit_newline(); ps.emit_indent(); } @@ -2844,9 +2848,10 @@ fn is_heredoc_call_chain_with_breakables(cc_elements: &[CallChainElement]) -> bo /// /// ## High-level rules /// -/// The two main rules that govern whether or not to multiline a method chain is to split across multiple lines if -/// (1) the whole chain exceeds the maximum line length or -/// (2) the chain contains blocks that are split across multiple lines +/// The three main rules that govern whether or not to multiline a method chain is to split across multiple lines if +/// (1) the user multilined the chain, +/// (2) the whole chain exceeds the maximum line length, or +/// (3) the chain contains blocks that are split across multiple lines /// /// That said, both of these have some *very large* asterisks, since there are a lot of contexts in which these /// have special cases for various reasons (see below). @@ -2884,6 +2889,38 @@ fn should_multiline_call_chain(ps: &mut dyn ConcreteParserState, method_call: &M CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(args, start_end), ]); + let all_op_locations = call_chain_to_check + .iter() + .filter_map(|cc_elem| match cc_elem { + CallChainElement::DotTypeOrOp(dot_type_or_op) => { + match dot_type_or_op { + // ColonColon is specially represented in the parser, and + // it can't be properly multilined anyways, so we ignore it here + DotTypeOrOp::ColonColon(..) => None, + DotTypeOrOp::StringDot(..) => None, + DotTypeOrOp::Op(Op(.., start_end)) + | DotTypeOrOp::DotType( + DotType::LonelyOperator(LonelyOperator(_, start_end)) + | DotType::Dot(Dot(_, start_end)), + ) => Some(start_end.clone()), + DotTypeOrOp::Period(Period(.., linecol)) => { + Some(StartEnd(linecol.0, linecol.0)) + } + } + } + _ => None, + }) + .collect::>(); + // 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) diff --git a/librubyfmt/src/ripper_tree_types.rs b/librubyfmt/src/ripper_tree_types.rs index 8cb70ae7..af39b81c 100644 --- a/librubyfmt/src/ripper_tree_types.rs +++ b/librubyfmt/src/ripper_tree_types.rs @@ -994,7 +994,7 @@ pub struct BlockArg(pub blockarg_tag, pub Ident); #[derive(Deserialize, Debug, Clone)] pub struct LineCol(pub LineNumber, pub u64); -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Debug, Clone, Eq, PartialEq)] pub struct StartEnd(pub LineNumber, pub LineNumber); impl StartEnd { @@ -1527,7 +1527,7 @@ pub struct Equals(pub equals_tag); def_tag!(dot_tag, "."); #[derive(Deserialize, Debug, Clone)] -pub struct Dot(pub dot_tag); +pub struct Dot(pub dot_tag, pub StartEnd); def_tag!(colon_colon_tag, "::"); #[derive(Deserialize, Debug, Clone)] @@ -1535,11 +1535,11 @@ pub struct ColonColon(pub colon_colon_tag); def_tag!(lonely_operator_tag, "&."); #[derive(Deserialize, Debug, Clone)] -pub struct LonelyOperator(pub lonely_operator_tag); +pub struct LonelyOperator(pub lonely_operator_tag, pub StartEnd); def_tag!(op_tag, "@op"); #[derive(Deserialize, Debug, Clone)] -pub struct Op(pub op_tag, pub Operator, pub LineCol); +pub struct Op(pub op_tag, pub Operator, pub LineCol, pub StartEnd); #[derive(RipperDeserialize, Debug, Clone)] pub enum Operator { From 82cc86cb0731cee76d3a55fed71bcac832c2bea7 Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Sat, 3 Dec 2022 11:51:47 -0800 Subject: [PATCH 3/3] Don't split :: calls across multiple lines --- fixtures/small/method_chains_expected.rb | 6 ++---- librubyfmt/src/format.rs | 7 ++++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fixtures/small/method_chains_expected.rb b/fixtures/small/method_chains_expected.rb index dde20abf..99ed221d 100644 --- a/fixtures/small/method_chains_expected.rb +++ b/fixtures/small/method_chains_expected.rb @@ -27,10 +27,8 @@ foo::bar&.nil? -foo - ::bar - &.nil? - ::klass +foo::bar + &.nil?::klass .true? Class diff --git a/librubyfmt/src/format.rs b/librubyfmt/src/format.rs index 1f97727c..e69c932a 100644 --- a/librubyfmt/src/format.rs +++ b/librubyfmt/src/format.rs @@ -2789,10 +2789,15 @@ fn format_call_chain_elements( ps.start_indent(); has_indented = true; } + let is_double_colon = match &d { + DotTypeOrOp::ColonColon(_) => true, + DotTypeOrOp::StringDot(val) => val == "::", + _ => false, + }; if render_multiline_chain // Separating `::` calls with a newline // isn't valid syntax - && !matches!(d, DotTypeOrOp::ColonColon(_)) + && !is_double_colon { ps.emit_newline(); ps.emit_indent();