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

Decide whether K"parens" should be visible to SyntaxNode #239

Closed
c42f opened this issue Mar 31, 2023 · 1 comment · Fixed by #269
Closed

Decide whether K"parens" should be visible to SyntaxNode #239

c42f opened this issue Mar 31, 2023 · 1 comment · Fixed by #269
Labels
Milestone

Comments

@c42f
Copy link
Member

c42f commented Mar 31, 2023

As of #222 etc we have a parse node for grouping parentheses. This is extremely useful for tooling and has allowed several parsing difficulties to be solved neatly.

But it's pretty inconvenient when using SyntaxNode in a way that few other tree nodes are - parentheses are permitted in many unusual situations in the reference parser, and often treated as completely invisible to the parser.

Also SyntaxNode is AST-like, but grouping parens are arguably syntax trivia and shouldn't be in the AST anyway.

So maybe we should include parens only in the green tree, but elide them when converting to SyntaxNode.

@c42f c42f added the design label Mar 31, 2023
@c42f c42f added this to the 0.4 milestone Mar 31, 2023
c42f added a commit that referenced this issue May 3, 2023
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
Copy link
Member Author

c42f commented May 3, 2023

Ugh this is pretty awkward no matter which way you look at it. Diverging the GreenNode and SyntaxNode trees for this one particular case feels bad.

Also when I tried to fix this in #265 I found that approach (make SyntaxNode point to its outermost GreenNode parent of type `K"parens") quite awkward as described there.

Another alternative approach is to have SyntaxNode always point to the associated GreenNode of the same kind. In this case, no SyntaxNode would ever point to a GreenNode of type K"parens".

The irritation there is that we occasionally need in Expr conversion to know whether a SyntaxNode was wrapped in a GreenNode of type K"parens" for emulating some of the Expr inconsistencies.

Hmm. Maybe it's time to convert directly from GreenNode to Expr, avoiding SyntaxNode entirely? That would be more efficient in any case...

c42f added a commit that referenced this issue 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 as completed in #269 May 7, 2023
c42f added a commit that referenced this issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant