Skip to content

Whether a to_a result is mutated finds its local writes off an index - #8149

Merged
matz merged 1 commit into
matz:masterfrom
amatsuda:pr/to-a-mutated-write-index
Oct 9, 2026
Merged

matz merged 1 commit into
matz:masterfrom
amatsuda:pr/to-a-mutated-write-index

Conversation

@amatsuda

@amatsuda amatsuda commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Compile time: an_to_a_result_mutated found the local writes of an Array-answering call by walking every local-variable write in the program and comparing its value, once per call asked about and every fixpoint round.

The rest of the question was already off the variable-site index (#8014): the receiver parent, and the calls on each local. This adds the missing piece to the same index: a chain of the local writes by the node they write (comp_lwrite_of_value / comp_lwrite_next). It's built in the same pass as the rest of the index and rebuilt under the same conditions. A chain rather than a single slot keeps the answer exact if a desugar ever shares a value node between writes.

Measured on an 86k-line actionpack program (a single-file ActionDispatch app with its gems inlined), on master plus #8138: this loop was the costliest line of a second-pass fixpoint round, about 14% of its samples (analyze_infer.c:1866).

The answer is a yes/no over the same writes, so behavior doesn't change. The emitted C for the 33k-line activesupport sample is byte-identical to master's.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Performance
    • Improved analysis performance for code involving conversions and local-variable mutations by narrowing the writes examined. This may reduce the time needed to process affected code without changing the analysis behavior.

an_to_a_result_mutated asks, for an Array a call answers, whether it is
mutated in place: as the receiver of a mutator, or through a local it is
written to. The receiver and the local's calls come off the
variable-site index (matz#8014), but the writes themselves were found by
walking every local-variable write of the program and comparing its
value, for each call asked about, every round. On the 86k-line
actionpack sample that walk was the costliest line of a second-pass
fixpoint round (~14% of its samples).

The site index now also chains the local writes by the node they write
(comp_lwrite_of_value), built in the same pass and kept as fresh. The
answer is a yes/no over the same writes, so nothing else changes; the
activesupport sample's C is byte-identical.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af34ff25-933a-447c-823d-16e4f604085e

📥 Commits

Reviewing files that changed from the base of the PR and between 4337d93 and 93d861c.


📒 Files selected for processing (3)
  • src/analyze_infer.c
  • src/compiler.c
  • src/compiler.h

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.



📝 Walkthrough

Walkthrough

The compiler now indexes local-variable writes by the node that supplies their value. Type analysis uses this index to find writes associated with a call when checking whether its result is mutated.

Changes

Local Write Index

Layer / File(s) Summary
Build and expose write chains
src/compiler.h, src/compiler.c
Compiler now stores write-chain arrays. Variable-site indexing builds the chains, traversal functions expose them, and comp_free releases the arrays.
Use write chains in type analysis
src/analyze_infer.c
an_to_a_result_mutated checks writes linked to the call instead of scanning every local-variable write. The receiver-mutator check remains unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: matz


Merge Risk

Merge Risk: ⚪ Minimal · up to 93d86

No merge-blocking issue was identified in the indexed lookup or its integration with type analysis.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 93d86

The new lookup preserves the existing mutation checks and remains within compile-time analysis. Its storage follows the existing rebuild and cleanup lifecycle. No introduced or worsened security issue was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For the inspected path, input-derived AST nodes affect compiler memory allocation and compile-time mutation analysis. The new helpers operate on an existing Compiler and node IDs; they do not introduce another identity or authority transition.

Trust Boundaries and Controls

  • observed — Value references are range-checked before indexing, traversal IDs are range-checked before array access, and the consumer independently validates node kind, value identity, variable name, and owning scope.

Resilience and Maintainability Implications

  • observed — The index adds two integer arrays proportional to AST node count. Partial allocation failure follows the pre-existing process-terminating out-of-memory policy rather than allowing analysis to consume a partially built index.



🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title describes the main change: using an index to find local writes when checking whether a to_a result is mutated. The wording is somewhat awkward but remains specific and related.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 too large.)



  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@github-actions github-actions Bot added the gate: no trailer The head commit carries no Gate trailer label Oct 9, 2026
@matz
matz merged commit e38bc84 into matz:master Oct 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate: no trailer The head commit carries no Gate trailer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants