Skip to content

fix(#1290): a parameter that shadows a file-scope name moves like a local - #1993

Merged
jlaustill merged 20 commits into
mainfrom
fix/1290-parameter-shadows-global
Oct 11, 2026
Merged

jlaustill merged 20 commits into
mainfrom
fix/1290-parameter-shadows-global

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A parameter that shadows a file-scope name now moves to <function>__<name>. This is the ADR-057 rule #1289 already applies to locals. Before this PR, global.total inside paramCase(u32 total) emitted as total, so it returned the parameter. Every place a parameter is written into C now reads its settled name: signature, prototype, callback typedef, body reads/writes, member access, and arguments. That also fixes #1994's pointer-held local.

// before
uint32_t S__paramCase(uint32_t total) { return total; }          // returns 42
// after (.c and .h both)
uint32_t S__paramCase(uint32_t S__paramCase__total) { return total; }   // returns 7

A parameter that shadows nothing keeps its source name.

Changes

  • LexicalFrames.emittedName: the parameter exemption is gone. 1.4 settles the parameter's C name once, on its ILocalDeclaration.
  • Program.withSettledParameters: copies each parameter's settled name onto IParameterInfo.emittedName. A frame is matched to its symbol by declaration span, so an E0425 duplicate still reaches E0425. The .h reads the name through IParameterSymbol.emittedName and the .c signature through FunctionContextManager.parameterEmittedName. Per-parameter facts (pass-by-value, modified, auto-const) stay keyed by the source name.
  • TranspileState.localCName: the one place a bound local's or parameter's body C name is decided. It records ADR-057 provenance when the name moves. emittedLocalNameAt and TypeValidator.resolveBoundIdentifier both call it.
    • parameterBindingAt returns IBoundParameter { info, emittedName }.
    • ParameterDereferenceResolver.resolve, SimpleIdentifierResolver (ISimpleIdentifierDeps.getParameter / resolveParameter) and the assignment-target path take that object.
  • ArgumentGenerator.handleIdentifierArg: passes a parameter on under its C name. Before, a moved parameter passed by its source name was the global it shadows, and in C++ the callee wrote that global.
  • PostfixExpressionGenerator: IMemberAccessContext.isRootOp is decided by position (op index 0, like the write path's isFirstOp). tryStructParamAccess and tryBitmapFieldAccess used to compare the C name with the source name, which broke once the root moved.
  • CodeGenWalker._buildPostfixChainDeps: the write path gets the root's holding from the parameter bound by the source name, and only for a bare root. Before, it used firstId (the C name), so S__f__pt.x = ... took ..
  • New ScopeFunctions.adopt: the in-place scope.functions rewrite DeferredTypes did, shared. Unit-tested in ScopeFunctions.test.ts.
  • Docs:

Pre-Submit Checklist

Code Quality

  • Code is properly formatted (pre-commit hook)
  • No oxlint errors in modified files (npx oxlint src/TRANSPILE src/PARSE/4-Resolve: 0 warnings, 0 errors)
  • TypeScript compiles without errors (npx tsc --noEmit)
  • All tests pass (npm test). Left to CI; see Testing.

Documentation

  • README.md updated (if feature-visible): n/a
  • ADR updated with implementation details (ADR-057)
  • learn-cnext-in-y-minutes.md updated (if syntax changed): n/a, no syntax change
  • Test README created in tests/[feature]/README.md: n/a, tests/bugs/ fixture

Git Hygiene

  • Only related files included
  • No debug code left in source
  • Commit messages are descriptive

Testing

Related Issues

Fixes #1290
Fixes #1994
Fixes #2001
Fixes #2003
Fixes #2004
Fixes #2010
Fixes #2011
Related to ADR-057, #1289, #1934

Second review

  • Whether an op applies to the chain's root is decided once, as IChainHead.rootOp. The read path's planned member ops and the write path's IPostfixOperation both carry it as appliesToRoot, so neither counts positions on its own.
  • Callback typedefs write the settled name, e.g. typedef void (*handler_fp)(uint32_t, uint32_t handler__buf[2]); (tests/bugs/issue-1290-parameter-shadows-global/callback-typedef).
  • Missing MISRA 5.3 gate tracked in MISRA C:2012 Rule 5.3 (inner scope hides outer) is not gated on generated C #1998.

Third review

Fourth review

#2003 box 1: statement children come from the lowered tree only

Joshua chose to make the box literally true: one place knows each statement's children by role.

  • StatementLowering (1.2) is the only code that reads a statement's children from the parse tree. Its TStatement names them by role (if.whenTrue/whenFalse, loop body, switch.cases[].body/defaultCase, block statements).
  • ReturnPathAnalyzer.analyze(program: IProgramSyntax) reads those role fields instead of Parser.ProgramContext. IFunctionDeclarationSyntax.nameSpan carries the span its diagnostic needs.
  • New StatementChildren.of(TStatement): the direct nested statements, in source order, with an exhaustive switch (a new statement kind is a compile error until it is placed). for init/update are not children; the two walkers that need them read them explicitly.
  • Walkers moved onto it: CallbackUseCollector, ModificationCollector (both now take the lowered IProgramSyntax; CNextResolver.resolve receives parsed.program), InitializationAnalyzer's scope-member pre-scan, and CodeGenWalker's callback-type pre-pass.
  • Deleted (no production user left): ChildStatementCollector, StatementExpressionCollector, AssignmentTargetExtractor and their tests, ExpressionUtils.collectUnaryFromOrExpr/collectAdditiveFromOrExprs, and their .dependency-cruiser.cjs PARSE_TREE_HELPERS entry. docs/architecture/parse-tree-sites.md regenerated: 96 -> 90 modules hold a parse tree.
  • Generated C is unchanged by this step (no fixture snapshot changes).
  • Tests: StatementChildren.test.ts (10), new ModificationCollector.test.ts (7: parameter lists, nested writes, qualified writes, calls in for headers, scope-qualified/this./cast calls, nested call arguments recorded once). ReturnPathAnalyzer/CallbackUseCollector/InitializationAnalyzer tests build their input with ProgramLowering.program(...).
  • Mutations, each restored afterwards:
    • each StatementChildren arm returning [] (if, while/for, doWhile/forever/critical, switch, block): 2, 2, 3, 1, 1 of its unit tests fail.
    • forever/critical returning []: all 4 issue-2003-forever-critical-children fixtures fail.
    • ReturnPathAnalyzer without its forever arm: 2 tests fail.
    • each consumer ignoring StatementChildren.of: CallbackUseCollector 1 unit test + callback-forever; ModificationCollector 1 unit test + parameter-write-forever; InitializationAnalyzer 2 unit tests + both this-write-*; CodeGenWalker both issue-1484-callback-local-nested fixtures.
  • Local at 14d9241c9: tsc, oxlint, knip (unused-code:check), depcruise, cspell, parse-tree:check, scope-joins:check, coverage:matrix:check, diagnostics:manifest:check, typecheck:scripts clean; vitest over every src/ dir and scripts/ passes; A parameter shadowing a global makes global.X return the parameter — silently wrong C, exit 0 #1290, A global.X assignment marks a same-named shadowing local as initialized, so E0381 never fires (exit 0) #2001, Three statement walkers skip forever (one also skips critical): a parameter written there is emitted const, a callback registered there keeps a by-value parameter; gcc rejects both, transpiler exits 0 #2003, E0381 binds names on its own scope stack, which never declares a for-header variable: a loop variable that shadows a scope member reads as the member (false E0381, exit 1) #2004 fixture dirs pass. CI on 14d9241c9: 16 passed, 0 failed. SonarCloud: 0 open issues (issues API).

Fifth review

Link to Devin session: https://app.devin.ai/sessions/e67b3642a2bc4254921c59a4b247eb59
Open in Devin Desktop: https://app.devin.ai/desktop/session/e67b3642a2bc4254921c59a4b247eb59?variant=devin
Requested by: @jlaustill

…ocal

ADR-057: 1.4 settles a shadowing parameter's C name as <function>__<name>,
and the .c signature, the .h prototype and every use in the body read that
one answer, so global.X no longer binds the parameter. ScopeFunctions.adopt
shares DeferredTypes' in-place scope.functions rewrite so the registry's
scope holds the settled function. TEMPLATE.md: the matrix axes cannot see
declaration kind.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

process(u32 process) shadows the function name, a file-scope name, so the
parameter moves to process__process as a local of that name already does.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

@jlaustill jlaustill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

PR: #1993: fix(#1290): a parameter that shadows a file-scope name moves like a local
Reviewed by: Claude (automated review)
Head: f785d4ae. CI: All Checks Passed success at 2026-10-11T13:28:06Z (the earlier failure was the cancelled superseded run). Sonar on PR 1993: 0 open issues, gate OK, new coverage 96.8%.

Overview

The decision is in the right place. 1.4 settles a parameter's C name once on its frame and stamps it on IParameterInfo, so the .h prototype and the .c signature read one answer. The ADR-057 rule now covers parameters, which the ADR requires.

The problem is downstream. Before this PR, a parameter's emitted name always equalled its source spelling, so every render path that emits or compares the source spelling was right by coincidence. This PR breaks that coincidence and finds only three of those paths. I found three more by probing struct, string, bitmap and pass-on parameters. Each one reproduces on this head and is correct on main when global. is not used. So this PR regresses programs that main compiles correctly: any parameter that shadows a global, even with no global. anywhere.

Findings

Severity Count
Critical 2
Important 1
Moderate 2
Nit 0

Key Issues

  1. [critical] Silent miscompile: a shadowing parameter passed to another C-Next function is passed as the global. ArgumentGenerator.handleIdentifierArg returns id, not emitted. Execution repros below: a string returns the wrong length with no warning, C++ writes the global through a reference, C segfaults.
  2. [critical] Non-compiling C: a shadowing struct parameter gets . for member read and write. A shadowing bitmap parameter's field read shifts the pointer. Cause: PostfixExpressionGenerator decides "is this the root" by ctx.result === ctx.rootIdentifier, which compares the emitted name with the source spelling. The same comparison already breaks pointer-held locals on main. I filed that as #1994 with a minimal repro and a control. This PR touches the same check, so it owns that fix: the structural fix covers both, and the PR can say Fixes #1994.
  3. [important] One decision, several derivations: the PR adds three call sites that each work out a parameter's C name with emittedLocalNameAt, and the argument path needs a fourth. TypeValidator.resolveBoundIdentifier makes the same binding→C-name decision for locals and records ADR-057 provenance. emittedLocalNameAt records nothing. As a result, both new fixtures are listed in docs/scope-context-matrix.md under "linked fixtures with no derivable context": they occupy no ADR-057 cell, so the cross-file fixture's mutation check reddens no transitive cell.
  4. [moderate] The fixture tests only u32 scalars and a u32 array. Struct, string, bitmap and pass-on parameters each render through their own ADR-006 path, and all three regressions above are in those paths.
  5. [moderate] ScopeFunctions.adopt was extracted but has no unit test of its own.

What Looks Good

  • The C name is decided in 1.4, on the frame, and both the .h and the .c read it, so the prototype and the definition cannot disagree.
  • ScopeFunctions.adopt removes the copy of the in-place scope.functions rewrite that withSettledParameters would otherwise have needed.
  • The plainCase(u32 fresh) negative control, and a mutation check that reverts the exemption and reddens both fixtures.

🤖 Automated review by Claude Code

Comment thread src/cnx/PARSE/4-Resolve/LexicalFrames.ts
Comment thread src/TRANSPILE/CodeGenWalker.ts Outdated
Comment thread src/TRANSPILE/CodeGenWalker.ts Outdated
Comment thread src/cnx/PARSE/4-Resolve/ScopeFunctions.ts
devin-ai-integration Bot and others added 2 commits October 11, 2026 13:50
…mber by position (Fixes #1994)

- ArgumentGenerator passes a parameter on under its settled C name: a moved
  parameter passed as written was the global it shadows (C++ wrote it).
- TranspileState.localCName is the one place a bound local's or parameter's
  body C name is decided, and records ADR-057 provenance when it moves;
  parameterBindingAt carries the parameter with that name to every site.
- PostfixExpressionGenerator decides 'member of the root' by position, as
  the write path's isFirstOp does, not by comparing the C name with the
  name as written (struct/bitmap parameters, #1994's pointer-held local).
- The write path's root holding is bound by the name as written, so a moved
  struct parameter's member write takes '->' and global.pt.x does not.
- Unit test for ScopeFunctions.adopt; fixtures for struct, string, bitmap,
  by-reference pass-on parameters and #1994's pointer-held local.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
…eads

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

@jlaustill jlaustill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

PR: #1993: fix(#1290): a parameter that shadows a file-scope name moves like a local
Reviewed by: Claude (automated review)

Overview

This moves a parameter that shadows a file-scope name to <function>__<name>, as #1289 does for locals. 1.4 decides the name once (LexicalFrames). The .c signature, the .h prototype and the body all read that decision. The approach is sound and I found no correctness bug in the changed code. The notes are about one decision still made in two places, one positional pairing worth asserting, and two places where the PR's claims run ahead of the code.

Verification I ran (head a61ed85a5, base 0cd96994b)

  • CI: 16 of 17 check-runs on a61ed85a517d concluded success, and Deploy Coverage was skipped (gh api .../commits/<sha>/check-runs). Sonar: total 0 open issues for pullRequest=1993, gate OK, new coverage 95.1%.
  • Mutations: reverting the struct root check to text fails parameter-kinds and pointer-local-shadow. Reverting the bitmap check fails parameter-kinds. Both match the PR's claims. Removing the new ScopeFunctions.adopt call in withSettledParameters fails Program.test.ts > the copy a scope holds > is the SAME object the file's symbol list holds, which passes 52/52 without the mutation. So that call is guarded too.
  • Extra probes, all correct: a callback parameter that shadows a global function, called both bare and with global.. A string<N> parameter written with strncpy and measured. Bit-range and compound writes through a by-reference parameter. A shadowing parameter as an array index, a switch subject and a ternary operand. A private scope function (no stray prototype). A duplicate function, which still reaches E0425 as claimed.

Findings

Severity Count
Critical 0
Important 1
Moderate 1
Nit 4

Key Issues

  • [important] Fixes #1994 would close #1994 with its first box unmet. The read path decides "is this the root" by position, and the write path still decides it in its own loop (PostfixChainBuilder.isFirstOp). That is decided twice, not once.
  • [moderate] Frame and symbol parameters are paired by index, but the invariant checks only the count.

PR description accuracy

  • The body says the ADR-057 edit was "approved in the issue thread". #1290's only comment (gh api --paginate repos/jlaustill/c-next/issues/1290/comments: 1 comment, 2026-10-11T11:37:18Z) is the bot's "Waiting on plan approval before changing the transpiler." There is no approval on the thread. ADR-057 already promised that global. sees past a shadow, so under CLAUDE.md this edit documents a codegen fix and may not have needed approval. Even so, the description shouldn't cite an approval the thread doesn't hold. Link where it was given, or reword.
  • "Every place a parameter is written into C now reads its settled name": the callback typedef doesn't (inline nit).

Not caused by this PR, found while probing

  • u32 total <- total + 1; in a block inside f(u32 total) emits uint32_t f__total = cnx_clamp_add_u32(f__total, 1U);, a read of uninitialized memory (exit 1). main emits the same self-read under the old parameter name, and also for u32 x <- x + 1; with no global involved. Already tracked by #1643.
  • MISRA C:2012 Rule 5.3 is not checked at all. The cppcheck 2.13 misra.py that validate:c runs has no misra_5_3 (it defines misra_5_1, 5_2, 5_4…5_9), and an obvious inner-hides-outer sanity case reports nothing. So a block-local that re-shadows a moved parameter (both become f__total, as nested locals already do) gets through ungated. I found no issue tracking this. Per the "fix it or file it" rule it needs its own issue.

What Looks Good

  • One decision, three readers. LexicalFrames settles the name once, and the .h prototype now matches the .c definition, as MISRA 8.3 requires of declarations. The PR also removes a whole class of Rule 5.3 violations (a parameter hiding a global) from generated C.
  • Fixtures that can't pass by coincidence: distinct values, execution tests in both C and C++, a negative control (plainCase(u32 fresh)), and a cross-file case.
  • IBoundParameter makes it structurally hard for a call site to hold a parameter without its C name, which is the right fix for the id/emitted confusion. ScopeFunctions extracts the DeferredTypes special case into a tested helper.

🤖 Automated review by Claude Code

Comment thread src/PARSE/4-Resolve/Program.ts Outdated
Comment thread src/PARSE/4-Resolve/Program.ts Outdated
Comment thread src/TRANSPILE/3-Render/codegen/helpers/ParameterDereferenceResolver.ts Outdated
Comment thread src/cnx/types/symbols/IParameterInfo.ts
devin-ai-integration Bot and others added 2 commits October 11, 2026 15:13
… names a parameter as its prototype does

- IChainHead.rootOp is the one decision; the read path's planned member ops
  and the write path's IPostfixOperation both carry it as appliesToRoot, so
  neither re-counts positions (#1994 box 1)
- callback typedefs write the settled parameter C name
- Program.ts: frames invariant and per-pair source-name invariant
- ParameterDereferenceResolver: emittedName alias, JSDoc
- test-adr: 057 on the #1290 and #1994 fixtures; ADR-057 references #1994

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
….4, which settles parameter C names

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

@jlaustill jlaustill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

PR: #1993: fix(#1290): a parameter that shadows a file-scope name moves like a local
Reviewed by: Claude (automated review), round 3, head d9809012

Overview

Both earlier rounds' findings are resolved on this head. "Applies to the root" is decided once, on IChainHead.rootOp. The argument path passes the C name. The callback typedef writes the settled name. The frame/symbol pairing asserts names per index. I re-checked each one and found no remaining issue in the changed render code. One defect remains in #1290's own construct, in a pass this PR does not touch: 1.3 Declare's modification analysis still binds global.<name> <- to the shadowing parameter by spelling (inline).

Verification I ran (head d9809012, base 0cd96994b, local clone)

  • npx tsc --noEmit clean. npm run unit: 428 files / 8496 tests pass. npm run test:q: 1620/1620, and the working tree was clean afterwards (no snapshot drift).
  • CI (gh api …/commits/d9809012…/check-runs, read 15:58Z): 15 runs success, Deploy Coverage skipped, and C Static Analysis (misra) still in_progress. The previous head ee2b8205 failed Unit Tests with 1.4 settled the C name of aPoke's parameter d, which is what d9809012 addresses. Sonar analysed d9809012 (SonarCloud Code Analysis success at 15:55:29Z): total 0 open issues for pullRequest=1993, gate OK, new coverage 96.4%.
  • Probes on this head, all correct:
    • .char_count read twice on a shadowing string<16> parameter: the length cache keys by the rendered C name.
    • bump(global.total) in a shadowing function: the collector's call-edge arm already excludes global..
    • An ADR-029 callback-type function whose parameter shadows a global: body rename and global. write.
    • global.pt.y read/write beside a struct parameter pt: rootIdentifier is undefined for a global. root, so rootParameter is too.
  • Cache: only C/C++ symbols are written to the cache (CachedSymbolReader rejects C-Next ones), so a stale IParameterInfo without emittedName cannot reach render.

Findings

Severity Count
Critical 0
Important 1
Moderate 0
Nit 1

Key Issues

  • [important] global.total <- … (and this.count <- …) marks the shadowing parameter as modified. It becomes uint32_t* / uint32_t& and Point* instead of by-value / const Point*. In C++, f(5) then does not compile. Cause and a measured one-line fix are inline.
  • [nit] PR description: the Changes list says docs/scope-context-matrix.md "is regenerated", but that file is not in the diff. It came back to main's content once the fixtures landed in already-occupied cells (your ee2b8205 reply says so). The Testing section's "355 files / 7426 tests pass" is also stale: this head runs 428 / 8496. Please carry the SHA a count came from, or drop the count.

Filed while probing (not this PR's code; each reproduces on main with no shadowing)

  • #1999: an ADR-029 callback-type function that writes its own primitive parameter is emitted as uint32_t* against typedef void (*handler_fp)(uint32_t). C segfaults at exit 0, and C++ rejects it. ADR-029 promises a matching typedef; which body-independent shape it should take is an open question for the owner.
  • #2000: a write to a block-local that shadows a parameter is recorded against the parameter. This is the lexical-scope arm of the inline finding; it needs block-declaration tracking, not the grammar alternative.

What Looks Good

  • One root decision: IChainHead.rootOp, carried as appliesToRoot by identity on both the planned read ops and IPostfixOperation. Neither path counts positions any more. A global./this. chain's root op is ops[1], and its root binds no parameter.
  • IBoundParameter: the C name travels with the parameter, so ParameterDereferenceResolver and SimpleIdentifierResolver cannot write the spelling. localCName is the single site that records ADR-057 provenance, which is why the fixtures now occupy matrix cells.
  • One 1.4 answer for the .c, .h and _fp typedef: the signature, prototype and typedef all read the parameter name 1.4 copied from its frame, asserted per pair by name. The d9809012 test change makes the opaque-callback case go through 1.4 rather than relaxing the invariant.

🤖 Automated review by Claude Code

…rameter modified

A shadowing parameter whose global the body writes through `global.` was
counted as modified by spelling, so it went by pointer/reference and lost
auto-const; in C++ a literal argument then did not compile.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
devin-ai-integration Bot and others added 2 commits October 11, 2026 16:57
The frames lookup reuses framesOf's one assertion instead of a second copy.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
A qualified assignment target names a file-scope variable or a scope member,
never a local. The scope-member pre-scan likewise skips global. targets and
bare writes a local or parameter binds. Fixtures in C and C++, each arm
mutation-checked.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

@jlaustill jlaustill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

PR: #1993: fix(#1290): a parameter that shadows a file-scope name moves like a local
Reviewed by: Claude (automated review), round 4, head bd4f034d

Overview

Round 3's finding is fixed: a global./this. assignment target no longer marks a parameter as modified. The new global-write-past-parameter fixture can fail. Its C++ cell compiles only if the parameter is by value. This round reviews the three commits since d9809012. The new #2001 fix in InitializationAnalyzer introduces a regression: valid code with a this. write inside critical/forever is now rejected. It also leaves one binding question answered two ways in a single analyzer. Both point to pre-existing defects, filed as #2003 and #2004. Each belongs to this PR for the reason given inline.

Verification I ran (local clone at bd4f034d, base 0cd96994b)

  • CI (gh api …/commits/bd4f034d…/check-runs, read 18:19Z): every check's latest run is success and Deploy Coverage is skipped. The one failure is the superseded run's All Checks Passed (17:20:51Z); that check's rerun succeeded at 17:29:04Z. Sonar: total 0 open issues for pullRequest=1993, gate OK.
  • Probes on this head vs main (npx tsx src/cli/index.ts … --target host):
    • this.x writes inside critical/forever: main accepts, this head reports E0381. This is a regression (inline).
    • for-variable write shadowing a member: the result depends on declaration order (inline).
    • Parameter shadowing a scope member: main misses the uninitialized member, this head catches it, but no fixture covers it (inline).
    • Bare write inside critical: correct on both.
  • Prototype of the walker fix (inline, +19 −123): npm run unit 428 files / 8505 tests, npm run test:q 1626/1626, no snapshot change.

Findings

Severity Count
Critical 0
Important 2
Moderate 1
Nit 0

Key Issues

  • [important] Regression: a this.x write inside critical { } or forever { } is now invisible to E0381. The listener's new early return relies on the #1019 pre-scan, and that pre-scan's hand-rolled walker skips both kinds. The shared ChildStatementCollector and CallbackUseCollector skip forever too, and on main that gives uncompilable C. #2003 has the reproductions and the measured one-walker fix.
  • [important] One binding question, two answers: the pre-scan asks 1.4 (lexicalDeclarationAt), and the listener asks its own ScopeStack. They disagree on a for-header variable, and the result then depends on declaration order. #2004 has the reproductions. IProgram.bindValue is the existing single decision, and it also covers the qualified-target tests.
  • [moderate] Test gap: a parameter that shadows a scope member, written bare (#1290's construct in #2001's arm), has no fixture.

Filed while probing

  • #2003: three statement walkers skip forever (one also skips critical). Reproductions on main: a struct parameter written in forever is emitted const (C and C++ reject it); a callback registered in forever keeps a by-value parameter (gcc -Werror rejects it); and the critical member write above.
  • #2004: E0381's own scope stack never declares a for-header variable. That gives a false E0381 on main, and on this head an order-dependent result for the write arm.

What Looks Good

  • Round 3's fix landed with a guard that can fail: global-write-past-parameter passes literal arguments, so its C++ cell compiles only while the parameter stays by value. The u32, struct and this. cases each have a control next to them, and the spelling mutation fails the fixture.
  • Invariants now have tests: Program.test.ts checks the frame/symbol pairing by count and by name, with a control, and FunctionContextManager.test.ts covers parameterEmittedName, including an unsettled parameter and an out-of-range index. withSettledParameters reuses framesOf's assertion rather than keeping a second copy.
  • The "check everywhere" sweep found #2001, and it was fixed with error fixtures that each name the case to stay silent, plus an execution control.

🤖 Automated review by Claude Code

Comment thread src/TRANSPILE/1-Analyze/InitializationAnalyzer.ts Outdated
Comment thread src/TRANSPILE/1-Analyze/InitializationAnalyzer.ts Outdated
devin-ai-integration Bot and others added 2 commits October 11, 2026 18:45
…ker covers forever

- ChildStatementCollector gains a forever arm; CallbackUseCollector and the
  InitializationAnalyzer scope-member pre-scan take their children from it
  instead of recursing by hand (they skipped forever, the pre-scan critical too)
- InitializationAnalyzer keys state by what IProgram.bindValue binds a spelling
  to, from the target's own root; its ScopeStack (now unused) is deleted and
  it no longer declares parameters itself
- fixtures: forever/critical children (#2003), for-variable binding (#2004),
  a parameter that shadows a scope member (#2001)

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
… round-4 fixtures

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
devin-ai-integration Bot and others added 3 commits October 11, 2026 19:09
…s a use; restore JSDoc

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
ReturnPathAnalyzer reads TStatement roles (whenTrue/whenFalse, case/default
bodies) from 1.2's lowering instead of the parse tree. CallbackUseCollector,
ModificationCollector, InitializationAnalyzer's scope-member scan and
CodeGenWalker's callback-type scan share StatementChildren.of, which has an
arm per statement kind. ChildStatementCollector, StatementExpressionCollector
and AssignmentTargetExtractor are deleted; five fewer modules hold a parse
tree. A nested call's argument is no longer recorded twice in the call graph.

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
…ionUtils helper

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

@jlaustill jlaustill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

PR: #1993: fix(#1290): a parameter that shadows a file-scope name moves like a local
Reviewed by: Claude (automated review), round 5, head 14d9241c9

Overview

Round 4's three findings are resolved. #2003 and #2004 are fixed with fixtures, and parameter-write-scope-member covers a parameter that shadows a scope member. This round reviews the five commits since bd4f034d: statement children from the lowered tree, InitializationAnalyzer on bindValue, and the deleted parse-tree helpers. I found no regression on valid code. Two pre-existing defects sit in code this PR rewrites, and the PR owns both (inline). #2003's first box is ticked on a claim that a grep disproves.

Verification I ran (scratch clone at 14d9241c9, base 0cd96994b)

  • npm run build and npx tsc --noEmit clean. npm run unit: 426 files / 8447 tests pass. npm run test:q: 1634/1634, and git status was clean afterwards (no snapshot drift).
  • CI (gh api …/commits/14d9241c9…/check-runs, read 20:37Z): both runs on this head are success on every check. Deploy Coverage is skipped. The latest run is All Checks Passed at 20:33:23Z. Sonar: SonarCloud Code Analysis success at 20:35:03Z, total 0 open issues for pullRequest=1993, gate OK.
  • Probes, main (7d7f27158) vs this head, npx tsx src/cli/index.ts … --target host:
    • E0704 on a scope method, and a non-void function whose body is critical { return 3; }: identical diagnostics and positions.
    • framedDeclaration's invariant: locals declared in a switch case, default, do and a bare block all transpile on both.
    • A nested-block local shadowing a parameter that shadows a global: correct on this head, and it executes under -Wall -Wextra -Werror.
    • A write in a for header: the same wrong output on both (inline, #2010).
    • A body local that redeclares a parameter: compiles on main, and gcc rejects this head's output (inline, #2011).

Findings

Severity Count
Critical 0
Important 3
Moderate 2
Nit 1

Key Issues

  • [important] #2010: a parameter written only in a for header becomes const Point* (gcc rejects the write) or by-value (the caller loses the write, exit 2). The #1019 pre-scan misses the same writes and reports a false E0381. Both walkers are rewritten here, and StatementChildren is what leaves the header out.
  • [important] #2011: with the parameter exemption gone, u32 f(u32 total) { u32 total <- 3; … } gives both declarations f__total, and gcc rejects the C at exit 0. main compiled it. The missing redeclaration diagnostic is the root defect, and one check covers all three shapes.
  • [important] #2003 box 1: NullCheckAnalyzer and InitializationAnalyzer's listener still read an if's or while's children from the parse tree. BracedBodyAnalyzer re-lists the children itself. The box is not true yet under either reading.
  • [moderate] CLAUDE.md is stale (not in the diff, so no inline comment): "Const Inference" (lines 1014–1018) still names ModificationCollector.walkStatementForModifications(), StatementExpressionCollector.collectAll() and ChildStatementCollector.collectAll(), and says to "Update both when adding new statement types". This PR deletes all three. The section should name StatementChildren.of and ModificationCollector.expressionsOf.
  • [moderate] Three switches over TStatement end in default and skip a new kind without a compile error (inline).

Filed while probing (each reproduces on main)

  • #2010: a write in a for header is invisible to 1.3's parameter-modification walk and to the #1019 member pre-scan. #1726 covers only the 2.1 listeners.
  • #2011: a name declared twice in one scope gets no diagnostic: two locals in one block, or a body local named like a parameter.

What Looks Good

  • StatementChildren.of ends in a never arm. Each arm's mutation was checked against both unit tests and fixtures, and the forever/critical mutation fails all four #2003 fixtures.
  • InitializationAnalyzer keeps state by the declaration 1.4 framed. Writes bind through ChainRoot.ofTarget. The line-only mutation, which fails 21 unit tests and a fixture, shows the span match is needed.
  • Three parse-tree helpers and ScopeStack are deleted along with their tests and the depcruise entry, and parse-tree-sites.md is regenerated (96 → 90) in the same commit.

🤖 Automated review by Claude Code

Comment thread src/cnx/PARSE/3-Declare/cnext/collectors/ModificationCollector.ts
Comment thread src/cnx/PARSE/4-Resolve/LexicalFrames.ts
Comment thread src/cnx/utils/StatementChildren.ts
Comment thread src/TRANSPILE/1-Analyze/ReturnPathAnalyzer.ts Outdated
Comment thread src/cnx/TRANSPILE/1-Analyze/InitializationAnalyzer.ts
devin-ai-integration Bot and others added 6 commits October 11, 2026 20:42
…1019 pre-scan

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
…tree; exhaustive statement switches

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
…no longer stores one

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>
…adows-global

Co-Authored-By: Joshua Austill <jlaustill@gmail.com>

# Conflicts:
#	.dependency-cruiser.cjs
#	CLAUDE.md
#	docs/architecture/parse-tree-sites.md
#	scripts/__tests__/artifact-lifetime.test.ts
#	src/cli/__tests__/cnx/TRANSPILE/1-Analyze/InitializationAnalyzer.test.ts
#	src/cli/__tests__/cnx/TRANSPILE/1-Analyze/testAnalysisContextFor.ts
#	src/cli/__tests__/cnx/TRANSPILE/CodeGenWalker.test.ts
#	src/cli/__tests__/cnx/TRANSPILE/ProgramGeneration.ts
#	src/cli/__tests__/cnx/TRANSPILE/TranspileState.test.ts
#	src/cnx/PARSE/3-Declare/cnext/__tests__/ModificationCollector.test.ts
#	src/cnx/PARSE/4-Resolve/ScopeFunctions.ts
#	src/cnx/PARSE/4-Resolve/__tests__/ScopeFunctions.test.ts
#	src/cnx/TRANSPILE/1-Analyze/RedeclarationAnalyzer.ts
#	src/cnx/TRANSPILE/1-Analyze/ScopeStack.ts
#	src/cnx/TRANSPILE/1-Analyze/__tests__/ScopeStack.test.ts
#	src/cnx/TRANSPILE/1-Analyze/helpers/__tests__/AssignmentTargetExtractor.test.ts
#	src/cnx/TRANSPILE/1-Analyze/helpers/__tests__/ChildStatementCollector.test.ts
#	src/cnx/TRANSPILE/1-Analyze/helpers/__tests__/StatementExpressionCollector.test.ts
#	src/cnx/TRANSPILE/1-Analyze/types/IRedeclarationError.ts
#	src/cnx/TRANSPILE/types/IBoundParameter.ts
#	src/cnx/utils/LoweredStatements.ts
#	src/cnx/utils/StatementChildren.ts
#	src/cnx/utils/StatementWrites.ts
#	src/cnx/utils/__tests__/LoweredStatements.test.ts
#	src/cnx/utils/__tests__/StatementChildren.test.ts
#	src/cnx/utils/__tests__/StatementWrites.test.ts
#	src/cnx/utils/ast/AssignmentTargetExtractor.ts
#	src/cnx/utils/ast/ChildStatementCollector.ts
#	src/cnx/utils/ast/StatementExpressionCollector.ts
@jlaustill
jlaustill enabled auto-merge October 11, 2026 22:11
@jlaustill
jlaustill merged commit f94c979 into main Oct 11, 2026
17 checks passed
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment