Skip to content

Compose boxed String builtin freshness with user returns - #8420

Merged
matz merged 5 commits into
matz:masterfrom
dchuk:fix/boxed-builtin-freshness
Oct 11, 2026
Merged

matz merged 5 commits into
matz:masterfrom
dchuk:fix/boxed-builtin-freshness

Conversation

@dchuk

@dchuk dchuk commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

Under --share-strings, a boxed String call such as Thread.new { "https://" }.value.sub("://", "") can be refused because the program also defines a same-named user method. The sharing walk applies generic container effects instead of composing the existing String builtin effect with the user returns.

Compose these effects only when the builtin metadata and complete dispatch shape account for the call. Revalidate the recorded builtin effect before using it for freshness, and preserve every user return and retained argument. Analyzer plan queries use an uncached resolver so inference cannot seed a premature code-generation plan.

Singleton accessor publication is not fully represented by the holder graph. Programs declaring singleton readers or writers therefore retain the existing container effects, including when an instance-method arm reaches that storage through a helper. Explicit class-method writes remain eligible. Generic tests cover these direct/transitive aliases as well as fresh, borrowed, nil, and container returns.

Validation

Full make -j6 gate CC=gcc-12 GATE_CACHE=1 TEST_JOBS= BENCH_PJOBS=1 passed on Linux x86_64, merged with master 4412e698feb4. Head d02b51ec9fb1 carries the verified Gate trailer for tested tree d35667493d4c; attestation changed no source or compiler bytes.

Optcarrot: OK
Benchmarks: 70 pass, 0 fail, 0 error, 0 skip
scale-test: boxed Hash store work at 2x the methods is 1.95x (limit 2.20)
scale-test: boxed-receiver alias work at 2x the writes is 1.86x (limit 2.20)
scale-test: instance_eval forwarding work at 2x the wrappers is 1.71x (limit 2.50)
scale-test: work at 4x the program is 5.00x (linear 4.00, limit 5.20)
scale-test: work at 4x the program, compiled to C, is 6.27x (limit 6.90)
scale-test: call-shape work at 4x the units, compiled to C, is 4.15x (linear 4.00, limit 4.50)
rubyspec-gate[language]: all 1340 expected-PASS examples still pass
rubyspec-gate[core/array]: all 819 expected-PASS examples still pass
rubyspec-gate[core/string]: all 878 expected-PASS examples still pass
gate-test-shared: known ERR: string_plain_mutator_result_kept
gate-test-shared: known ERR: yield_string_mutator_tail
gate-test-shared: 6816 pass, 2 known failures, 0 new failures
rubyspec-gate[core/hash]: all 300 expected-PASS examples still pass
Tests: 6818 pass, 0 fail, 0 error
rubyspec-gate[core/integer]: all 347 expected-PASS examples still pass
rubyspec-gate[core/range]: all 210 expected-PASS examples still pass
gate: ALL GREEN

The two listed shared-corpus failures already exist on master4412. Focused baseline/candidate compiles produce identical #6765 refusals for both; the known-failure list was not changed.

  • Six new focused sharing-verifier cases passed on the preceding identical patch. Direct/transitive alias controls and GC stress 0/1 match frozen CRuby 4.0; actual native borrowed bindings remain refused.

  • Optcarrot generated C is byte-identical to master4412 (616,836 bytes; SHA-256 c05a6c722851314f695b908536097947f324d9c430fb81d5749b42d71b4adefa); checksum 59662 and all six scale ratios match.

  • The separate full sharing verifier is not green on clean master 61dedaea or the prior patched 9e0fe408 integration: both have the same two unratcheted exception-channel diagnostics, one net_http_zlib generated-C parity failure, eight stale entries, and 115 channel gaps. Complete normalized logs match; current 4412e698 focused baseline/candidate checks also match. No ratchet entry was added or waived, and unobserved channels are not proven safe.

  • Hosted clang CI and full CodeRabbit review passed on 41f473318d67, the exact same tree. No actionable review comments remain; the generic docstring warning remains disclosed. Hosted clang CI and Gate verification also pass on the final attested head d02b51ec9fb1 (run); the final review-thread check found zero threads. Other platform lanes are skipped by the PR workflow.

  • New tests have .expected files matching CRuby 4.0 with --enable-frozen-string-literal

  • No added test uses values past 2^31

  • Complete make gate and same-master compiler-performance comparison

  • Depends on: none

Gate: green tree 38933e8 master dacaa29 (linux-x86_64 gcc-12.2.0) tests 6801/0
@coderabbitai

coderabbitai Bot commented Oct 10, 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: fb0760ce-f227-459d-89e3-b3ed85611707





📥 Commits

Reviewing files that changed from the base of the PR and between 4412e69 and 41f4733.






📒 Files selected for processing (16)
  • docs/limitations.md
  • src/analyze_share.c
  • src/call_plan.c
  • src/call_plan.h
  • test/share/boxed_pure_user_arm_keeps_nested_argument.rb
  • test/share/boxed_pure_user_arm_keeps_nested_argument.rb.expected
  • test/share/boxed_sub_class_value_return_keeps_nested_string.rb
  • test/share/boxed_sub_class_value_return_keeps_nested_string.rb.expected
  • test/share/boxed_sub_fresh_with_user_arm.rb
  • test/share/boxed_sub_fresh_with_user_arm.rb.expected
  • test/share/boxed_sub_transitive_class_accessor_keeps_nested_string.rb
  • test/share/boxed_sub_transitive_class_accessor_keeps_nested_string.rb.expected
  • test/share/boxed_sub_transitive_explicit_class_ivar_keeps_nested_string.rb
  • test/share/boxed_sub_transitive_explicit_class_ivar_keeps_nested_string.rb.expected
  • test/share/boxed_sub_user_arms_run_both.rb
  • test/share/boxed_sub_user_arms_run_both.rb.expected





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
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The share analysis now composes eligible boxed String builtin rows with user-method returns. It records and revalidates freshness evidence for mixed calls. Tests cover nested argument retention and nested String aliases across several user-method return shapes.

Changes

Boxed String dispatch

Layer / File(s) Summary
Plan resolution and analysis facts
src/call_plan.c, src/call_plan.h, src/analyze_share.c
cplan_poly_fresh resolves a plan without using the memoized plan cache. The share analysis stores facts for mixed-call freshness and singleton-accessor detection.
Mixed dispatch and composition proof
src/analyze_share.c
The analysis selects boxed builtin rows and checks whether eligible String rows can be composed with user-method returns. It joins accepted results and skips builtin peek marks for calls with user targets.
Freshness revalidation and lifecycle
src/analyze_share.c
The analysis records and revalidates mixed-call evidence against current targets, rows, types, and plan details. It frees freshness records and indexes.
Mixed-call regression coverage
test/share/boxed_*, docs/limitations.md
Tests cover nested argument retention, String substitutions with multiple user-method return shapes, and nested String aliases. The documentation describes dispatch constraints.

Priority: ⬇️ Low

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

Change: Bug fix












Merge Risk: ⚪ Minimal · up to 41f47

No actionable defect is established in the changed String-sharing behavior. Merge after the normal pending checks complete.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 41f47

The change permits additional string-sharing cases only after checking the call shape, while retaining aliases from user methods. No introduced security defect was identified in the inspected paths, but broader validation and security coverage remain incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is ownership reasoning for programs containing mixed boxed String/user-method calls. Program-controlled methods and arguments influence that reasoning, but the inspected change does not add a service, credential, or tenant-authority transition.

Trust Boundaries and Controls

  • observed — User arguments are bound into their method flows and user returns are joined before builtin composition. Calls with user targets skip builtin-only peek settlement, preventing the builtin's no-retention behavior from erasing aliases retained by a user arm.
  • observed — Composition rejects unsupported argument or block shapes, competing builtin faces, dynamic lookup routes, missing-method candidates, and unaccounted dispatch arms. Unsupported calls retain generic container effects instead of gaining freshness authority.

Resilience and Maintainability Implications

  • observed — The fresh resolver bypasses the memoized plan rather than populating it during inference. Its scratch result is consumed immediately, and recorded freshness stores copied facts rather than retaining the scratch-plan pointer. This separates evolving analysis evidence from code-generation planning.







Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 9 files. (7 skipped: … 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 clearly and concisely describes the primary change: composing boxed String builtin freshness with user-method returns.


Full details: Docstring Coverage

Explanation

Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 9 files. (7 skipped: 7 unsupported.)




  • 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: verified The head commit's Gate trailer names the tree its merge with master gives label Oct 10, 2026
@github-actions github-actions Bot added gate: no trailer The head commit carries no Gate trailer and removed gate: verified The head commit's Gate trailer names the tree its merge with master gives labels Oct 10, 2026
@dchuk

dchuk commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

dchuk added 2 commits October 10, 2026 21:16
Singleton accessor publication is not fully represented in the sharing
holder graph. Replacing the generic container effects could lose an alias
returned directly by a class method or indirectly through an instance arm.
Keep the previous effects whenever the program declares singleton readers
or writers; explicit class-method writes remain eligible.

Add direct and transitive identity regressions and an explicit-write control.
@dchuk

dchuk commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@dchuk

dchuk commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

…-freshness

Gate: green tree d356674 master 4412e69 (linux-x86_64 gcc-12.2.0) tests 6818/0
@dchuk
dchuk force-pushed the fix/boxed-builtin-freshness branch from 41f4733 to d02b51e Compare October 11, 2026 00:46
@github-actions github-actions Bot added gate: verified The head commit's Gate trailer names the tree its merge with master gives and removed gate: no trailer The head commit carries no Gate trailer labels Oct 11, 2026
@dchuk
dchuk marked this pull request as ready for review October 11, 2026 01:10
@matz
matz merged commit 3dcfed5 into matz:master Oct 11, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate: verified The head commit's Gate trailer names the tree its merge with master gives

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants