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

Incorrect output for the to_s method of Attribute AST nodes #4098

Closed
olbat opened this issue Mar 2, 2017 · 3 comments
Closed

Incorrect output for the to_s method of Attribute AST nodes #4098

olbat opened this issue Mar 2, 2017 · 3 comments
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. status:implemented topic:compiler
Milestone

Comments

@olbat
Copy link
Contributor

olbat commented Mar 2, 2017

I was playing around with the Crystal's parser and got this issue:

$ cat test.cr
require "compiler/crystal/syntax"
ast = Crystal::Parser.parse(%{@[Test(1, 2, n1: 1, n2: 2)]})
puts code = ast.to_s
Crystal::Parser.parse(code)


$ crystal run test.cr
@[Test(1, 2, n1: 1n2: 2)]
expecting token ')', not 'n2' (Crystal::SyntaxException)
0x4e0937: ??? at /opt/crystal/src/compiler/crystal/syntax/lexer.cr 2711:9
0x4e0907: ??? at /opt/crystal/src/compiler/crystal/syntax/lexer.cr 2710:5
0x4e33ec: *Crystal::Parser#check<Symbol>:Nil at /opt/crystal/src/compiler/crystal/syntax/parser.cr 5353:7
0x50dce2: *Crystal::Parser#parse_attribute:Crystal::Attribute at /opt/crystal/src/compiler/crystal/syntax/parser.cr 1182:15
...


$ crystal --version
Crystal 0.21.0 [c2c2276] (2017-02-21)

I think there must be something wrong with the to_s method of Attribute AST nodes: a comma is missing between the two named arguments ...

@olbat olbat changed the title Incorrect output for the to_s method of Attributes AST nodes with multiple named arguments Incorrect output for the to_s method of Attribute AST nodes Mar 2, 2017
olbat added a commit to olbat/crystal that referenced this issue Mar 2, 2017
Commas are missing in the result of the `to_s` method of Attribute nodes
so the output is invalid (i.e. `@[Foo(a: 1b: 2)]`).
@olbat
Copy link
Contributor Author

olbat commented Mar 2, 2017

I tried to come with a fix #4099, just le me know if I did something wrong.

I also noticed that there is very few tests of the output to_s on C bindings-related AST nodes in the spec, should I add some at the same time ?

@RX14
Copy link
Contributor

RX14 commented Mar 2, 2017

@olbat I'm sure more specs are always welcome!

@olbat
Copy link
Contributor Author

olbat commented Mar 2, 2017

Done !

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Mar 2, 2017
olbat added a commit to olbat/crystal that referenced this issue Mar 3, 2017
Commas are missing in the result of the `to_s` method of Attribute nodes
so the output is invalid (i.e. `@[Foo(a: 1b: 2)]`).
olbat added a commit to olbat/crystal that referenced this issue Mar 3, 2017
Commas are missing in the result of the `to_s` method of Attribute nodes
so the output is invalid (i.e. `@[Foo(a: 1b: 2)]`).
asterite pushed a commit that referenced this issue Mar 3, 2017
Commas are missing in the result of the `to_s` method of Attribute nodes
so the output is invalid (i.e. `@[Foo(a: 1b: 2)]`).
@bcardiff bcardiff added this to the Next milestone Mar 6, 2017
@bcardiff bcardiff closed this as completed Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. status:implemented topic:compiler
Projects
None yet
Development

No branches or pull requests

4 participants