parser: anchor incomplete-fn-header errors at the offending function (ILO-P020)#305
Merged
Conversation
When a function header is malformed (missing return type, missing `>`, etc.) and the line ends before the parser has what it needs, it used to walk across the unindented newline and consume tokens from the next function. The resulting error span landed on the wrong function, sending personas to bisect the wrong line. Record decl-boundary metadata in `Parser::new` (parallel to the filtered token stream, one entry per token slot), then check it at the two header pinch points in `parse_fn_decl` (after params, after the `>`) and at the top of `parse_params` so the param loop cannot drag the next function's name across the boundary. `parse_type` gets a safety net for nested type slots (`R` / `M` / `F` / `L` / `O` / `S`) that anchors its existing ILO-P007 at the previous token. The friendly diagnostic is a new ILO-P020 with a hint pointing at the `name params>type;body` shape and the indent-the-continuation workaround. Picked P020 not P019 because P017/P018/P019 are already documented in SPEC.md / ai.txt for the import system.
Registry entry covers the three header shapes that surface this: missing return type after `>`, missing `>` entirely, and the indent-the-continuation workaround. SKILL.md common-mistakes gets a matching line so agents who see the code from the json diag have a direct pointer to the fix.
Nine cases pin the regression and the symmetry: missing err-type, missing `>`, missing return-type-after-arrow, middle-of-three attribution, first-fn attribution, last-fn attribution with trailing newline, zero-param-fn variant, valid multi-fn parity, and valid indented continuation parity. The valid-program cases are the safety rail — the boundary checks must not reject well-formed input. Pure-EOF on the final function (`... main a:n>R`) is not covered: it falls back to ILO-P008 with `Span::UNKNOWN` (a pre-existing limitation unrelated to this fix). The trailing-newline case covers the same shape with a proper anchor.
The header-level boundary check in `check_fn_header_boundary` now treats EOF as a soft boundary alongside top-level decl boundaries, and `parse_type`'s safety net anchors EOF errors at `prev_span()` rather than the default `Span::UNKNOWN`. Same model as the mid-file boundary case: errors land on the last token of the offending function rather than line 1 col 1. `prev_span()` is the right anchor for EOF because the parser has already consumed the function name and at least the first malformed header token, so `prev_span` always points at the offending line. The existing parser unit test `eof_while_expecting_identifier` had been pinning the old shape (bare `f` yielded an EOF message); updated its assertion to accept the new ILO-P020 wording, which is strictly more informative for personas. EOF rendering as line 1 col 1 is infra-wide (every error site uses `peek_span()` which returns `Span::UNKNOWN` at EOF) but route-around in fn-header parsing is enough for the reported regression. A follow-up could replace the `peek_span()` fallback site-by-site.
Restore the last-fn case to assert real attribution (`main a:n>R` at EOF → line 3, not line 1), and add a sibling case for missing `>` at EOF asserting ILO-P020 specifically. Same shape as the mid-file boundary cases above but exercises the EOF branch of `check_fn_header_boundary` and the EOF guard in `parse_type`.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
Fixes a token-spend trap reported by qa-tester + devops-sre rerun3: a malformed function header in a multi-function file used to report its error span on the wrong function. Personas wasted session time bisecting which function actually had the bug.
The root cause is in
Parser::new: everyToken::Newlineis filtered out before parsing. Whenparse_fn_declreads a malformed header,parse_type/parse_paramshappily walked across the newline into the next function and emitted the error there.Manifesto framing: a correctly attributed span lands the persona on the right line first try instead of forcing a token-expensive bisect.
Repro (before → after)
ILO-P003: expected Greater, got Ident("main")at line 3 col 1 (the start ofmain).ILO-P007: expected type, got end of lineat line 2 col 8 (anchored onf2's line).ILO-P003at line 3 col 1.ILO-P020: incomplete function header for \f2`: header runs off the end of the lineat line 2 col 1, with a hint pointing at thename params>type;body` shape and the indent-the-continuation workaround.EOF cases (file ends mid-header) used to fall through to
Span::UNKNOWNand render as line 1 col 1. Now they anchor at the previous token (last token of the offending function's line) and surface ILO-P020 with the same friendly message.What's in the diff (per commit)
parser: anchor incomplete-header errors at the offending function— Parser records decl-boundary metadata inParser::new(parallelVec<Option<Span>>populated before newlines are filtered out). New helpercheck_fn_header_boundaryruns at the two header pinch points inparse_fn_decl(after params, after>).parse_paramsstops at a boundary so the loop can't drag the next function's name in as another param.parse_typegets a safety net for nested type slots that anchors its existing ILO-P007 atprev_span().diag: document ILO-P020 incomplete function header— registry entry with the three failure shapes (missing return type, missing>, missing err-type afterR) and the indent-the-continuation workaround. SKILL.md common-mistakes gets a matching entry as Fix unary negation -x parsing #12.test: cross-line attribution for parse errors in multi-fn files— initial 9 regression cases.parser: extend ILO-P020 to cover EOF mid-fn-header—check_fn_header_boundarytreats EOF as a soft boundary alongside decl boundaries.parse_typeEOF case anchors atprev_span()instead ofSpan::UNKNOWN. Existingeof_while_expecting_identifierparser unit test had its assertion updated to accept the strictly-better ILO-P020 wording.test: cover EOF mid-fn-header attribution— 1 additional case + restoration oferror_on_last_function_stays_on_last_functionto assert proper attribution (not the EOF-fallback shape it was relaxed to mid-development).ILO-P020 not P019 because P017/P018/P019 are already documented in SPEC.md/ai.txt for the import system. Pre-existing namespace mess; this PR doesn't touch it, just steps around.
Test plan
tests/regression_multi_fn_error_span.rs— 10 cases covering missing-err-type, missing->, missing-return-type-after-arrow, middle-of-three attribution, first-fn attribution, last-fn EOF attribution, last-fn missing->EOF attribution, zero-param-fn variant, valid multi-fn parity, valid indented continuation parity. All passing.cargo test --lib parser::) — 295/295 passing, including the updatedeof_while_expecting_identifier.cannot find libilo.a — Searched: target/release/libilo.abecause they hardcodetarget/and ignore myCARGO_TARGET_DIR=target-isolatedoverride. CI uses the default target dir and won't hit this — pre-existing infra constraint, not caused by this PR.Follow-ups
Span::UNKNOWN(line 1 col 1) is infra-wide — every error site usespeek_span()which returnsUNKNOWNat EOF. This PR routes around it for the fn-header case specifically. A follow-up could replacepeek_span()with aprev_span()-on-EOF fallback site by site.