Enforce range non-associativity and item-modifier placement#74
Merged
StreamDemon merged 3 commits intoJul 2, 2026
Merged
Conversation
Two remaining over-acceptances from the section-16 review:
- The precedence table marks `..`/`..=` as non-associative, but the
parser gave them ordinary left-associative binding powers, so
`a..b..c` silently parsed as `(a..b)..c`. A range operand may not
itself be an unparenthesized range: the chain is now a parse error
("range operators cannot be chained; parenthesize one side"), while
`(a..b)..c` stays legal because the parenthesized form is explicit.
- Item modifiers were eaten before dispatch and never validated, so
`pub async struct`, `offchain mod`, `pub impl`, and `pub actor` all
parsed. Grammar places `offchain`/`async` only on fn_def, and omits
`pub` from impl_block, actor_def, onchain_mod/event_def, and
extern_block. Misplaced modifiers now error on the modifier's own
span, and the item still parses afterward for error recovery.
Adds four unit tests (chained-range rejection, single/parenthesized
range acceptance, and both directions of the modifier matrix), a
ranges_modifiers.sp corpus fixture, and the crates/AGENTS.md contract
updates.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3CinyyqWL6SfCLrKGm3ja
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 5/5
- In
crates/sploosh-parser/src/lib.rs, the main risk is test coverage depth: modifier-placement and range-chaining tests currently assert diagnostic text but not that errors stay anchored to the intended operator/modifier spans, so a future span regression could ship without failing tests. This looks safe to merge overall, but adding explicit span assertions forerror_at(token.span, ...)and the second../..=token before merging would de-risk the new diagnostics behavior.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Cubic review on PR #74: both tests validated message text only, while the PR promises span anchoring — the second range operator for chained ranges, the modifier's own token for misplaced modifiers. The tests now assert the error span alongside the message, so the anchoring cannot silently regress. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3CinyyqWL6SfCLrKGm3ja
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Adds parser diagnostics for range chaining and misplaced item modifiers, with tests. Low risk as it only rejects previously invalid syntax per the grammar.
Re-trigger cubic
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
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
The last two over-acceptances from the §16 review — closing out the correctness wave's planned scope:
docs/reference/operator-precedence.md) marks../..=assoc "None", but the parser gave them ordinary left-associative binding powers —a..b..csilently parsed as(a..b)..c. Chained ranges now error ("range operators cannot be chained; parenthesize one side") with the span on the second range operator.(a..b)..cstays legal: the parenthesized form is explicit, and the AST cannot (and should not) distinguish it after the fact.pub async struct S {},offchain mod m;,pub impl User {}, andpub actor A {}all parsed. Per the grammar,offchain/asyncprefix onlyfn_def, andpubis absent fromimpl_block,actor_def,onchain_mod/event_def, andextern_block. Misplaced modifiers now error on the modifier's own token span, and the item still parses afterward so error recovery stays useful.Stacked-PR note: targets the integration branch
fix/parser-correctness-base(PR #69) — sibling of #70/#72/#73. Shares the usual conflict points with the siblings (tests-module tail, corpus array, AGENTS.md accepted bullet; and #72 adds an identicalerror_athelper — keep one copy at merge).Related Issue
None (review-driven; part of the parser correctness wave tracked in PR #69).
Spec Sections Affected
None changed — implements the precedence table's "None" associativity row and §16's modifier placement as written.
Checklist
docs/pages (crates/AGENTS.md subset contract)Build Targets Tested
Test Plan
range_operators_cannot_chain(a..b..c,a..=b..c,a..b..=call rejected with the chaining message),single_and_parenthesized_ranges_parse(lo..hi,lo..=hi,(a..b)..caccepted),offchain_and_async_apply_only_to_fn_items(five rejected placements +pub offchain async fnaccepted),pub_rejected_where_grammar_omits_it(rejected on impl/actor/onchain/extern; accepted on struct/enum/trait/mod/use/const/type).tests/corpus/ranges_modifiers.spcovering the accepted side (ranges in expressions,pubdeclarations,pub offchain async fn).cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(21 tests) green locally and viascripts/docker-check.ps1 -Build(Ubuntu parity).Summary by cubic
Enforces non-associativity for
../..=and validates item modifier placement per §16 to close parser over-acceptances. Chained ranges now error, and misplacedpub,offchain, andasyncare rejected with clear, anchored diagnostics; tests assert the error spans.Bug Fixes
a..b..c; allow(a..b)..c.offchain/asynctofnitems.pubonimpl,actor,onchain, andextern.Migration
(a..b)..c.offchain/asynconly onfn, and removepubfromimpl/actor/onchain/extern.Written for commit 36c17f1. Summary will update on new commits.