Refactor 'Crystal::Formatter#format_def_args' (#4992)

makenowjust · Martin Verzilli · commit 74050a429050 · 2017-09-17T11:02:50.000-03:00
Fix #1924 Close #4980, #4990 This is just refactoring but it contains many bug fixes and improvements. These come from refactoring effects, so I cannot separate them from this. Bug fixes and improvements: - work formatting double splat with trailing comma (#4990) - work formatting when there is newline or comment between argument and comma. ```crystal def foo(a ,) end def foo(a # comment ,) end ``` is formatted to correctly: ```crystal def foo(a) end def foo(a # comment ) end ``` - format arguments with 2 spaces indentation (#1924) - distinguish comment followed on newline and argument. The formatter keeps this for example: ```crystal def foo(a # argument 'a' is ... ) end def foo(a # more argument here in future... ) end ``` And, perhaps there are other bug fixes and improvements because `format_def_args` has too many bugs to remember. Refactoring is so great!! * Rename 'wrote_newline' inside 'format_def_arg' to 'just_wrote_newline' And fix some conditions for readbility.
diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr
@@ -157,7 +157,8 @@ describe Crystal::Formatter do
   assert_format "def   foo (  x , y , )  \n  end", "def foo(x, y)\nend"
   assert_format "def   foo (  x , y ,\n)  \n  end", "def foo(x, y)\nend"
   assert_format "def   foo (  x ,\n y )  \n  end", "def foo(x,\n        y)\nend"
-  assert_format "def   foo (\nx ,\n y )  \n  end", "def foo(\n        x,\n        y)\nend"
+  assert_format "def   foo (\nx ,\n y )  \n  end", "def foo(\n  x,\n  y\n)\nend"
+  assert_format "class Foo\ndef   foo (\nx ,\n y )  \n  end\nend", "class Foo\n  def foo(\n    x,\n    y\n  )\n  end\nend"
   assert_format "def   foo (  @x)  \n  end", "def foo(@x)\nend"
   assert_format "def   foo (  @x, @y)  \n  end", "def foo(@x, @y)\nend"
   assert_format "def   foo (  @@x)  \n  end", "def foo(@@x)\nend"
@@ -581,6 +582,7 @@ describe Crystal::Formatter do
   assert_format "lib Foo\nfun foo(x : Int32, ... )  :  Int32\nend", "lib Foo\n  fun foo(x : Int32, ...) : Int32\nend"
   assert_format "lib Foo\n  fun foo(Int32) : Int32\nend"
   assert_format "fun foo(x : Int32) : Int32\n  1\nend"
+  assert_format "fun foo(\n  x : Int32,\n  ...\n) : Int32\n  1\nend"
   assert_format "lib Foo\n  fun foo = bar(Int32) : Int32\nend"
   assert_format "lib Foo\n  fun foo = \"bar\"(Int32) : Int32\nend"
   assert_format "lib Foo\n  $foo  :  Int32 \nend", "lib Foo\n  $foo : Int32\nend"
@@ -742,8 +744,8 @@ describe Crystal::Formatter do
   assert_format "foo(\n# x\n1,\n\n# y\nz: 2\n)", "foo(\n  # x\n  1,\n\n  # y\n  z: 2\n)"
   assert_format "foo(\n# x\n1,\n\n# y\nz: 2,\n\n# a\nb: 3)", "foo(\n  # x\n  1,\n\n  # y\n  z: 2,\n\n  # a\n  b: 3)"
   assert_format "foo(\n 1, # hola\n2, # chau\n )", "foo(\n  1, # hola\n  2, # chau\n)"
-  assert_format "def foo(\n\n#foo\nx,\n\n#bar\nz\n)\nend", "def foo(\n        # foo\n        x,\n\n        # bar\n        z)\nend"
-  assert_format "def foo(\nx, #foo\nz #bar\n)\nend", "def foo(\n        x, # foo\n        z  # bar\n        )\nend"
+  assert_format "def foo(\n\n#foo\nx,\n\n#bar\nz\n)\nend", "def foo(\n  # foo\n  x,\n\n  # bar\n  z\n)\nend"
+  assert_format "def foo(\nx, #foo\nz #bar\n)\nend", "def foo(\n  x, # foo\n  z  # bar\n)\nend"
   assert_format "a = 1;;; b = 2", "a = 1; b = 2"
   assert_format "a = 1\n;\nb = 2", "a = 1\nb = 2"
   assert_format "foo do\n  # bar\nend"
@@ -1017,11 +1019,18 @@ describe Crystal::Formatter do
   assert_format "foo(A |\nB |\nC)", "foo(A |\n    B |\n    C)"
   assert_format "def foo\n  case x\n  # z\n  when 1\n  end\nend"
   assert_format "foo { |x| (x).a }"
-  assert_format "def foo(\n        &block)\nend"
+  assert_format "def foo(\n  &block\n)\nend"
   assert_format "def foo(a,\n        &block)\nend"
-  assert_format "def foo(\n        a,\n        &block)\nend"
+  assert_format "def foo(\n  a,\n  &block\n)\nend"
   assert_format "def foo(a,\n        *b)\nend"
+  assert_format "def foo(a\n,        *b)\nend", "def foo(a,\n        *b)\nend"
+  assert_format "def foo(a # comment\n,        *b)\nend", "def foo(a, # comment\n        *b)\nend"
   assert_format "def foo(a,\n        **b)\nend"
+  assert_format "def foo(\n  **a\n)\n  1\nend"
+  assert_format "def foo(**a,)\n  1\nend", "def foo(**a)\n  1\nend"
+  assert_format "def foo(\n  **a # comment\n)\n  1\nend"
+  assert_format "def foo(\n  **a\n  # comment\n)\n  1\nend"
+  assert_format "def foo(\n  **a\n\n  # comment\n)\n  1\nend"
   assert_format "def foo(**b, # comment\n        &block)\nend"
   assert_format "def foo(a, **b, # comment\n        &block)\nend"
 
diff --git a/src/compiler/crystal/tools/formatter.cr b/src/compiler/crystal/tools/formatter.cr
@@ -1390,190 +1390,126 @@ module Crystal
     end
 
     def format_def_args(args : Array, block_arg, splat_index, variadic, double_splat)
-      to_skip = 0
-
-      # If there are no args, remove extra "()", if any
-      if args.empty?
+      # If there are no args, remove extra "()"
+      if args.empty? && !block_arg && !double_splat && !variadic
         if @token.type == :"("
-          prefix_size = @column + 1
-
-          write "(" if block_arg || double_splat || variadic
-          next_token
-
-          skip_space
-          if @token.type == :NEWLINE
-            write_line
-            write_indent(prefix_size)
-            skip_space_or_newline(prefix_size)
-          end
-
-          if double_splat
-            write_token :"**"
-            double_splat.accept self
-            skip_space_or_newline
-          end
-
-          if block_arg
-            if double_splat
-              write_token :","
-              found_comment = skip_space_or_newline
-              if found_comment
-                write_indent(prefix_size)
-              else
-                write " "
-              end
-            end
-            write_token :"&"
-            skip_space
-            to_skip += 1 if at_skip?
-            accept block_arg
-            skip_space_or_newline
-          end
-
-          if variadic
-            skip_space_or_newline
-            write_token :"..."
-            skip_space_or_newline
-          end
-
+          next_token_skip_space_or_newline
           check :")"
           next_token
-          write ")" if block_arg || double_splat || variadic
         end
-      else
-        prefix_size = @column + 1
+        return 0
+      end
 
-        old_indent = @indent
-        next_needs_indent = false
-        has_parentheses = false
-        found_comment = false
+      # Count instance variable arguments. See `at_skip?`.
+      to_skip = 0
 
-        if @token.type == :"("
-          has_parentheses = true
-          write "("
-          next_token_skip_space
-          if @token.type == :NEWLINE
-            write_line
-            skip_space_or_newline(prefix_size)
-            next_needs_indent = true
-          end
-          skip_space_or_newline
-        else
-          write "("
-        end
+      wrote_newline = false
+      found_first_newline = false
 
-        comma_written = false
+      old_indent = @indent
+      @indent = @column + 1
 
-        args.each_with_index do |arg, i|
-          if next_needs_indent
-            write_indent(prefix_size)
-          end
+      write_token :"("
+      skip_space
+
+      # When "(" follows newline, it turns on two spaces indentation mode.
+      if @token.type == :NEWLINE
+        @indent = old_indent + 2
+        found_first_newline = true
+        wrote_newline = true
 
+        write_line
+        next_token_skip_space_or_newline
+      end
+
+      args.each_with_index do |arg, i|
+        has_more = !last?(i, args) || double_splat || block_arg || variadic
+        wrote_newline = format_def_arg(wrote_newline, has_more) do
           if i == splat_index
             write_token :"*"
             skip_space_or_newline
+            next if arg.external_name.empty? # skip empty splat argument.
           end
 
-          if i == splat_index && arg.external_name.empty?
-            # Nothing
-          else
-            indent(prefix_size, arg)
-            to_skip += 1 if @last_arg_is_skip
-          end
-
-          skip_space
-
-          if @token.type == :","
-            has_more = !last?(i, args) || double_splat || block_arg
-
-            if has_more
-              write ","
-              comma_written = true
-            end
-            next_token
-            found_comment = skip_space
-            if @token.type == :NEWLINE
-              if has_more
-                indent(prefix_size) { consume_newlines }
-                next_needs_indent = true
-              end
-            elsif found_comment
-              next_needs_indent = true
-            else
-              next_needs_indent = false
-              write " " if has_more
-            end
-            skip_space_or_newline
-          else
-            comma_written = false
-          end
+          arg.accept self
+          to_skip += 1 if @last_arg_is_skip
         end
+      end
 
-        if double_splat
-          if next_needs_indent
-            write_indent(prefix_size)
-            next_needs_indent = false
-          end
-          unless comma_written
-            write ", "
-            comma_written = true
-          end
+      if double_splat
+        wrote_newline = format_def_arg(wrote_newline, block_arg) do
           write_token :"**"
-          double_splat.accept self
           skip_space_or_newline
-          if block_arg
-            check :","
-            write ","
-            comma_written = true
-            found_comment = next_token_skip_space
-            if found_comment
-              next_needs_indent = true
-              found_comment = false
-            else
-              if @token.type == :NEWLINE
-                indent(prefix_size) { consume_newlines }
-                next_needs_indent = true
-              else
-                write " "
-              end
-            end
-          end
+
+          to_skip += 1 if at_skip?
+          double_splat.accept self
         end
+      end
 
-        if block_arg
-          if next_needs_indent
-            write_indent(prefix_size)
-            next_needs_indent = false
-          end
-          unless comma_written
-            write ", "
-            comma_written = true
-          end
+      if block_arg
+        wrote_newline = format_def_arg(wrote_newline, false) do
           write_token :"&"
-          skip_space
+          skip_space_or_newline
+
           to_skip += 1 if at_skip?
-          accept block_arg
-          skip_space
+          block_arg.accept self
         end
+      end
 
-        if variadic
-          write_token ", ", :"..."
-          skip_space_or_newline
+      if variadic
+        wrote_newline = format_def_arg(wrote_newline, false) do
+          write_token :"..."
         end
+      end
 
-        if has_parentheses
-          skip_space_or_newline
-          write_indent(prefix_size) if found_comment
-          write_token :")"
+      if found_first_newline && !wrote_newline
+        write_line
+        wrote_newline = true
+      end
+      write_indent(found_first_newline ? old_indent : @indent) if wrote_newline
+      write_token :")"
+
+      @indent = old_indent
+
+      to_skip
+    end
+
+    def format_def_arg(wrote_newline, has_more)
+      write_indent if wrote_newline
+
+      yield
+
+      # Write "," before skipping spaces to prevent inserting comment between argument and comma.
+      write "," if has_more
+
+      just_wrote_newline = skip_space
+      if @token.type == :NEWLINE
+        if has_more
+          consume_newlines
+          just_wrote_newline = true
         else
-          write_indent(prefix_size) if found_comment
-          write ")"
+          # `last: true` is needed to write newline and comment only if comment is found.
+          just_wrote_newline = skip_space_or_newline(last: true)
         end
+      end
 
-        @indent = old_indent
+      if @token.type == :","
+        found_comment = next_token_skip_space
+        if found_comment
+          just_wrote_newline = true
+        elsif @token.type == :NEWLINE
+          if has_more && !just_wrote_newline
+            consume_newlines
+            just_wrote_newline = true
+          else
+            just_wrote_newline |= skip_space_or_newline(last: true)
+          end
+        else
+          write " " if has_more && !just_wrote_newline
+        end
       end
 
-      to_skip
+      just_wrote_newline
     end
 
     # The parser transforms `def foo(@x); end` to `def foo(x); @x = x; end` so if we
diff --git a/src/compiler/crystal/tools/init.cr b/src/compiler/crystal/tools/init.cr
@@ -102,13 +102,14 @@ module Crystal
       property silent : Bool
 
       def initialize(
-                     @skeleton_type = "none",
-                     @name = "none",
-                     @dir = "none",
-                     @author = "none",
-                     @email = "none",
-                     @github_name = "none",
-                     @silent = false)
+        @skeleton_type = "none",
+        @name = "none",
+        @dir = "none",
+        @author = "none",
+        @email = "none",
+        @github_name = "none",
+        @silent = false
+      )
       end
     end
 
diff --git a/src/llvm/lib_llvm_ext.cr b/src/llvm/lib_llvm_ext.cr
@@ -14,10 +14,11 @@ lib LibLLVMExt
   fun di_builder_finalize = LLVMDIBuilderFinalize(DIBuilder)
 
   fun di_builder_create_function = LLVMDIBuilderCreateFunction(
-                                                               builder : DIBuilder, scope : Metadata, name : Char*,
-                                                               linkage_name : Char*, file : Metadata, line : UInt,
-                                                               composite_type : Metadata, is_local_to_unit : Bool, is_definition : Bool,
-                                                               scope_line : UInt, flags : LLVM::DIFlags, is_optimized : Bool, func : LibLLVM::ValueRef) : Metadata
+    builder : DIBuilder, scope : Metadata, name : Char*,
+    linkage_name : Char*, file : Metadata, line : UInt,
+    composite_type : Metadata, is_local_to_unit : Bool, is_definition : Bool,
+    scope_line : UInt, flags : LLVM::DIFlags, is_optimized : Bool, func : LibLLVM::ValueRef
+  ) : Metadata
 
   fun di_builder_create_file = LLVMDIBuilderCreateFile(builder : DIBuilder, file : Char*, dir : Char*) : Metadata
   fun di_builder_create_compile_unit = LLVMDIBuilderCreateCompileUnit(builder : DIBuilder,
