Skip to content

A builtin's Integer or Float answer fills a dispatch slot that holds its nil - #8472

Merged
matz merged 1 commit into
matz:masterfrom
makenowjust:fix-builtin-arm-oint-lift
Oct 11, 2026
Merged

matz merged 1 commit into
matz:masterfrom
makenowjust:fix-builtin-arm-oint-lift

Conversation

@makenowjust

@makenowjust makenowjust commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

A call on a boxed value whose class has a reader that can answer nil, named like a builtin's Integer or Float method,
dispatches over the class and the builtin. The C did not build:

class Job
  attr_reader :pid
  def initialize(pid) = (@pid = pid)
end

def pid_of(m) = m.pid

p pid_of(Job.new(4))      # 4
p pid_of(Job.new(nil))    # nil
_, st = Process.wait2(Process.spawn("true"))
p pid_of(st) > 0          # true
# error: assigning to 'sp_oint' from incompatible type 'sp_int'

The same happened for utime, stime, to_i, to_f, ord, length and bytesize. bf34b79 fixed arity alone.

The call is an oint node, and its builtin arm re-enters the builtin's emitter with g_poly_builtin_arm set.
node_is_oint still counted the program's reader there, so the arm took the emitter's plain sp_int / sp_float for
the oint.

What changes:

  • The builtin arm: in the call's own builtin arm, node_is_oint now asks for the builtin's answer alone.
    nullable_int_value leaves out the program's methods of that name (g_nn_builtin_call). emit_oint_expr then lifts
    a plain answer, and keeps an answer that holds its own nil (exitstatus).
  • The other arms: the String pre-arm and the to_i / to_f default arm take the slot's oint the same way.
  • The per-emitter lifts (the Time readers, tv_sec, arity) are removed, since the arm now does the lifting.
  • Under --int-overflow=promote:
    • The reader's ivar is boxed.
    • poly_dispatch_nullable counted only an Integer or Float ivar, so the dispatch slot was plain, and the reader's arm
      raised TypeError for its nil (year, hour, length, to_i).
    • A boxed ivar now counts as well.

test/poly_builtin_reader_beside_nil_reader.rb covers eleven such names, on the object (with values and with nils) and
on the builtin. Its expected output is CRuby 4.0.7's. Without the change, the C does not build under raise or promote.

Verification:

  • The new test passes in raise, wrap and promote, together with 206 related tests, through make, on macOS. The related
    tests are those whose classes have readers or methods named like builtin Integer readers, plus the poly-dispatch /
    reader-named / Tms tests.
  • make scale-test passes; tools/gate.rb check-range passes.
  • make gate: ALL GREEN on 4eb12fd on c6704b9, Linux x86_64, gcc 15.2.0 (6917 tests, 0 failures). The full
    corpus under wrap (6901 tests) and promote (6932 tests): 0 failures.
  • -m32 (clang -m32, -O0, Linux): the build is clean, test-pch passes, and the new test passes in raise, wrap and
    promote.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected results from built-in numeric conversions and accessors when called alongside nullable values. Integer and floating-point results now retain the expected value or nil across string, time, process, and callable methods.
    • Added coverage for built-in readers with populated and nil-valued records, including process status and timing, numeric conversions, strings, times, and callable arity.

…its nil

A call on a boxed value whose class has a reader that can answer nil,
named like a builtin's Integer or Float method (pid, to_i, length,
utime, ...), dispatches over the class and the builtin. The call is an
oint node, and its builtin arm re-enters the builtin's emitter with
g_poly_builtin_arm set. node_is_oint still counted the program's reader
there, so the arm took the emitter's plain sp_int / sp_float for the
oint, and the C did not build. bf34b79 wrapped Proc#arity alone.

In the call's own builtin arm, node_is_oint now asks for the builtin's
answer alone: nullable_int_value leaves the program's methods of the
name out. emit_oint_expr then lifts a plain answer and keeps one that
holds its own nil (exitstatus). The String pre-arm and the to_i / to_f
default arm take the slot's oint the same way. The per-emitter lifts
(the Time readers, tv_sec, arity) are gone.

Under --int-overflow=promote the reader's ivar is boxed, and
poly_dispatch_nullable counted only an Integer or Float ivar, so the
dispatch slot was plain and the reader's arm raised TypeError for its
nil. A boxed ivar counts as well.

test/poly_builtin_reader_beside_nil_reader.rb covers eleven such names
on the object (with values and with nils) and on the builtin. Its
expected output is CRuby 4.0.7's.
@coderabbitai

coderabbitai Bot commented Oct 11, 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: ee067fdb-a2a0-4694-a204-15554f3ea208

📥 Commits

Reviewing files that changed from the base of the PR and between 91b8722 and 4eb12fd.


📒 Files selected for processing (7)
  • src/analyze.c
  • src/analyze.h
  • src/codegen_call.c
  • src/codegen_poly_plan.c
  • src/codegen_util.c
  • test/poly_builtin_reader_beside_nil_reader.rb
  • test/poly_builtin_reader_beside_nil_reader.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

The changes update nullable-integer analysis for poly-builtin calls and adjust code generation for accessor and conversion results. A new test exercises these readers with populated and nil-valued records, along with process, string, time, and lambda values.

Changes

Poly-builtin nullable readers

Layer / File(s) Summary
Track nullable builtin calls
src/analyze.h, src/analyze.c, src/codegen_util.c
The analyzer adds g_nn_builtin_call and uses it when checking nullable integer results for a builtin call. Poly dispatch now considers a TY_POLY ivar nullable when nil_rides is true.
Emit builtin results for nullable slots
src/codegen_call.c, src/codegen_poly_plan.c, test/poly_builtin_reader_beside_nil_reader.rb, test/poly_builtin_reader_beside_nil_reader.rb.expected
Code generation adjusts result handling for boxed Time and Proc/Method accessors, String conversions, and numeric-conversion fallbacks. The new test exercises readers with populated and nil-valued records, plus process, string, time, and lambda values.

Priority: ➖ Normal

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

Change: Bug fix


Merge Risk | ⚪ Minimal · up to 4eb12

Merge Risk: ⚪ Minimal · up to 4eb12

No actionable issue remains from the reviewed changes; the PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 4eb12

The change corrects nullable numeric results rather than expanding access or privileges. No security regression was identified in the inspected paths, but exceptional recovery and concurrent compilation were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The established affected scope is compiler query state and generated-program numeric dispatch semantics. The routed test methods do not establish additional production reachability, authority, or cross-service exposure. The capped dependency evidence does not establish exhaustive downstream coverage.

Trust Boundaries and Controls

  • observed — The builtin-only selector is activated only when emission is inside a builtin arm and the dispatch-skip node equals the queried node. The analysis exclusion likewise checks exact node identity, rather than excluding every program reader sharing the method name.

Resilience and Maintainability Implications

  • observed — The selector saves and restores its previous value before returning from the normal query path. Builtin-arm emission separately restores its pin, recovery buffer, emission state, and views after successful emission or a caught refusal. View unwinding restores stacked arm contexts, but the new selector is a separate global; these mechanisms do not by themselves prove selector cleanup for every interruption or isolation between concurrent compiler instances.

Pre-merge checks | Passed 4 | Inconclusive 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (3 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: builtin Integer or Float results now populate a dispatch slot that may hold nil.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (3 skipped: 1 unsupported, 2 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 11, 2026
@matz
matz merged commit 644ed74 into matz:master Oct 11, 2026
6 checks passed
@makenowjust
makenowjust deleted the fix-builtin-arm-oint-lift branch October 11, 2026 07:16
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