Improve reliability and aesthetics of comments stripper (#1085)

Reliably remove comments from Rails config files when generating
Suspenders apps. With the parser gem, we're now removing comments by
processing their surrounding code using AST (abstract syntax tree)
processors.

We were using a regex to do that, which is text-based and thereby
subject to edge cases that only a proper AST solution can handle.

** First motivation

This pull request, #1044,
made changes to some config files to fix the Action Mailer asset host.

While working on merging that PR, the tests were not passing because
they looked for an output that was missing in one of the changed
config files:

    config.action_mailer.asset_host = \
      %q{"https://#{ENV\.fetch\("ASSET_HOST", ENV\.fetch\("APPLICATION_HOST"\)\)}}

The PR code changes are kind of straightforward and that config option
should have been present in the config file but it was not.

The reason was that Suspenders was removing code comments with an
overly permissive regex : (/^.*#.*$/), which removed every line
containing a # character, no matter where they were in the line - so
it was also removing the new Action Mailer config setting.

That regex was also silently messing up with other config lines, for
example:

    config.public_file_server.headers = {
      'Cache-Control' => "public, max-age=#{2.days.to_i}"
    }

As a quick fix to merge that PR, I've changed the regex to /^\s*#.*$/
but it was not an ideal solution - which is what I'm striving for with
this commit. With the old text-based regex solution, there were still
nasty edge cases to deal with, and we may never know when they'd
occur. Also, the previous regex did not work with inline comments.

** Second motivation

The final aesthetics of the config files could be improved in my
opinion. This was how they looked like:

    Rails.application.configure do
      config.cache_classes = false
      config.eager_load = false
      config.consider_all_requests_local = true
      if Rails.root.join('tmp', 'caching-dev.txt').exist?
        config.action_controller.perform_caching = true
        config.action_controller.enable_fragment_cache_logging = true
        config.cache_store = :memory_store
        config.public_file_server.headers = {
          'Cache-Control' => "public, max-age=#{2.days.to_i}"
        }
      else
        config.action_controller.perform_caching = false
        config.cache_store = :null_store
      end
      config.active_storage.service = :local
      config.action_mailer.raise_delivery_errors = true
      config.after_initialize do
        Bullet.enable = true
        Bullet.bullet_logger = true
        Bullet.rails_logger = true
      end

      # ...
    end

And with the newest solution they look like this:

    Rails.application.configure do
      config.cache_classes = false
      config.eager_load = false
      config.consider_all_requests_local = true

      if Rails.root.join('tmp', 'caching-dev.txt').exist?
        config.action_controller.perform_caching = true
        config.action_controller.enable_fragment_cache_logging = true

        config.cache_store = :memory_store
        config.public_file_server.headers = {
          'Cache-Control' => "public, max-age=#{2.days.to_i}"
        }
      else
        config.action_controller.perform_caching = false
        config.cache_store = :null_store
      end

      config.action_mailer.raise_delivery_errors = true

      config.after_initialize do
        Bullet.enable = true
        Bullet.bullet_logger = true
        Bullet.rails_logger = true
      end

      # ...

Which preserves newlines where appropriate, so that's easier to read.

** Third motivation

This pull request uses the parser gem, and I feel like introducing
that gem in the project may be a good move over the long term.

For example, we need to finish up the gem action to sort gem
declarations alphabetically when adding gems to the Gemfile, and the
most reliable way to do that is with AST parsing.

** Other related changes

1. Make the indentation of some templates match the Rails
environment's config files'. This goes along with the aesthetics
introduced in this commit.

2. Because of (1), I've also found a bug that was concatenating two
lines of codes in config/environments/production.rb with no blank line
between them (in email_generator.rb), so I'm taking the opportunity to
fix it

3. standardrb fixes after updating to the latest standardrb version

.ruby-version

@@ -1 +1 @@
-2.7.2
+2.7.4

.standard.yml

@@ -1,2 +1,4 @@
 ignore:
   - "templates/partials/db_optimizations_configuration.rb"
+  - "templates/partials/pull_requests_config.rb"
+  - "templates/partials/email_smtp.rb"

lib/suspenders.rb

@@ -27,5 +27,6 @@
 require "suspenders/generators/production/single_redirect"
 require "suspenders/generators/staging/pull_requests_generator"
 require "suspenders/actions"
+require "suspenders/actions/strip_comments_action"
 require "suspenders/adapters/heroku"
 require "suspenders/app_builder"

lib/suspenders/actions.rb

@@ -6,7 +6,7 @@ def replace_in_file(relative_path, find, replace)
       unless contents.gsub!(find, replace)
         raise "#{find.inspect} not found in #{relative_path}"
       end
-      File.open(path, "w") { |file| file.write(contents) }
+      File.write(path, contents)
     end
 
     def action_mailer_host(rails_env, host)

lib/suspenders/actions/strip_comments_action.rb

@@ -0,0 +1,254 @@
+require "parser/current"
+
+module Suspenders
+  module Actions
+    class StripCommentsAction
+      class << self
+        def call(source)
+          parser = Parser::CurrentRuby.new
+
+          source
+            .then { |s| strip_comments(s, parser) }
+            .then { |s| strip_trailing_whitespace(s) }
+            .then { |s| strip_dup_newlines(s) }
+            .then { |s| strip_leading_scope_newlines(s, parser) }
+        end
+
+        private
+
+        def strip_comments(source, parser)
+          StripComments.call(source, parser.reset)
+        end
+
+        def strip_trailing_whitespace(source)
+          source.gsub(/[[:blank:]]+$/, "")
+        end
+
+        def strip_dup_newlines(source)
+          source.gsub(/\n{2,}/, "\n\n")
+        end
+
+        def strip_leading_scope_newlines(source, parser)
+          StripLeadingScopeNewlines.call(source, parser.reset)
+        end
+      end
+
+      # Strips full-line and inline comments from a buffer but does
+      # not remove whitespaces or newlines after the fact. Example
+      # input:
+      #
+      #     MyGem.application.configure do |config|
+      #       # Full-line comment
+      #       config.option1 = :value # Inline comment
+      #     end
+      #
+      # The output is:
+      #
+      #     MyGem.application.configure do |config|
+      #
+      #       config.option1 = :value
+      #     end
+      class StripComments
+        class << self
+          def call(source, parser)
+            buffer = Parser::Source::Buffer.new(nil, source: source)
+            rewriter = Parser::Source::TreeRewriter.new(buffer)
+
+            _, comments = parser.parse_with_comments(buffer)
+
+            comments.each do |comment|
+              strip_comment(comment, buffer, rewriter)
+            end
+
+            rewriter.process
+          end
+
+          private
+
+          def strip_comment(comment, buffer, rewriter)
+            expr = comment.location.expression
+
+            if full_line_comment?(expr)
+              expr = full_line_comment_expr(expr, buffer)
+            end
+
+            rewriter.remove(expr)
+          end
+
+          def full_line_comment_expr(expr, buffer)
+            pos = BackwardStringScanner.beginning_of_line_pos(expr, expr.begin_pos)
+
+            Parser::Source::Range.new(buffer, pos, expr.end_pos + 1)
+          end
+
+          def full_line_comment?(expr)
+            expr.source == expr.source_line.lstrip
+          end
+        end
+      end
+
+      # A tiny, non-stateful backward string scanner somewhat inspired
+      # by Ruby's StringScanner. Ruby's StringScanner is unable to
+      # seek backward on a string.
+      class BackwardStringScanner
+        def self.beginning_of_line_pos(expr, initial_pos)
+          skip_before(expr, initial_pos) { |char| char == "\n" }
+        end
+
+        def self.skip_before(expr, initial_pos, &block)
+          skip_until(expr, initial_pos, -1, &block)
+        end
+
+        def self.skip_until(expr, initial_pos, lookup_inc = 0)
+          pos = initial_pos
+
+          loop do
+            break if pos.zero?
+            char = expr.source_buffer.source[pos + lookup_inc]
+            break if yield(char)
+            pos -= 1
+          end
+
+          pos
+        end
+      end
+
+      # The intent of this class is purely aesthetic: remove leading
+      # newlines inside of code scopes like blocks and begin/end.
+      # Example input:
+      #
+      #     module MyGem
+      #
+      #       MyGem.application.configure do |config|
+      #
+      #         config.option1 = true
+      #
+      #         config.option2 = false
+      #       end
+      #     end
+      #
+      # The output is:
+      #
+      #     module MyGem
+      #       MyGem.application.configure do |config|
+      #         config.option1 = true
+      #
+      #         config.option2 = false
+      #       end
+      #     end
+      class StripLeadingScopeNewlines
+        def self.call(source, parser)
+          buffer = Parser::Source::Buffer.new(nil, source: source)
+          ast = parser.parse(buffer)
+
+          LeadingNewlineStripRewriter.new.rewrite(buffer, ast).lstrip
+        end
+
+        class LeadingNewlineStripRewriter < Parser::TreeRewriter
+          def on_module(node)
+            strip_newline_before(node.children[1])
+            strip_newline_after(node.children.last)
+
+            super
+          end
+
+          def on_class(node)
+            strip_newline_before(node.children[2])
+            strip_newline_after(node.children.last)
+
+            super
+          end
+
+          def on_begin(node)
+            handle_begin(node)
+
+            super
+          end
+
+          def on_kwbegin(node)
+            strip_newline_before(node.children[0])
+            strip_newline_after(node.children.last)
+
+            handle_begin(node)
+
+            super
+          end
+
+          def on_block(node)
+            strip_newline_before(node.children[2])
+            strip_newline_after(node.children.last)
+
+            super
+          end
+
+          private
+
+          def handle_begin(node)
+            strip_blank_lines_between_setter_calls(node.children)
+
+            node.children.each do |child_node|
+              send("on_#{child_node.type}", child_node)
+            end
+          end
+
+          def strip_blank_lines_between_setter_calls(children)
+            pairs = children.each_cons(2).to_a
+
+            pairs.each do |(node_before, node_after)|
+              if setter_call?(node_before) && setter_call?(node_after)
+                strip_newline_before(node_after)
+              end
+            end
+          end
+
+          def setter_call?(node)
+            node.children[1].to_s.end_with?("=")
+          end
+
+          def strip_newline_before(node)
+            return if node.nil?
+
+            expr = node.location.expression
+            end_pos = find_end_pos(expr, expr.begin_pos)
+            begin_pos = find_begin_pos(expr, end_pos)
+
+            strip_source_range(expr, begin_pos, end_pos)
+          end
+
+          def strip_newline_after(node)
+            return if node.nil?
+
+            expr = node.location.expression
+            source = expr.source_buffer.source
+
+            if source[expr.end_pos + 1] == "\n"
+              strip_source_range(expr, expr.end_pos, expr.end_pos + 1)
+            end
+          end
+
+          def find_end_pos(expr, begin_pos)
+            BackwardStringScanner.skip_until(expr, begin_pos) do |char|
+              char == "\n"
+            end
+          end
+
+          def find_begin_pos(expr, end_pos)
+            BackwardStringScanner.skip_before(expr, end_pos) do |char|
+              char != "\n" && char != " "
+            end
+          end
+
+          def strip_source_range(expr, begin_pos, end_pos)
+            remove(
+              Parser::Source::Range.new(
+                expr.source_buffer,
+                begin_pos,
+                end_pos
+              )
+            )
+          end
+        end
+      end
+    end
+  end
+end

lib/suspenders/app_builder.rb

@@ -205,15 +205,10 @@ def remove_config_comment_lines
       ]
 
       config_files.each do |config_file|
-        path = File.join(destination_root, "config/#{config_file}")
+        path = Pathname(destination_root).join("config", config_file)
+        source = Actions::StripCommentsAction.call(path.read)
 
-        accepted_content = File.readlines(path).reject { |line|
-          line =~ /^\s*#.*$/ || line =~ /^$\n/
-        }
-
-        File.open(path, "w") do |file|
-          accepted_content.each { |line| file.puts line }
-        end
+        path.write(source)
       end
     end
 

lib/suspenders/generators/production/email_generator.rb

@@ -7,14 +7,14 @@ def smtp_configuration
         copy_file "smtp.rb", "config/smtp.rb"
 
         prepend_file "config/environments/production.rb",
-          %{require Rails.root.join("config/smtp")\n}
+          %{require Rails.root.join("config/smtp")\n\n}
       end
 
       def use_smtp
         inject_template_into_file(
           "config/environments/production.rb",
           "partials/email_smtp.rb",
-          after: "config.action_mailer.perform_caching = false"
+          after: "config.action_mailer.perform_caching = false\n"
         )
       end
 

spec/support/file_operations.rb

@@ -1,6 +1,10 @@
 module FileOperations
   module_function
 
+  def read_file(file)
+    TestPaths.app_path.join(file).read
+  end
+
   def touch_file(file)
     path = app_path.join(file)
     path.join("..").mkpath