celfmt: fold filter into consuming map during simplify - #107
Conversation
Add a peephole optimisation that rewrites filter-then-comprehension chains into a single comprehension with a filter argument: x.filter(v, cond).map(v, t) → x.map(v, cond, t) x.filter(v, cond).transformList(_, v, t) → x.transformList(_, v, cond, t) x.filter(v, cond).transformMap(_, v, t) → x.transformMap(_, v, cond, t) x.filter(v, cond).transformMapEntry(_, v, t) → x.transformMapEntry(_, v, cond, t) For two-variable comprehensions the merge is restricted to cases where the first iteration variable (index/key) is "_", since the filter alters element indices and merging would change semantics if the index is used.
chrisberkhout
left a comment
There was a problem hiding this comment.
I like reading it with the separate filter, but I guess this is okay. Is it more efficient?
I found some problems with AI that I think are real:
Bug 1: folding fails when the filter's source is itself a macro
This includes chained filters:
$ celfmt -s <<< 'state.a.filter(u, u > 0).filter(v, v < 10).map(v, v * 2)' failed to format program: unsupported expression: 0 $ celfmt -s <<< 'state.a.map(u, u + 1).filter(v, v > 0).map(v, v * 2)' failed to format program: unsupported expression: 0Root cause: cel-go stores nested macro references inside a macro-call tree as unspecified exprs carrying only the ID (
parser/helper.go:macroTarget = p.exprFactory.NewUnspecifiedExpr(target.ID())). The new macro call built by the fold keepsfilterTargetreferencing the source by its original ID, but the AST-update visitor then moves that macro entry away:filterSource := iterRange.AsComprehension().IterRange() if mcall, ok := info.GetMacroCall(filterSource.ID()); ok { info.SetMacroCall(iterRange.ID(), mcall) info.ClearMacroCall(filterSource.ID()) } iterRange.SetKindCase(filterSource)After the fold, the whole subtree is rendered from the macro tree keyed at the consuming comprehension's ID, so the source's entry must stay at its original ID — the remap breaks the reference the formatter follows. I verified the fix: deleting the
if mcall, ok := ...block (keeping onlyiterRange.SetKindCase(filterSource)) makes both cases fold correctly (state.a.map(u, u + 1).map(v, v > 0, v * 2)etc.) and all 177 tests still pass. The remap looks like it was pattern-matched frominlineAs, where it is needed. A regression test for the chained case belongs in the table.
Bug 2:
transform*folds change semantics when the source is a mapCEL's
filtermacro on a map iterates and yields keys, while the two-vartransform*comprehensions on a map bind (key, value). So for a map source the fold rebinds the variable from filtered keys to values. Verified with mito:state.m = {"a": 1, "b": 2, "c": 3} original: state.m.filter(k, k != "b").transformList(_, v, v) → ["a", "c"] folded: state.m.transformList(_, v, v != "b", v) → [3, 1, 2]The
_-index guard doesn't help here — the problem isn't index reuse, it's thatvnow binds values instead of keys. Sincecelfmtcompiles withstateasDyn, there's no type information to distinguish list from map sources, so I don't see a way to apply thetransformList/transformMap/transformMapEntryfolds soundly in general — they may need to be dropped, or gated on a syntactically provable list source. Themapfold is unaffected: bothfilterand themapmacro iterate keys on maps, som.filter(k, c).map(v, t)≡m.map(v, c, t)for both source types.
Minor notes
- The rename-capture guard (
filter_map_rename_capturetest) and the exact-count idempotency check are both solid.- After a rename, the underlying AST comprehension still uses the old variable name — only the macro tree is renamed. That's harmless today because rendering goes through the macro tree, but it leaves the AST and macro map inconsistent, same caveat as
inlineAs.- Tests pass (177 in 2 packages), but the table has no chained-filter, macro-source, or map-source cases — exactly where both bugs live.
There's a part that is fragile for future additions:
the second switch that builds
newArgsusesdefault:to mean "the three transform functions", andvalIdx == 1doubles as the "is a two-var comprehension" test. If someone adds a new function name to the first switch with a different shape, the second switch'sdefaultwill silently indexargs[2]for it. Mirroring the case list in both switches (or switching onvalIdx) would remove that trap.
Yes, it is. The filter needs to populate a new array, so a filter in front of a map makes unnecessary allocations. |
Uh oh!
There was an error while loading. Please reload this page.