Fix combined cartesian / flattened generators #263
Conversation
Codecov Report
@@ Coverage Diff @@
## main #263 +/- ##
=======================================
Coverage 96.41% 96.41%
=======================================
Files 15 15
Lines 3929 3931 +2
=======================================
+ Hits 3788 3790 +2
Misses 141 141
|
|
I'm pretty sure this is a good approach at this point, but as usual deciding on a satisfying naming is kind of tricky. I don't love @timholy or @BenChung thoughts? I find the use of |
|
Side note ... returning to tackle this issue has been psyching me out for months now, it's a relief to have finally done it :-) |
The combined cartesian/flattened generator syntax is a very unusual use
case found in only one or two packages in the general registry. It looks
like this
((a,b,c,d) for a=as, b=bs for c=cs, d=ds)
Practically all such generators end up as a flat list (though ... should
they?) but the ordering of tuples (a,b,c,d) in the list depends on which
parts are cartesian (separated by commas) and which parts are flattened
(separated by for's).
Here we fix the `SyntaxNode` representation of these, preferring a single
`generator` head rather than combined `flatten` and `generator`, and
instead grouping the sets of cartesian iterations into
`cartesian_iterator` groups. This keeps the tree structure flat and
closer to the source text while also faithfully representing these
unusual forms. For example, the above parses as
(generator
(tuple a b c d)
(cartesian_iterator
(= a as)
(= b bs))
(cartesian_iterator
(= c cs)
(= d ds)))
In addition, we also make use of the `cartesian_iterator` head in `for`
loops, replacing the `block` which was used previously (but is not
semantically or syntactically a block!). Thus `for i=is, j=js body end`
now parses as
(for
(cartesian_iterator
(= i is)
(= j js))
body)
This also improves Expr conversion, removing some special cases which
were necessary when processing `block's` in that situation.
c42f
force-pushed
the
c42f/generator-flatten-fixes
branch
from
b7cd78c to
4714172
Compare
|
I'll go ahead and merge this because I think it's generally correct and a good idea. Still interested in hearing feedback though! |
|
Right - sorry, in the middle of moving (cities & jobs) so cycles are a bit wanting at the moment. Should be back to normal by next week. I think that's this is a good change. Understanding the semantics of cartesian iterators was a big headache for SemanticAST and this'd make it a lot clearer what's happening. I'll need to rework my representation a little but that goes without saying. |
|
Awesome, thanks for the feedback Ben, much appreciated ❤️ |
The combined cartesian/flattened generator syntax is a very unusual use case found in only one or two packages in the general registry. It looks like this
Practically all such generators end up as a flat list (though ... should they?) but the ordering of tuples (a,b,c,d) in the list depends on which parts are cartesian (separated by commas) and which parts are flattened (separated by for's).
Here we fix the
SyntaxNoderepresentation of these, preferring a singlegeneratorhead rather than combinedflattenandgenerator, and instead grouping the sets of cartesian iterations intocartesian_iteratorgroups. This keeps the tree structure flat and closer to the source text while also faithfully representing these unusual forms. For example, the above parses asIn addition, we also make use of the
cartesian_iteratorhead inforloops, replacing theblockwhich was used previously (but is not semantically or syntactically a block!). Thusfor i=is, j=js body endnow parses asThis also improves Expr conversion, removing some special cases which were necessary when processing
block'sin that situation.Fixes perhaps the last important case in #134 🎉