Skip to content

Commit

Permalink
Refactor 'Crystal::Formatter#format_def_args' (#4992)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@makenowjust
makenowjust authored and Martin Verzilli committed Sep 17, 2017
1 parent af8c4f6 commit 74050a4
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 169 deletions.
19 changes: 14 additions & 5 deletions spec/compiler/formatter/formatter_spec.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"

Expand Down
242 changes: 89 additions & 153 deletions src/compiler/crystal/tools/formatter.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions src/compiler/crystal/tools/init.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 5 additions & 4 deletions src/llvm/lib_llvm_ext.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 74050a4

Please sign in to comment.