You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently we turn rewrite rules into Rust code directly inside the macros that parse the rules themselves. This makes it somewhat difficult to keep track of what's going on, and difficult to easily extend the syntax.
This PR splits code generation into two parts. First we parse the surface syntax into a (hopefully) sensible and intuitive AST representation. This representation is then lowered into Rust source code in a separate pass.
This also means we can now test each component separately, but in order to keep the PR somewhat small, I decided not to change the tests at this time. The existing end-to-end (observing the behaviour of the compiled rules) continue to work without issue.
The first commit takes care of the tree templates -- the output part of rules, whereas the second handles the patterns we match the input CST against (the "input" to a rule).
Worth noting is the fact that templates get lowered into raw Rust code for constructing the appropriate tree (which is not entirely trivial, due to the postfix ? operator for discarding entire subtrees if they contain interpolated Nones), whereas queries/patterns are interpreted at runtime. (Compiling these into static Rust code is not entirely straightforward due to things like backtracking, and in practice query interpretation is fast enough).
This does mean there's a bit of awkwardness where patterns are concerned -- we parse them into an AST when handling the rule! macro, and then map this to an essentially equivalent Query AST for interpretation. I considered consolidating these two into a single structure, and it may be worth pursuing later, but it made it a lot harder to see what's going on in this PR, so I ultimately decided no to do it at this point.
Adds a structured representation of tree templates, with a lowering
operation for turning them into Rust code. Thus, instead of generating
the code directly inside the rule parser, we now create the internal
AST, and then lower it into Rust code. This makes the boundary between
parsing and code generation cleaner and easier to test.
Extends the previous work. We now explicitly split apart parsing of
rules from lowering them into Rust code.
There is some redundancy here -- a Pattern is basically the same as the
Query that we eventually execute, but cleaning that up would introduce a
lot of churn, so I've tried to leave it as-is for now.
This is a weird pattern (that we don't use) that had weird semantics
previously.
A minimal example would be something like
```
((item)* (separator))* @Items
```
Here, it's not really clear what @Items should be capturing. The current
implementation (post-AST rewrite) simply panics at compile-time, with a
message that may or may not be helpful. As the Copilot review correctly
points out, this is a regression compared to the previous behaviour
(which dealt with the issue by just throwing away the `(item)*` bit).
This compiled, but I don't think the old behaviour is particularly
sensible either.
For that reason, we now explicitly warn that capturing a group
containing a nested repetition is an error.
At present, we never employ the above pattern in our code. If this
changes, then we'll likely want to provide a sensible semantics for this
case rather than just panicking.
Copilot's second review sent me down a bit of a rabbit hole.
The fundamental question is the following: what (if anything) does it
mean to capture a query sequence?
That is, consider the following query:
```
(foo ((bar) (baz)) @quux)
```
Here, `@quux` is attached to a subquery that matches a sequence of
nodes, `bar` followerd by `baz`. So, what should this actually capture?
The current implementation treats this in a somewhat surprising way,
equivalent to if we had written
```
(foo ((bar) @quux (baz) @quux))
```
that is, both kinds of child nodes get pushed into the `quux` capture.
To me, this behaviour is very surprising and unintuitive (and since
there is an explicit way to express this anyway, there doesn't seem to
be much reason to prefer the "shorthand" version).
For this reason, the present commit just disallows adding a capture to a
query sequence entirely. A capture is only attached to a single query
node, never a sequence of nodes.
Note that this does not mean that a _capture_ can't contain a sequence
of nodes. Both
```
(foo (bar)* @bars)
```
and
```
(foo ((bar) @bars)*)
```
(which are equivalent) capture all of the `bar` children and put them in
`bars` as a `Vec<Id>`.
In the new AST, we represent the syntax in the order it appears, so
`(bar)* @bars` is a capture of a repeated node match. However, when we
lower this into an executable query, we push the capture in, so it
becomes more like the second version. We're still only capturing a
single node at a time, but we may do so several times due to the
repetition.
(One might consider getting rid of the first syntax entirely, then, but
I think it's more readable to write this as "capture of repeat" rather
than "repeat of capture".)
Finally, in addition to no longer allowing captures to apply to
sequences, we now also do not allow sequences to be empty (i.e. `()`) or
contain a single element. The first would have no effect (an empty
sequence always succeeds), and the second is redundant (a singleton
sequence is equivalent to its single element). All of these checks are
enforced at compile-time.
The public macro still expands to yeast::query::QueryNode; Pattern is the new crate-private intermediate AST that parse_query_top immediately lowers. Describing the public result as Pattern documents a type callers cannot access and misstates the macro API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently we turn rewrite rules into Rust code directly inside the macros that parse the rules themselves. This makes it somewhat difficult to keep track of what's going on, and difficult to easily extend the syntax.
This PR splits code generation into two parts. First we parse the surface syntax into a (hopefully) sensible and intuitive AST representation. This representation is then lowered into Rust source code in a separate pass.
This also means we can now test each component separately, but in order to keep the PR somewhat small, I decided not to change the tests at this time. The existing end-to-end (observing the behaviour of the compiled rules) continue to work without issue.
The first commit takes care of the tree templates -- the output part of rules, whereas the second handles the patterns we match the input CST against (the "input" to a rule).
Worth noting is the fact that templates get lowered into raw Rust code for constructing the appropriate tree (which is not entirely trivial, due to the postfix
?operator for discarding entire subtrees if they contain interpolatedNones), whereas queries/patterns are interpreted at runtime. (Compiling these into static Rust code is not entirely straightforward due to things like backtracking, and in practice query interpretation is fast enough).This does mean there's a bit of awkwardness where patterns are concerned -- we parse them into an AST when handling the
rule!macro, and then map this to an essentially equivalentQueryAST for interpretation. I considered consolidating these two into a single structure, and it may be worth pursuing later, but it made it a lot harder to see what's going on in this PR, so I ultimately decided no to do it at this point.