fix: pipe operator into module-qualified calls (#326)#410
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFixes pipe desugaring in the type checker to accept module-qualified calls as pipe targets, adds unit and conformance tests for that case, updates the conformance manifest, and adjusts documentation and metrics (conformance suite count and known-issues entry). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
=======================================
Coverage 90.30% 90.30%
=======================================
Files 49 49
Lines 19104 19107 +3
Branches 220 220
=======================================
+ Hits 17252 17255 +3
Misses 1848 1848
Partials 4 4
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:
|
#326) _check_pipe in checker/expressions.py only handled FnCall on the RHS; ModuleCall was passed to the fallback _synth_expr which saw an arg-less call and emitted E201. The fix synthesises a new ast.ModuleCall with the LHS prepended and delegates to _check_module_call — mirroring the pattern already used in wasm/operators.py and smt.py. Also adds: - Two regression tests in TestModuleCallParsed (single + chained pipe) - Conformance test ch04_pipe_module_call.vera (level: run, vera.math::abs) - Removes #326 from KNOWN_ISSUES.md Co-Authored-By: Claude <noreply@anthropic.invalid>
6a202e2 to
ae85a53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_checker.py`:
- Around line 3308-3335: Add a test that locks the argument order for piping
into module-qualified calls that already take arguments: create a new test (e.g.
test_pipe_module_call_arg_order_regression) that uses the same mod =
self._resolved(("math",), self.MATH_MODULE) setup and constructs a source where
a value is piped into a module call that already has an asymmetric argument (for
example: `{ `@Int.0` |> math::max(123) }`) then run parse_to_ast and typecheck
with resolved_modules=[mod] and assert no error diagnostics; this ensures the
pipe inserts the receiver as the first argument (math::max(value, 123)) rather
than appending and will catch future prepend/append regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6edb91f3-1568-4660-879c-a62bd0f17179
⛔ Files ignored due to path filters (2)
tests/conformance/ch04_pipe_module_call.verais excluded by!**/*.veratests/conformance/vera/math.verais excluded by!**/*.vera
📒 Files selected for processing (6)
CLAUDE.mdKNOWN_ISSUES.mdTESTING.mdtests/conformance/manifest.jsontests/test_checker.pyvera/checker/expressions.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TESTING.md (1)
83-120:⚠️ Potential issue | 🟡 MinorDocumentation counts are now internally inconsistent.
The updated 65-program values are correct in this section, but the file still references older totals at Line 154 (
315 tests) and Lines 319 and 361 (64 conformance programs). Please update those lines to keep TESTING.md self-consistent.🛠️ Suggested doc fix
-# Via pytest (parametrized — 315 tests) +# Via pytest (parametrized — 325 tests) -| `check_conformance.py` | All 64 conformance programs pass their declared level (parse/check/verify/run) | +| `check_conformance.py` | All 65 conformance programs pass their declared level (parse/check/verify/run) | -| `check_conformance.py` | All 64 conformance programs pass their declared level | +| `check_conformance.py` | All 65 conformance programs pass their declared level |As per coding guidelines, "Review Markdown files for factual accuracy against the codebase, broken links, and outdated information."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TESTING.md` around lines 83 - 120, Update TESTING.md so all conformance counts match the conformance suite's actual size (65 programs). Replace the outdated numeric references ("315 tests" and every "64 conformance programs") with the correct values consistent with the manifest and the section that lists 65 programs; double-check the Directory structure and any "Count" column entries in the "Test levels" table and the textual summaries to use 65 where appropriate (refer to tests/conformance/ and manifest.json to verify). Ensure no other scattered references to 64 or 315 remain so the document is internally consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@TESTING.md`:
- Around line 83-120: Update TESTING.md so all conformance counts match the
conformance suite's actual size (65 programs). Replace the outdated numeric
references ("315 tests" and every "64 conformance programs") with the correct
values consistent with the manifest and the section that lists 65 programs;
double-check the Directory structure and any "Count" column entries in the "Test
levels" table and the textual summaries to use 65 where appropriate (refer to
tests/conformance/ and manifest.json to verify). Ensure no other scattered
references to 64 or 315 remain so the document is internally consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d3a128f-c26a-4b02-b41e-0acf0ff9b0c0
⛔ Files ignored due to path filters (2)
tests/conformance/ch04_pipe_module_call.verais excluded by!**/*.veratests/conformance/vera/math.verais excluded by!**/*.vera
📒 Files selected for processing (6)
CLAUDE.mdKNOWN_ISSUES.mdTESTING.mdtests/conformance/manifest.jsontests/test_checker.pyvera/checker/expressions.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
- Regenerate docs/ site assets (conformance count 64→65) - Fix remaining TESTING.md stale references: 315→325 parametrized tests, two more 64→65 conformance counts in script table and pre-commit table - Add test_pipe_module_call_arg_order_regression to lock down that the pipe LHS is prepended as first arg (not appended) - Update test_checker.py count 496→497, line count 5495→5519 - Update total test count 3103→3104 Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_checker.py`:
- Around line 3322-3334: In test_pipe_chained_module_calls_typechecks, the
source string imports an unused symbol `max`; update the import to only bring in
`abs` by changing the import clause in the test's source from "import math(abs,
max);" to "import math(abs);" so the test only imports what it uses (reference:
the test function name test_pipe_chained_module_calls_typechecks and the source
variable containing the import).
- Around line 3336-3358: The test test_pipe_module_call_arg_order_regression
currently uses math::max which is symmetric (max(`@Int`,`@Int`)) so it cannot catch
prepend-vs-append regressions; change the test to call an asymmetric function
(e.g. math::tag with signature tag(`@Int`,`@String` -> `@String`)) so the LHS being
appended would produce a type error—update the test source to import math(tag)
and use the pipe expression `@Int.0` |> math::tag("ok") (or add tag to MATH_MODULE
and then use that symbol) so the argument ordering is actually validated by the
typechecker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3a16427-78a7-48a2-b575-d3dae49fe01d
⛔ Files ignored due to path filters (1)
docs/llms.txtis excluded by!docs/**
📒 Files selected for processing (2)
TESTING.mdtests/test_checker.py
- Add tag(@int, @string -> @string) to MATH_MODULE so prepend-vs-append is type-observable: if LHS were appended, checker would see String where Int is expected and emit a type error - Replace symmetric math::max(123) with math::tag("ok") in the arg-order regression test - Fix unused 'max' import in test_pipe_chained_module_calls_typechecks - Update test_checker.py line count in TESTING.md (5519→5522) Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai The TESTING.md counts were already fixed in commit f43cf89 (pushed before this review round) — all |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Summary
_check_pipeinvera/checker/expressions.pyonly handledFnCallon the RHS. AModuleCallfell through to the_synth_exprfallback, which saw a zero-argument call and emitted a spurious E201 (arity mismatch)ast.ModuleCallwith the LHS prepended as first arg and delegates to_check_module_call— the same pattern already used inwasm/operators.pyandsmt.py(which both handled this correctly)Module::fn(@T.0)where a pipe would have been natural) — none found beyond the intentional qualified-call demo inexamples/modules.veraChanges
vera/checker/expressions.py— addModuleCallbranch to_check_pipetests/test_checker.py— two regression tests inTestModuleCallParsed(single pipe, chained pipes)tests/conformance/ch04_pipe_module_call.vera— newrun-level conformance test usingvera.math::abs()tests/conformance/vera/math.vera—vera.mathstub for the conformance suite resolverKNOWN_ISSUES.md— remove Pipe operator with module-qualified calls produces spurious E201 #326Test plan
vera checkaccepts@Int.0 |> mod::fn()without E201x |> mod::f() |> mod::g()type-check correctlyCloses #326
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements