From e1e2692b356f67b7151edb0258d0408f958c4a2b Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Mon, 8 Jan 2024 00:24:17 +0000 Subject: [PATCH] Fix multiline handling --- fixtures/small/multiline_aryptn_actual.rb | 8 ++ fixtures/small/multiline_aryptn_expected.rb | 10 +++ fixtures/small/multiline_fndptn_actual.rb | 7 ++ fixtures/small/multiline_fndptn_expected.rb | 10 +++ librubyfmt/rubyfmt_lib.rb | 24 +++++- librubyfmt/src/format.rs | 87 +++++++++++---------- librubyfmt/src/ripper_tree_types.rs | 35 ++++++--- 7 files changed, 127 insertions(+), 54 deletions(-) create mode 100644 fixtures/small/multiline_aryptn_actual.rb create mode 100644 fixtures/small/multiline_aryptn_expected.rb create mode 100644 fixtures/small/multiline_fndptn_actual.rb create mode 100644 fixtures/small/multiline_fndptn_expected.rb diff --git a/fixtures/small/multiline_aryptn_actual.rb b/fixtures/small/multiline_aryptn_actual.rb new file mode 100644 index 00000000..fe288344 --- /dev/null +++ b/fixtures/small/multiline_aryptn_actual.rb @@ -0,0 +1,8 @@ +case [] +in [first, + *, + last] + puts "bees!!!" +else + "no bees" +end \ No newline at end of file diff --git a/fixtures/small/multiline_aryptn_expected.rb b/fixtures/small/multiline_aryptn_expected.rb new file mode 100644 index 00000000..0ac3e42e --- /dev/null +++ b/fixtures/small/multiline_aryptn_expected.rb @@ -0,0 +1,10 @@ +case [] +in [ + first, + *, + last + ] + puts("bees!!!") +else + "no bees" +end diff --git a/fixtures/small/multiline_fndptn_actual.rb b/fixtures/small/multiline_fndptn_actual.rb new file mode 100644 index 00000000..85830a39 --- /dev/null +++ b/fixtures/small/multiline_fndptn_actual.rb @@ -0,0 +1,7 @@ +case [] +in *, "bees are present", + *; then + puts "bees!!!" +else + "no bees" +end \ No newline at end of file diff --git a/fixtures/small/multiline_fndptn_expected.rb b/fixtures/small/multiline_fndptn_expected.rb new file mode 100644 index 00000000..32e71340 --- /dev/null +++ b/fixtures/small/multiline_fndptn_expected.rb @@ -0,0 +1,10 @@ +case [] +in [ + *, + "bees are present", + * + ] + puts("bees!!!") +else + "no bees" +end diff --git a/librubyfmt/rubyfmt_lib.rb b/librubyfmt/rubyfmt_lib.rb index 3d385c84..a8a82449 100644 --- a/librubyfmt/rubyfmt_lib.rb +++ b/librubyfmt/rubyfmt_lib.rb @@ -391,8 +391,28 @@ def on_break(arg) [:break, arg, start_end_for_keyword('break')] end - def on_var_field(*args) - with_lineno { super } + def on_var_field(ident) + line = if ident + # ident.last = [line, col] + ident.last.first + else + # When ident is nil, this represents a "*" pattern, which + # is lexed as an op. *However*, this can also happen during + # an "implicit" splat, e.g. `in String,` (with a trailing comma), + # in which case there's not a last op_location either, so at that + # point we give up because `lineno` would be incorrect + # + # Important to note: in this case, we must `shift` and not `pop`, + # because in the case of fndptn, this isn't run until after both + # splats have been lexed, so their op locations will be in the reverse order + @op_locations.shift + end + start_end = if line + [[line, line]] + else + [nil] + end + super + start_end end def on_tlambda(*args) diff --git a/librubyfmt/src/format.rs b/librubyfmt/src/format.rs index bc7a5815..f255c9d4 100644 --- a/librubyfmt/src/format.rs +++ b/librubyfmt/src/format.rs @@ -3413,32 +3413,34 @@ fn format_aryptn(ps: &mut dyn ConcreteParserState, aryptn: Aryptn) { if let Some(collection_name) = maybe_collection_name { format_var_ref(ps, collection_name.clone()); } - ps.breakable_of( - BreakableDelims::for_array(), - Box::new(|ps| { - let mut vals = Vec::new(); - if let Some(pre_star_list) = maybe_pre_star_list { - vals.append( - &mut pre_star_list - .into_iter() - .map(|item| item.into_expression()) - .collect::>(), - ); - } - if let Some(star) = maybe_star { - vals.push(pattern_splat_as_expr(star.clone())); - } - if let Some(post_star_list) = maybe_post_star_list { - vals.append( - &mut post_star_list - .into_iter() - .map(|item| item.into_expression()) - .collect::>(), - ); - } - format_list_like_thing_items(ps, vals, None, false); - }), - ); + ps.new_block(Box::new(|ps| { + ps.breakable_of( + BreakableDelims::for_array(), + Box::new(|ps| { + let mut vals = Vec::new(); + if let Some(pre_star_list) = maybe_pre_star_list { + vals.append( + &mut pre_star_list + .into_iter() + .map(|item| item.into_expression()) + .collect::>(), + ); + } + if let Some(star) = maybe_star { + vals.push(pattern_splat_as_expr(star.clone())); + } + if let Some(post_star_list) = maybe_post_star_list { + vals.append( + &mut post_star_list + .into_iter() + .map(|item| item.into_expression()) + .collect::>(), + ); + } + format_list_like_thing_items(ps, vals, None, false); + }), + ); + })); } fn format_fndptn(ps: &mut dyn ConcreteParserState, fndptn: Fndptn) { @@ -3446,19 +3448,21 @@ fn format_fndptn(ps: &mut dyn ConcreteParserState, fndptn: Fndptn) { if let Some(collection_name) = maybe_collection_name { format_var_ref(ps, collection_name); } - ps.breakable_of( - BreakableDelims::for_array(), - Box::new(|ps| { - let mut vals = values - .into_iter() - .map(|item| item.into_expression()) - .collect::>(); - vals.insert(0, pattern_splat_as_expr(pre_splat)); - vals.push(pattern_splat_as_expr(post_splat)); - - format_list_like_thing_items(ps, vals, None, false); - }), - ); + ps.new_block(Box::new(|ps| { + ps.breakable_of( + BreakableDelims::for_array(), + Box::new(|ps| { + let mut vals = values + .into_iter() + .map(|item| item.into_expression()) + .collect::>(); + vals.insert(0, pattern_splat_as_expr(pre_splat)); + vals.push(pattern_splat_as_expr(post_splat)); + + format_list_like_thing_items(ps, vals, None, false); + }), + ); + })); } fn pattern_splat_as_expr(var_field: VarField) -> Expression { @@ -3466,7 +3470,10 @@ fn pattern_splat_as_expr(var_field: VarField) -> Expression { if let Some(name) = var_field.1 { ident.push_str(name.to_local_string().as_str()); } - Expression::Ident(Ident::new(ident, LineCol(var_field.2.start_line(), 0))) + Expression::Ident(Ident::new( + ident, + LineCol(var_field.2.map(|se| se.start_line()).unwrap_or(0), 0), + )) } pub fn format_retry(ps: &mut dyn ConcreteParserState, r: Retry) { diff --git a/librubyfmt/src/ripper_tree_types.rs b/librubyfmt/src/ripper_tree_types.rs index a5ef810b..1a7914cb 100644 --- a/librubyfmt/src/ripper_tree_types.rs +++ b/librubyfmt/src/ripper_tree_types.rs @@ -271,12 +271,11 @@ impl Expression { .as_ref() .and_then(|exprs| exprs.first().and_then(|expr| expr.start_line())) .or_else(|| { - Some( - star.as_ref() - .expect("If first exprs list is empty, there must be a * pattern") - .2 - .start_line(), - ) + star.as_ref() + .expect("If first exprs list is empty, there must be a * pattern") + .2 + .as_ref() + .map(|se| se.start_line()) }), // Pick the first of either expression, since these can be e.g. `foo..bar` or `foo..` or `..bar` Expression::Dot2(Dot2(_, maybe_first_expr, maybe_second_expr)) @@ -328,7 +327,9 @@ pub enum MLhsInner { impl MLhsInner { pub fn start_line(&self) -> Option { match self { - MLhsInner::VarField(VarField(.., start_end)) => Some(start_end.start_line()), + MLhsInner::VarField(VarField(.., start_end)) => { + start_end.as_ref().map(|se| se.start_line()) + } MLhsInner::Field(Field(_, expr, ..)) => expr.start_line(), MLhsInner::RestParam(RestParam(.., rest_param_assignable)) => rest_param_assignable .as_ref() @@ -611,7 +612,9 @@ impl RestParamAssignable { match self { RestParamAssignable::ArefField(ArefField(.., linecol)) | RestParamAssignable::Ident(Ident(.., linecol)) => Some(linecol.0), - RestParamAssignable::VarField(VarField(.., start_end)) => Some(start_end.start_line()), + RestParamAssignable::VarField(VarField(.., start_end)) => { + start_end.as_ref().map(|se| se.start_line()) + } } } } @@ -632,7 +635,9 @@ pub enum Assignable { impl Assignable { pub fn start_line(&self) -> Option { match self { - Assignable::VarField(VarField(.., start_end)) => Some(start_end.start_line()), + Assignable::VarField(VarField(.., start_end)) => { + start_end.as_ref().map(|se| se.start_line()) + } Assignable::RestParam(RestParam(.., rest_param_assignable)) => rest_param_assignable .as_ref() .and_then(|rpa| rpa.start_line()), @@ -671,7 +676,11 @@ pub struct ConstPathField(pub const_path_field_tag, pub Box, pub Con def_tag!(var_field_tag, "var_field"); #[derive(Deserialize, Debug, Clone)] -pub struct VarField(pub var_field_tag, pub Option, pub StartEnd); +pub struct VarField( + pub var_field_tag, + pub Option, + pub Option, // In rare cases, this can be nil, see `on_var_field` in rubyfmt_lib +); def_tag!(field_tag, "field"); #[derive(Deserialize, Debug, Clone)] @@ -2397,7 +2406,7 @@ impl ExpressionOrVarField { match self { ExpressionOrVarField::Expression(expr) => expr, ExpressionOrVarField::VarField(var_field) => { - let start_line = var_field.2.start_line(); + let start_line = var_field.2.map(|se| se.start_line()).unwrap_or(0); Expression::Ident(Ident::new( var_field .1 @@ -2412,7 +2421,9 @@ impl ExpressionOrVarField { pub fn start_line(&self) -> Option { match self { ExpressionOrVarField::Expression(expr) => expr.start_line(), - ExpressionOrVarField::VarField(var_field) => Some(var_field.2.start_line()), + ExpressionOrVarField::VarField(var_field) => { + var_field.2.as_ref().map(|se| se.start_line()) + } } } }