Fix spurious error on non-ident-headed type arguments#82
Merged
Conversation
`type_args_after_open` probed for an associated-type binding by calling `ident()` and backtracking on failure — but `ident()` records an "expected identifier" diagnostic before the backtrack, and the checkpoint only rewound the position, never the error list. The fallback type parse then succeeded while the stale error survived, so spec-legal arguments like `Vec<&str>`, `Vec<(i64, i64)>`, and `Vec<[u8; 4]>` were rejected. The binding now commits by peeking `IDENT "="` before consuming anything, the same shape as the parser's other lookaheads (turbofish, send-statement head), so no failed probe ever runs. A genuinely bad binding value (`Item = <garbage>`) now recovers to the next comma or `>` instead of re-parsing the binding name as a type argument. Closes #78. The probe cases from the issue land as regression tests, plus a corpus fixture for non-ident-headed generic arguments.
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Input as Token Stream
participant Parser as type_args_after_open()
participant Errors as Error List
participant Output as Vec<TypeArg>
Note over Parser: Loop over type arguments until '>' or EOF
loop for each type arg
Parser->>Input: peek current token
Input-->>Parser: TokenKind
Parser->>Input: peek next token (peek_kind_at(1))
Input-->>Parser: TokenKind or None
alt IDENT followed by '=' (NEW lookahead – no probe)
Parser->>Input: consume IDENT
Parser->>Input: consume '='
Parser->>Input: parse type()
Input-->>Parser: Ty
Parser->>Parser: push TypeArg::Assoc { name, ty }
else else – direct type parse
Parser->>Input: parse type()
alt type parsed successfully
Input-->>Parser: Ty
Parser->>Parser: push TypeArg::Type(ty)
else type parse fails
Parser->>Parser: recover_until(Comma, Gt)
Note over Parser: No stale error from abandoned ident() probe
end
end
Parser->>Input: try consume comma
alt comma consumed
Parser->>Parser: continue loop
else no comma
Parser->>Parser: break
end
end
Parser-->>Output: list of TypeArg
Auto-approved: Fix #78: replace probe-and-backtrack with lookahead in type_args_after_open to avoid stale errors on non-ident-headed type args like Vec<&str>. Adds tests.
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
Vec<&str>,Vec<(i64, i64)>,Vec<[u8; 4]>, andMap<String, Vec<&str>>were rejected with a spurious "expected identifier" even though they are legal per §16 (type_arg = type | IDENT "=" type).type_args_after_opencalledident()and backtracked by resettingself.pos— butident()pushes its failure diagnostic intoself.errorsfirst, and the checkpoint never truncated the error list. The fallback type parse succeeded while the stale error survived, soparse_programreturnedErr.IDENT "="before consuming anything — the same lookahead shape asturbofishand the send-statement head — so no failed probe ever runs. This also improves recovery for a genuinely bad binding value (Item = <garbage>now recovers to the next,/>instead of re-parsingItemas a type argument).self.pos = checkpointappears nowhere else), so no other production has this failure mode.Related Issue
Closes #78. Found during the enhancement wave (PR #71) while designing the attribute-argument parser; deliberately split out because it is a behavior change.
Spec Sections Affected
None — the spec and
docs/reference/grammar.mdalready say these shapes are legal; this makes the parser match them.Checklist
docs/pages — N/A; crates/AGENTS.md's accepted-subset list already claimed these shapes, the fix makes reality match itBuild Targets Tested
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspaceall green locally (56 tests, 14 corpus fixtures).Test Plan
non_ident_headed_type_args_parse— the four probe cases from Parser: spurious 'expected identifier' for non-ident-headed type args (Vec<&str>, tuples, arrays) #78, plus a structural assertion thatVec<&str>'s argument survives asTypeArg::Type(Type::Reference { .. })wrappingstr.assoc_binding_with_non_ident_headed_value_parses—Iter<Item = Vec<&str>>keeps working through the new lookahead, asserted down to theTypeArg::Assocshape.tests/corpus/generic_args.spfixture covers the shapes end-to-end via the auto-discovering corpus harness.dyn_trait_type_args_with_assoc_binding) and the full suite pass unchanged.Summary by cubic
Fixes a false "expected identifier" error when parsing generic type arguments that don’t start with an identifier. Uses
IDENT '='lookahead for associated-type bindings, so valid shapes likeVec<&str>now parse correctly (closes #78).type_args_after_openwithIDENT '='lookahead to avoid stale diagnostics and improve recovery to the next comma or '>' on bad bindings.tests/corpus/generic_args.sp.Written for commit fcc29f0. Summary will update on new commits.