Repository navigation
[codex] Add parser-only pipe operator sugar - #462
Conversation
ChatGPT ReviewPrinciple audit. FAIL-CLOSED. This diff mostly satisfies fail-closed. The new chatgpt-review-555f4e69-f689-42… ILLEGAL STATES UNREPRESENTABLE. This mostly satisfies the principle. The good call here is what the PR did not add: there is no new FACTS FLOW FORWARD. Partially satisfied. The new parse-stage facts that matter downstream do flow forward: callee name, argument order with the piped lhs injected as arg0, left-to-right nesting, and full expression span are all preserved in the desugared chatgpt-review-555f4e69-f689-42… COPRODUCT DISSOLUTION. At the important boundary, this is the right direction. The PR closes a surface grammar gap without growing chatgpt-review-8bfd33ee-ee6b-40… SINGLE AUTHORITY. This is the blocking problem. chatgpt-review-555f4e69-f689-42… chatgpt-review-31b85e04-383d-4b… chatgpt-review-7e00b910-daa0-49… API-LEVEL ENFORCEMENT. Also violated, for the same root reason. Nothing in the API forces pipe targets to stay in lockstep with ordinary call targets; a future contributor can widen normal call syntax and forget to widen chatgpt-review-555f4e69-f689-42… Design question. Is Verdict. REQUEST_CHANGES. The direction is mostly right: parser-only normalization and no new IR node are exactly the right instincts. But the implementation sets the pattern at the wrong level by hand-parsing a second call-target grammar in LOOP HEALTH: 📈 CONVERGING — this round closes a surface gap without adding a new IR variant, but it should not lock in a duplicate parser authority for calls. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87c47b8a99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn parse_pipe(&mut self) -> Result<SurfaceExpr, Diagnostic> { | ||
| let mut lhs = self.parse_comparison()?; | ||
| while matches!(self.peek().kind, TokenKind::PipeArrow) { | ||
| self.bump(); | ||
| lhs = self.parse_pipe_target(lhs)?; |
There was a problem hiding this comment.
Let piped expressions compose with later operators
parse_expr now routes through parse_pipe, and parse_pipe only consumes |> stages after an initial comparison parse; it never lets the resulting expression flow back into additive/comparison parsing. As a result, inputs like 5 |> negate + 1 or x |> f == y leave +/== unconsumed and fail to parse, even though pipe desugars to an ordinary call. This is a functional limitation introduced with the pipe grammar and is especially problematic because this parser currently has no parenthesized-expression fallback.
Useful? React with 👍 / 👎.
Review — APPROVEClean Prereq 5 implementation. Parser-only, no substrate or lowering changes, exactly per §4.3. What's right:
One test gap (non-blocking): No test for the error case — e.g., Scope note from the PR body is accurate: Verdict: ship it. Prereq 5 done. |
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 87c47b8a
BLOCKING (1)
Root Cause
src/v3/compiler/src/parse.rsPipe was added as a bespoke outer grammar stage instead of through the same precedence path that the syntax spec owns → integrate|>into the existing operator-precedence parser so the parser anddag_syntax_speccannot drift.
Non-blocking — Strengths
src/v3/compiler/tests/pipe_desugar.rsThe tests exercise unary injection, multi-arg injection, and left-to-right chaining at the DAG boundary, which is the right real consumer for a parser-only feature.
ROADMAP — Verified
- Prereq 5 parser-only desugar shape: The diff keeps pipe out of lowering and rewrites it to ordinary
Callnodes during parsing, which matches docs/substrate-reflection-design.md §4.3.
ROADMAP — Incomplete
- Prereq 5 precedence fidelity: The prerequisite is not complete until the parser matches the authoritative
|>binding power already declared indsl/extdeps/languages/dag/syntax.dag.
|> precedence is wrong in the new grammar and that is meaningfully harder to unwind after more pipe syntax lands.
| } | ||
|
|
||
| fn parse_pipe(&mut self) -> Result<SurfaceExpr, Diagnostic> { | ||
| let mut lhs = self.parse_comparison()?; |
There was a problem hiding this comment.
BLOCKING: Parsing the pipe left-hand side as a full comparison makes |> weaker than ==, +, and *, which contradicts the single syntax authority in dsl/extdeps/languages/dag/syntax.dag and violates INVARIANTS "No duplicate representations".
|
BLOCKING (1) Root Cause
Non-blocking — Strengths
ROADMAP — Verified
ROADMAP — Incomplete
|
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
Director review — #462 (Prereq 5, pipe sugar)LGTM. Exactly matches §4.3's specification: pure parser sugar, first-arg injection, desugars during parsing, no substrate or lowering change. Tokenizer: Parser: Lowering: Comment-only updates removing "pipes" from scaffold gap lists. No functional changes. Tests: 5+ tests covering unary pipe, multi-arg injection, left-to-right chaining, precedence with addition, precedence with comparison. Structural assertions on the desugared Call shape confirm the desugar is happening at parse time. Design coherence: §4.3 says "by the time lowering runs, there is no pipe — just a normal function call." The implementation matches — No issues. Ship-ready. |
|
Meta-review in progress... (view conversation) Loop-health check: is this review cycle making forward progress, or shifting debt? Posts in ~5-15 minutes. |
What changed
|>tokenization to the v3 lexerx |> ftof(x)x |> f(y, z)parses asf(x, y, z)Why it changed
docs/substrate-reflection-design.md§4.3 specifies pipe as pure parser sugar, not a substrate or lowering feature. The parser previously had no|>token or desugar step, so prereq 5 was still missing.Impact
Callexpressions after parse-time rewritingRoot cause
The tokenizer only recognized plain
|, and the expression parser stopped at comparison/additive/term/primary without a pipe stage. That left|>unavailable even though the design already committed to first-arg injection.Validation
cargo test -p v3-compilerNotes
[1,2,3] |> lengthexample still depends on the separate list-literal parser work; this PR keeps scope to parser-only pipe sugar and validates the desugar structurally with the current grammar