[fix] Render inline conditionals in source order to fix line handling (#894)

* Fixtures

* Render conditionals in source order and fixup comments

* Combine indenting/dedenting and operate in-place

Author: reese

fixtures/small/multiline_call_in_modifier_conditional_actual.rb

@@ -0,0 +1,82 @@
+module Example
+  class Application < Rails::Application
+    config.middleware.insert_before(
+      ActionDispatch::Cookies,
+      SessionCookieUpgrader
+    ) if Rails.env.staging? || Rails.env.production?
+  end
+end
+
+foo(
+  a,
+  b
+) if c
+
+foo(
+  a,
+  b
+) unless c
+
+foo(
+  a,
+  b
+) if some_method(
+  x,
+  y
+)
+
+foo(a, b) if some_method(
+  x,
+  y
+)
+
+# Surrounding blank lines and a leading comment should be preserved when
+# the modifier conditional gets converted to block form.
+
+# leading comment
+config.middleware.insert_before(
+  ActionDispatch::Cookies,
+  SessionCookieUpgrader
+) if Rails.env.staging? || Rails.env.production?
+
+# Multiple modifier conditionals separated by blank lines.
+foo(
+  a
+) if c
+
+bar(
+  b
+) unless d
+
+# Comments and blank lines inside the statement body are preserved.
+foo(
+  a,
+  # mid-arg comment
+  b
+) if cond
+
+foo(
+  a,
+
+  b
+) if cond
+
+foo(
+  a,
+
+  # comment after blank
+  b
+) if cond
+
+# Trailing comment on the modifier-if line is hoisted as a leading comment.
+foo(
+  a,
+  b
+) if cond # trailing
+
+def m
+  foo(
+    a,
+    b
+  ) if cond # trailing
+end

fixtures/small/multiline_call_in_modifier_conditional_expected.rb

@@ -0,0 +1,110 @@
+module Example
+  class Application < Rails::Application
+    if Rails.env.staging? || Rails.env.production?
+      config.middleware.insert_before(
+        ActionDispatch::Cookies,
+        SessionCookieUpgrader
+      )
+    end
+  end
+end
+
+if c
+  foo(
+    a,
+    b
+  )
+end
+
+unless c
+  foo(
+    a,
+    b
+  )
+end
+
+if some_method(
+    x,
+    y
+  )
+  foo(
+    a,
+    b
+  )
+end
+
+if some_method(
+    x,
+    y
+  )
+  foo(a, b)
+end
+
+# Surrounding blank lines and a leading comment should be preserved when
+# the modifier conditional gets converted to block form.
+
+# leading comment
+if Rails.env.staging? || Rails.env.production?
+  config.middleware.insert_before(
+    ActionDispatch::Cookies,
+    SessionCookieUpgrader
+  )
+end
+
+# Multiple modifier conditionals separated by blank lines.
+if c
+  foo(
+    a
+  )
+end
+
+unless d
+  bar(
+    b
+  )
+end
+
+# Comments and blank lines inside the statement body are preserved.
+if cond
+  foo(
+    a,
+    # mid-arg comment
+    b
+  )
+end
+
+if cond
+  foo(
+    a,
+
+    b
+  )
+end
+
+if cond
+  foo(
+    a,
+
+    # comment after blank
+    b
+  )
+end
+
+# Trailing comment on the modifier-if line is hoisted as a leading comment.
+# trailing
+if cond
+  foo(
+    a,
+    b
+  )
+end
+
+def m
+  # trailing
+  if cond
+    foo(
+      a,
+      b
+    )
+  end
+end

librubyfmt/src/comment_block.rs

@@ -54,17 +54,24 @@ impl CommentBlock {
         })
     }
 
+    /// Set each comment's leading indent to exactly `indent_depth` spaces
     pub fn apply_spaces(mut self, indent_depth: ColNumber) -> Self {
-        if indent_depth == 0 {
-            return self;
-        }
-
-        let indent = get_indent(indent_depth as usize);
+        let target = indent_depth as usize;
         for comment in &mut self.comments {
             // Ignore empty vecs -- these represent blank lines between
             // groups of comments
-            if !comment.is_empty() && !comment.starts_with(b"=begin") {
-                comment.to_mut().splice(0..0, indent.iter().copied());
+            if comment.is_empty() || comment.starts_with(b"=begin") {
+                continue;
+            }
+            let current = comment.iter().take_while(|&&b| b == b' ').count();
+            if current == target {
+                continue;
+            }
+            if current > target {
+                comment.to_mut().drain(0..(current - target));
+            } else {
+                let extra = get_indent(target - current);
+                comment.to_mut().splice(0..0, extra.iter().copied());
             }
         }
         self

librubyfmt/src/parser_state.rs

@@ -958,19 +958,21 @@ impl<'src> ParserState<'src> {
         self.breakable_entry_stack
             .push(Breakable::InlineConditional(entry));
 
+        // Format in source order — statement before predicate — so that
+        // `on_line` calls advance in the order of the source
         self.with_start_of_line(false, |ps| {
-            ps.new_block(format_predicate);
+            ps.new_block(format_statement);
         });
 
         self.breakable_entry_stack
             .last_mut()
             .expect("just pushed InlineConditional")
             .as_conditional_layout_mut()
             .expect("just pushed InlineConditional")
-            .switch_to_statement();
+            .switch_to_predicate();
 
         self.with_start_of_line(false, |ps| {
-            ps.new_block(format_statement);
+            ps.new_block(format_predicate);
         });
 
         let cle = self
@@ -980,7 +982,21 @@ impl<'src> ParserState<'src> {
             .into_conditional_layout()
             .expect("InlineConditional always returns Some from into_conditional_layout");
 
+        // Preserve any comments that were extracted during the conditional
+        // (e.g. a trailing `# comment` on the modifier line) so they aren't
+        // dropped when restoring the comments that were pending beforehand.
+        // The accumulated comments may have an inner-scope indent baked in
+        // (from `apply_spaces` at capture time), so re-apply at the
+        // conditional's outer indent before merging.
+        let outer_spaces = self.current_spaces();
+        let accumulated = self
+            .comments_to_insert
+            .take()
+            .map(|c| c.apply_spaces(outer_spaces));
         self.comments_to_insert = saved_comments;
+        if let Some(acc) = accumulated {
+            self.comments_to_insert.merge(acc);
+        }
 
         self.push_target(ConcreteLineTokenAndTargets::ConditionalLayoutEntry(cle));
     }

librubyfmt/src/render_targets.rs

@@ -533,7 +533,7 @@ impl<'src> ConditionalLayoutEntry<'src> {
             statement_tokens: Vec::new(),
             keyword,
             indent_depth,
-            phase: ConditionalLayoutPhase::Predicate,
+            phase: ConditionalLayoutPhase::Statement,
         }
     }
 
@@ -544,13 +544,13 @@ impl<'src> ConditionalLayoutEntry<'src> {
         }
     }
 
-    pub fn switch_to_statement(&mut self) {
+    pub fn switch_to_predicate(&mut self) {
         debug_assert_eq!(
             self.phase,
-            ConditionalLayoutPhase::Predicate,
-            "switch_to_statement called when not in Predicate phase"
+            ConditionalLayoutPhase::Statement,
+            "switch_to_predicate called when not in Statement phase"
         );
-        self.phase = ConditionalLayoutPhase::Statement;
+        self.phase = ConditionalLayoutPhase::Predicate;
     }
 
     /// Returns the number of tokens in the current phase's token list.