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

AST source locations for named mapping keys. #13864

Closed
ekpyron opened this issue Jan 11, 2023 · 0 comments · Fixed by #13877
Closed

AST source locations for named mapping keys. #13864

ekpyron opened this issue Jan 11, 2023 · 0 comments · Fixed by #13877
Assignees
Milestone

Comments

@ekpyron
Copy link
Member

ekpyron commented Jan 11, 2023

Following up on #13384: we missed the fact that the key names in that PR do not carry source location information, which will be an issue for tooling, which relies on everything in the AST being annotated with source fields.

It's not particularly nice, but the most straightforward solution would be to copy the behaviour for the names of VariableDeclarations (those have a nameLocation field besides the name field, i.e. we could just add keyNameLocation and valueNameLocation).

Once we accumulate a worthwhile set of breaking AST changes, we can generally transition parsing of the mapping keys and values themselves to parsing as VariableDeclarations - that'd simplify this, but we can't do it right away without breaking the AST, which we promised to avoid, when possible.

@zemse if you want to add those, let us know - although we'd need a PR rather soon (within the next week or so), since we're planning to release soon and this should be fixed beforehand. So if you don't have time, let us know as well ideally, then we can take care of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants