Skip to content

fix(scanner): loop Pass B to a fixed point, closing issue #18#23

Merged
flaglint merged 1 commit into
mainfrom
fix/pass-b-fixed-point
Jul 7, 2026
Merged

fix(scanner): loop Pass B to a fixed point, closing issue #18#23
flaglint merged 1 commit into
mainfrom
fix/pass-b-fixed-point

Conversation

@flaglint

@flaglint flaglint commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #18. Pass B (composite-literal field binding) ran as a single forward sweep over parsed files, not a fixed-point iteration — a theoretical order-dependence risk flagged during independent review of #14, before merge.

Changed to loop until a full sweep discovers nothing new, capped at len(parsed)+1 sweeps as a safety bound a real fixed point can never reach.

Honest note on scope

While investigating a regression fixture, I could not construct a reachable repro matching the issue's own illustrative example with today's binding mechanics — a composite-literal field binding is always keyed "TypeName.Field" (never a bare identifier), and compositeLiteralFieldBindings only resolves a field's value through a direct constructor/factory call or a bare-identifier lookup — never a field-selector expression. Since Go identifiers can't contain ., a "TypeName.Field" binding can structurally never be discovered via that bare-identifier lookup path — meaning one composite literal currently can never depend on another's binding at all.

The fix is still correct and worth keeping: it removes the order-dependence class of bug outright rather than relying on it happening not to matter today, costs at most one extra sweep in the common (no-change) case, and is already in place if composite-literal resolution is ever extended to support selector-expression values (which would make the scenario reachable).

Verification

  • Full test suite green — no fixture changes needed, since existing composite-literal fixtures already exercise the (now-looped) pass without any behavior change.
  • Re-verified against weaviate (a real 4500+-file repo): same 4 real usages detected, no measurable performance regression (~2s, unchanged).

Summary by CodeRabbit

  • Bug Fixes
    • Improved resolution of nested composite literal field references across files, so related values are now more reliably recognized regardless of file order.
    • Reduced cases where valid field bindings could be missed during scanning, making analysis results more consistent.

Pass B (composite-literal field binding) ran as a single forward sweep
over parsed files sorted by relative path, not a fixed-point iteration —
a theoretical order-dependence risk flagged during independent review of
#14, before merge (not a demonstrated real-world miss).

Changed to loop until a full sweep discovers nothing new, capped at
len(parsed)+1 sweeps as a safety bound a real fixed point can never
reach (a genuine wrapper-of-wrapper chain can't legitimately be deeper
than the number of files in the scan).

Honest note on scope: while investigating a regression fixture for this,
I could not construct a reachable repro matching the issue's own
illustrative example with today's binding mechanics — a composite-literal
field binding is always keyed "TypeName.Field" (never a bare identifier),
and compositeLiteralFieldBindings only resolves a field's value through
either a direct constructor/factory call or a bare-identifier lookup —
never a field-selector expression. Since Go identifiers can't contain ".",
a "TypeName.Field" binding can structurally never be discovered via that
bare-identifier lookup path, meaning one composite literal currently can
never depend on another's binding at all. The fix remains correct, safe,
and worth keeping regardless: it removes the order-dependence class of
bug outright rather than relying on it happening to not matter today, and
it costs at most one extra sweep in the common (no-change) case — verified
against weaviate (a real 4500+-file repo), same 4 real usages detected,
no measurable performance regression (~2s, unchanged). If composite-literal
resolution is ever extended to support selector-expression values (making
the theoretical scenario reachable), this fix is already in place for it.

Full test suite green, no fixture changes needed since the existing
composite-literal fixtures already exercise the (now-looped) pass without
any behavior change.

Fixes #18

Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 639a02a3-6049-4336-b3b6-59add6ad4610

📥 Commits

Reviewing files that changed from the base of the PR and between e0b59bc and d9db73f.

📒 Files selected for processing (1)
  • internal/scanner/scanner.go

📝 Walkthrough

Walkthrough

The composite-literal field-binding discovery in runWholeScanAnalysis (Pass B) is changed from a single forward sweep over parsed files to a bounded fixed-point iteration loop that re-walks scopes and updates bindings until convergence or a safety cap is reached.

Changes

Composite-literal binding resolution

Layer / File(s) Summary
Fixed-point iteration for field bindings
internal/scanner/scanner.go
Pass B now repeatedly re-walks file/function scopes, recomputes compositeLiteralFieldBindings for each composite literal, and updates per-package bindings until a sweep yields no changes or a len(parsed)+1 safety cap is hit, replacing the previous single-pass, order-dependent approach.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main scanner change: looping Pass B to a fixed point to resolve the order-dependence bug.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pass-b-fixed-point

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@flaglint flaglint merged commit c267d60 into main Jul 7, 2026
14 checks passed
@flaglint flaglint deleted the fix/pass-b-fixed-point branch July 7, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scanner: composite-literal binding is a single forward pass, not fixed-point — order-dependent false negative

2 participants