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

Clean up and document syntax tree child access API + mark public API #484

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

c42f
Copy link
Member

@c42f c42f commented Aug 7, 2024

Here I commit to a more consistent but simpler child access API for syntax trees, as informed by the JuliaLowering work so far:

  • is_leaf(node) is given a precise definition (previously !haschildren() - but that had issues - see Rename haschildren() to is_leaf() #483)
  • children(node) returns the child list, or nothing if there are no children. The nothing might be seen as inconvenient, but mapping across the children of a leaf node is probably an error and one should probably branch on is_leaf first.
  • numchildren(node) is documented
  • node[i], node[i:j] are documented to index into the child list

We distinguish GreenNode and its implementation of span from SyntaxNode and its implementation of byte_range and sourcetext - these seem to just have very different APIs, at least as of now.

I've deleted the questionable overloads of multidimensional getindex and the child function in favor of single dimensional getindex. I don't know whether anyone ever ended up using these. But I didn't and they didn't seem useful+consistent enough to keep the complexity.

I've kept setindex! for now, to set a child of a SyntaxNode. Though I'm not sure this is a good idea to support by default and I didn't mark it public yet.

Also I've added more docs!

Also mark the public parts of the API as public in Julia 1.11+

@c42f c42f force-pushed the caf/tree-API-cleanup branch 2 times, most recently from 6cd1b85 to 57c8c3c Compare August 7, 2024 10:34
Copy link
Sponsor Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think TypedSyntax's tests do exploit multidimensional child, but that's an easy change. I presume this will become 0.5.0?

@c42f
Copy link
Member Author

c42f commented Aug 9, 2024

I presume this will become 0.5.0?

We need to get to 1.0 so we can use semver properly. So the next release will be 1.0.0 :-) I've been working really hard on a lot of 1.0.0 changes in the last few weeks, trying to get the documentation and APIs in better shape. See also #472, but I keep finding more to do - it turns out there's a lot of changes I want to squeeze in which are informed by the JuliaLowering work.

@c42f
Copy link
Member Author

c42f commented Aug 9, 2024

I think TypedSyntax's tests do exploit multidimensional child, but that's an easy change.

We could keep child() if you think it's useful? In that case I'd document what it does and mark it public. It's more the multidimensional getindex which is dubious and should probably be deleted.

@timholy
Copy link
Sponsor Member

timholy commented Aug 9, 2024

Either way is fine; it's not such a hard change to make to TypedSyntax's tests. A regex search suggests there are 74 uses of child(arg, i, j...) and that's not too bad to change if you think it would be cleaner without.

Here I commit to a more consistent but simpler child access API for
syntax trees, as informed by the JuliaLowering work so far:

* `is_leaf(node)` is given a precise definition (previously
  `!haschildren()` - but that had issues - see #483)
* `children(node)` returns the child list, or `nothing` if there are no
  children. The `nothing` might be seen as inconvenient, but mapping
  across the children of a leaf node is probably an error and one should
  probably branch on `is_leaf` first.
* `numchildren(node)` is documented
* `node[i]`, `node[i:j]` are documented to index into the child list

We distinguish `GreenNode` and its implementation of `span` from
`SyntaxNode` and its implementation of `byte_range` and `sourcetext` -
these seem to just have very different APIs, at least as of now.

I've deleted the questionable overloads of multidimensional `getindex`
and the `child` function in favor of single dimensional getindex. I
don't know whether anyone ever ended up using these. But I didn't and
they didn't seem useful+consistent enough to keep the complexity.

I've kept setindex! for now, to set a child of a `SyntaxNode`. Though
I'm not sure this is a good idea to support by default.
@c42f c42f merged commit 0a0aa04 into main Aug 9, 2024
36 checks passed
@c42f c42f deleted the caf/tree-API-cleanup branch August 9, 2024 20:09
@c42f
Copy link
Member Author

c42f commented Aug 9, 2024

Ok thanks! Let's go with the more minimal API then :)

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.

2 participants