Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crystal tool format on 'Range' #13202

Open
willy610 opened this issue Mar 19, 2023 · 3 comments · May be fixed by #13917 or #13901
Open

crystal tool format on 'Range' #13202

willy610 opened this issue Mar 19, 2023 · 3 comments · May be fixed by #13917 or #13901
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up kind:bug topic:tools:formatter

Comments

@willy610
Copy link

crystal tool format

Problems when formatting source containing

result_set.rows =(from..to).map { |i| result_set.rows[i] }

Solved by inserting space after the = like

result_set.rows = (from..to).map { |i| result_set.rows[i] }

crystal -v
Crystal 1.7.3 (2023-03-07)

LLVM: 14.0.6
Default target: aarch64-apple-darwin22.3.0

Same problem and solution in VC Code and Extensions

@Blacksmoke16
Copy link
Member

What the actual error is, plus trace:

$ ccrystal tool format --show-backtrace test.cr 
Using compiled compiler at /home/george/dev/git/crystal/.build/crystal
Index out of bounds (IndexError)
  from /home/george/dev/git/crystal/src/indexable.cr:803:12 in 'last'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2660:22 in 'visit'
  from /home/george/dev/git/crystal/src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
  from /home/george/dev/git/crystal/src/compiler/crystal/syntax/visitor.cr:20:7 in 'accept'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:4812:24 in 'indent'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2897:11 in 'format_args_simple'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2859:54 in 'format_args'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2853:5 in 'format_args'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2850:29 in 'format_call_args'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:2736:11 in 'visit'
  from /home/george/dev/git/crystal/src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:22:7 in 'format'
  from /home/george/dev/git/crystal/src/compiler/crystal/tools/formatter.cr:5:5 in 'format'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:169:7 in 'format'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:137:16 in 'format_source'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:133:7 in 'format_file'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:120:11 in 'format_file_or_directory'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:113:9 in 'format_many'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:102:9 in 'run'
  from /home/george/dev/git/crystal/src/compiler/crystal/command/format.cr:63:5 in 'format'
  from /home/george/dev/git/crystal/src/compiler/crystal/command.cr:174:10 in 'tool'
  from /home/george/dev/git/crystal/src/compiler/crystal/command.cr:114:7 in 'run'
  from /home/george/dev/git/crystal/src/compiler/crystal/command.cr:51:5 in 'run'
  from /home/george/dev/git/crystal/src/compiler/crystal/command.cr:50:3 in 'run'
  from /home/george/dev/git/crystal/src/compiler/crystal.cr:11:1 in '__crystal_main'
  from /home/george/dev/git/crystal/src/crystal/main.cr:115:5 in 'main_user_code'
  from /home/george/dev/git/crystal/src/crystal/main.cr:101:7 in 'main'
  from /home/george/dev/git/crystal/src/crystal/main.cr:127:3 in 'main'
  from /usr/lib/libc.so.6 in '??'
  from /usr/lib/libc.so.6 in '__libc_start_main'
  from /home/george/dev/git/crystal/.build/crystal in '_start'
  from ???

couldn't format './test.cr', please report a bug including the contents of it: https://github.com/crystal-lang/crystal/issues

@straight-shoota straight-shoota added good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up labels May 26, 2023
@hovsater
Copy link
Contributor

hovsater commented Oct 23, 2023

I've begun looking into this. The problem can be found here:

if assignment
skip_space
next_token
if @token.type.op_lparen?
write "=("
slash_is_regex!
next_token
format_call_args(node, true, base_indent)
skip_space_or_newline
write_token :OP_RPAREN
else
write " ="
skip_space
accept_assign_value_after_equals node.args.last
end
return false
end

Specifically, if we find a left parentheses, we treat it differently from regular assignment. The problem of course is that range literals starts with the same operand. I was hoping I could use node.has_parentheses? to check whether or not parentheses were used, but unfortunately, it seems like it always returns false.

I'd love to get some input and guidance on how to move forward. 🙂

@hovsater hovsater linked a pull request Oct 23, 2023 that will close this issue
@straight-shoota
Copy link
Member

straight-shoota commented Oct 30, 2023

I think the current formatter behaviour is a bit weird.

foo =(bar) formats to foo=(bar). That's a possible interpretation, but foo = (bar) is probably the more common expectation.
foo= (bar) formats to x.foo = (bar) which seems better to me.

Changing this behaviour keeps the logic pretty sleek:

--- i/src/compiler/crystal/tools/formatter.cr
+++ w/src/compiler/crystal/tools/formatter.cr
@@ -2732,10 +2732,12 @@ module Crystal
       passed_backslash_newline = @token.passed_backslash_newline

       if assignment
+        space_before_assignment = @token.type.space?
         skip_space

         next_token
-        if @token.type.op_lparen?
+        if @token.type.op_lparen? && !space_before_assignment
           write "=("
           slash_is_regex!
           next_token

As a result, only foo=( is considered standard call syntax. If there's any white space around =, the special assignment syntax is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. help wanted This issue is generally accepted and needs someone to pick it up kind:bug topic:tools:formatter
Projects
None yet
4 participants