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

Remove K"parens" from SyntaxNode #265

Closed
wants to merge 2 commits into from
Closed

Conversation

c42f
Copy link
Member

@c42f c42f commented May 3, 2023

One approach to fix #239:

  • Add a head field to SyntaxNode to separate it from GreenNode
  • When a K"parens" node is encountered in the green tree, replace it with its child.
  • However, have the replacement point to the parens GreenNode so we can still observe parentheses

This turns out to work, but is awkward. It's especially awkward/surprising for sourcetext, as we have things like

julia> kind(parsestmt(SyntaxNode, "(10)"))
K"Integer"

julia> sourcetext(parsestmt(SyntaxNode, "(10)"))
"(10)"

We could fix sourcetext() to avoid this inconsistency at the cost of traversing the raw tree dynamically. But then do we also need to change range()?

Overall this approach is not feeling great to me.

c42f added 2 commits May 4, 2023 06:29
Possible fix for #239:

* Add a `head` field to `SyntaxNode` to separate it from `GreenNode`
* When a `K"parens"` node is encountered in the green tree, replace it with its child.
* However, have the replacement point to the parens `GreenNode` so we can still observe parentheses

This turns out to work, but is awkward.  It's especially awkward/surprising for `sourcetext`, as we have things like

```julia
julia> kind(parsestmt(SyntaxNode, "(10)"))
K"Integer"

julia> sourcetext(parsestmt(SyntaxNode, "(10)"))
"(10)"
```

We could fix `sourcetext()` to avoid this inconsistency at the cost of traversing the raw tree dynamically.  But then do we also need to change `range()`?
c42f added a commit that referenced this pull request May 7, 2023
Another approach to fix #239.

Elide all `K"parens"` nodes from the `SyntaxNode` tree, replacing them
with their single child.

With #268 merged, this approach works more neatly and doesn't have the
downsides of #265.
c42f added a commit that referenced this pull request May 7, 2023
Another approach to fix #239.

Elide all `K"parens"` nodes from the `SyntaxNode` tree, replacing them
with their single child.

With #268 merged, this approach works more neatly and doesn't have the
downsides of #265.
@c42f c42f closed this in #269 May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide whether K"parens" should be visible to SyntaxNode
1 participant