formatting expressions with paren elision (?) is not idempotent

[froydnj]

Commit: dc86aa6

Input

→ View on rubyfmt.run

# initial input
module Amp
  def self.trim_team(team)
    {
      descendent_members: (team[:descendant_members].map { |v| self.trim_person } if team[descendant_members]
        .present?)
    }
  end
end

# output of formatting the above
module Amp
  def self.trim_team(team)
    {
      descendent_members: (if team[descendant_members].present?
        team[:descendant_members].map { |v| self.trim_person }
      end)
    }
  end
end

# output of formatting the above, which is
# - not idempotent
# - what formatting the first bit should have produced in the first place
module Amp
  def self.trim_team(team)
    {
      descendent_members: (
        if team[descendant_members].present?
          team[:descendant_members].map { |v| self.trim_person }
        end
      )
    }
  end
end

Output

# initial input
module Amp
  def self.trim_team(team)
    {
      descendent_members: (if team[descendant_members].present?
        team[:descendant_members].map { |v| self.trim_person }
      end)
    }
  end
end

# output of formatting the above
module Amp
  def self.trim_team(team)
    {
      descendent_members: (
        if team[descendant_members].present?
          team[:descendant_members].map { |v| self.trim_person }
        end
      )
    }
  end
end

# output of formatting the above, which is
# - not idempotent
# - what formatting the first bit should have produced in the first place
module Amp
  def self.trim_team(team)
    {
      descendent_members: (
        if team[descendant_members].present?
          team[:descendant_members].map { |v| self.trim_person }
        end
      )
    }
  end
end

Expected behavior

See embedded comments above.

[froydnj]

I believe this is the last blocking issue for reformatting Stripe's codebase, though there are some test failures I think I need to chase down.

Looking at #787, I don't see how the is_multiline_node logic could be correct for modifier forms: the above testcase is proof that we can and do convert modifier forms into non-modifier forms as appropriate. IIUC the only way it could be made correct is to actually render the relevant nodes and make the determination from there, or insert a bunch of logic into the rendering process so parens could be correctly stripped out there. Either of those is an incredibly big hammer to apply.

WDYT @fables-tales ?

[froydnj]

I guess the alternative is that we could just match the Ripper behavior for whether to multiline modifier forms:

cdb4ebb#diff-63b92541cacb647f9227c6eac250e906cb60dade9a39a892517f2d99ec122a28L2910-L2911

Compare the prism logic:

rubyfmt/librubyfmt/src/format_prism.rs

Lines 3740 to 3766 in bf6d619

	// For while/until, always use the inline format for modifiers, since
	// some transformations are unsafe.
	// For if/unless, check if we should convert to block form.
	let should_convert_to_block = match conditional {
	Conditional::While(_) | Conditional::Until(_) => false,
	Conditional::If(_) | Conditional::Unless(_) => {
	let predicate = conditional.predicate();
	let statements = conditional.statements();
	let predicate_start_line =
	ps.get_line_number_for_offset(predicate.location().start_offset());
	let predicate_end_line =
	ps.get_line_number_for_offset(predicate.location().end_offset());
	if predicate_start_line != predicate_end_line {
	true
	} else {
	// Check if it renders multiline due to length
	ps.will_render_as_multiline(|next_ps| {
	format_inline_conditional(
	next_ps,
	predicate,
	statements,
	conditional_keyword,
	)
	})
	}
	}
	};

[reese]

I'd be curious to hear what fable thinks, but I think it's perhaps more straightforward to just not elide parens for conditionals at all and just make these checks return true. Like, the conditional is on multiple lines in some of the examples in #814, so I think those should break to the block form since they're split across multiple lines. If Ripper didn't break to block form for those, I would consider that a Ripper bug. (This is another reason I personally had FUD with the paren elission behavior -- deciding whether or not to render a node based on the multilining decisions of its inner nodes seems ~fraught, IMO.)

[froydnj]

I still have FUD with the paren elision, but I think the paren elision code doesn't get called for the bits in #814, so I think dealing with paren elision is a separate issue. I'm not particularly happy with the fix in #814, since it makes things a good bit slower, and it would be nice if a better solution could be found.

[reese]

FWIW I'm fine to merge #814 if that unblocks you landing stuff at Stripe, but I wonder if the real fix is making paren expressions breakables so that if they work like

(render_single_line if single_line?)
(
  if is_multi_line?
    render_multi_line!
  end
)

# This would break to multi-line like the above when formatted
(this_would_render_to_multiline if some_really_long_expression_over_120...) # you get the idea

Making them breakables would go hand-in-hand with #778