fix(pipeline): widen lower_where sig to accept any predicate expression#40
Open
cuzzo wants to merge 1 commit into
Open
fix(pipeline): widen lower_where sig to accept any predicate expression#40cuzzo wants to merge 1 commit into
cuzzo wants to merge 1 commit into
Conversation
PipelineHost#lower_where had `expr_node: AST::BinaryOp` in its Sorbet sig. But CLEAR's `WHERE` clause accepts ANY boolean expression: comparisons (`x > 0`), function calls (`isPositive(x)`), literals (`TRUE`), identifiers (`flag`), unary ops (`!cond`), method calls (`x.isPositive()`), etc. The narrow sig caused a runtime Sorbet TypeError on every non-BinaryOp predicate that flowed through this method: Parameter 'expr_node': Expected type AST::BinaryOp, got type AST::FuncCall ... The Zig backend's `lower_pipeline` didn't typically reach `lower_where` for these expressions (its CONCURRENT path falls back to the legacy string-based `transpile_pipeline`), which is why master tests stayed green -- the bug only surfaced via the register-VM emitter's bc-specific concurrent dispatch (register_bc_emitter.rb routing CONCURRENT WHERE through PipelineHost#lower_where with the user-written predicate). Fix: match `visit_pipeline_expr_mir`'s `T.untyped` (the method `lower_where`'s body delegates to). A union like `T.any(AST::BinaryOp, AST::FuncCall)` would still reject other valid predicate shapes; matching the sibling's precedent is the correct width. Sister methods `lower_select` and `lower_take_while` have no sig at all -- they take any expression too. This brings `lower_where` into line. New regression spec: spec/pipeline_host_lower_where_sig_spec.rb constructs a lower_where call with each of the 6 valid predicate AST shapes and asserts no `Parameter 'expr_node'` TypeError is raised. Without this fix, 5 of the 6 cases fail. Found by VM stress-test: 80_concurrent_where (CONCURRENT WHERE isPositive(_.value)) on the bc target.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40 +/- ##
==========================================
- Coverage 90.09% 90.08% -0.02%
==========================================
Files 185 185
Lines 49984 49987 +3
Branches 11984 11986 +2
==========================================
- Hits 45035 45031 -4
- Misses 4949 4956 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Bencher Report| Branch | hotfix-pipeline-where |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | leak-build-ms | Measure (units) x 1e3 | leak-count | Measure (units) | leak-run-ms | Measure (units) |
|---|---|---|---|---|---|---|
| benchmarks/concurrent/05_backpressure/bench | 📈 view plot | 5.45 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 1,648.35 units |
| benchmarks/concurrent/10_shard_vs_locked/bench | 📈 view plot | 5.23 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 60,004.42 units |
| benchmarks/concurrent/16_observables/bench | 📈 view plot | 5.37 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 96.93 units |
| benchmarks/inter-clear/03_concurrent_mvcc_vs_rwlock/bench | 📈 view plot | 2.77 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 371.86 units |
| benchmarks/sequential/07_pointer_chase/bench | 📈 view plot | 1.79 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 553.17 units |
| benchmarks/sequential/12_weak_ref_graph/bench | 📈 view plot | 1.82 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 259.83 units |
| benchmarks/server/03_pathological/server | 📈 view plot | 1.94 units x 1e3 | 📈 view plot | 0.00 units | 📈 view plot | 1,002.75 units |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
PipelineHost#lower_wherehad a Sorbet sig that constrainedexpr_nodetoAST::BinaryOp, but CLEAR'sWHEREclause accepts ANY boolean expression. The narrow sig caused a runtime Sorbet TypeError for any non-BinaryOp predicate.Found while VM-stress-testing the register backend:
transpile-tests/80_concurrent_where.chtusesWHERE isPositive(_.value)(aFuncCallpredicate) and tripped the sig.Why master appeared green
The Zig backend's pipeline pipeline routes CONCURRENT operators through the legacy string-based
transpile_pipelinepath, which doesn't reachlower_wherefor these expressions. The bc emitter's bc-specific concurrent dispatch (lower_bc_concurrent_where) callslower_wheredirectly with the user-written predicate — that's where the sig fires. So the bug was latent on master and only surfaced via the register VM.Fix
One line: match
visit_pipeline_expr_mir'sT.untyped(which the method's body delegates to). A narrower union (T.any(AST::BinaryOp, AST::FuncCall, AST::Literal)) would still reject other valid shapes (Identifier, UnaryOp, MethodCall, ...). Sibling methodslower_selectandlower_take_whilehave no sig at all — this bringslower_whereinto line.Regression test
spec/pipeline_host_lower_where_sig_spec.rbconstructs alower_wherecall with each of 6 valid predicate AST shapes (BinaryOp / FuncCall / Literal / Identifier / UnaryOp / MethodCall) and asserts noParameter 'expr_node'TypeError is raised.Without this fix, 5 of the 6 cases fail. Verified by stashing the fix and re-running the spec.
Test plan
bundle exec rspec spec/pipeline_host_lower_where_sig_spec.rb— 7 examples, 0 failuresbundle exec prspec spec/— 4691 examples, 0 failures./clear test transpile-tests/— 540 passed, 0 leakstranspile-tests/80_concurrent_where.chtpasses on the register VM (will be visible on register-machine PR rebase)🤖 Generated with Claude Code