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

Parser: Add missing locations of various AST nodes #13452

Merged
merged 10 commits into from
May 16, 2023

Conversation

FnControlOption
Copy link
Contributor

Continuation of #11798

I also have a bunch more missing locations added locally but still need to add tests for them. I've prioritized nodes that don't have a start location since in many cases it's possible to infer the end_location from child nodes.

`foo : Bar` and `baz` in `->(foo : Bar, baz) { }`
@oprypin
Copy link
Member

oprypin commented May 10, 2023

Do you think it's possible to make a generated test suite that asserts that all nodes have correct locations?

@oprypin
Copy link
Member

oprypin commented May 10, 2023

(that's a separate discussion though, not something that should block this PR or be in it at all)

@straight-shoota
Copy link
Member

I've extracted that question to #13453 to cleanly separate from this PR discussion. It's a good one 👍

`@[Foo]` in `def x(@[Foo] y) end`

The location must be set inside `parse_annotation` for when the
latter is called directly instead of indirectly via `parse_atomic`.
These direct calls to `parse_annotation` are found in `parse_param`,
`parse_lib_body_exp`, and `parse_enum_body_expressions`.
`private def foo` in `enum Foo; private def foo; 1; end; end`
`{Foo, Bar}` in `x : {Foo, Bar}`
@straight-shoota straight-shoota added this to the 1.9.0 milestone May 15, 2023
@straight-shoota straight-shoota merged commit 206e04f into crystal-lang:master May 16, 2023
45 checks passed
@FnControlOption FnControlOption deleted the parser branch May 19, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants