Skip to content

A yield in a block a receiver widened to poly late lifts reaches its block - #10178

Merged
matz merged 1 commit into
matz:masterfrom
dai199:yield-late-poly-lift
Oct 25, 2026
Merged

matz merged 1 commit into
matz:masterfrom
dai199:yield-late-poly-lift

Conversation

@dai199

@dai199 dai199 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor
class Rel
  def initialize(src)
    @src = src
    @records = nil
  end

  def preloaded(records, loaded)
    @records = records if loaded
    self
  end

  def loaded_records
    records = @records
    if records.nil?
      records = @src.dup
      @records = records
    end
    records
  end

  def map
    loaded_records.map { |x| yield x }
  end
end

class Box
  def target = cache

  def cache
    if missing_value.nil?
    end
  end

  def parts = Rel.new([1]).preloaded(target, false)
end

def doubled(rel) = rel.map { |w| w * 2 }
p doubled(Rel.new([1, 2, 3]))

CRuby prints [2, 4, 6]; Spinel refused it at compile time: "yield in a
block that becomes a Proc, inside a method spliced into a caller that passes
it a block" (refuse_yield_lost_block, 75f03a5). While an_phase_procs looks
at map, loaded_records answers an Array, so the block is spliced with
map and map with its caller. Only an_phase_late_widen settles what
preloaded stores from target, which has no type until then, and turns
loaded_records poly. A poly receiver's dispatch materializes the block as a
proc (a_block_is_lifted), where the caller's block is out of reach. In the
roundhouse tiny-api tree the same shape widens in an_phase_method_backstops
(a method reached through method(:sym) pinned there).

The loop in an_phase_procs that lowers a yielding method into a function
taking its block as a proc is now an_lower_yielders, and it runs again after
an_phase_nullable_returns, once the late phases have widened what they widen.
A method it did not lower the first time and does now goes through the same
lowering, mark_proc_captures included.

test/yield_in_block_lifted_by_late_poly.rb covers the late-widened receiver,
an empty one, and the sibling each that stays spliced.

Verification

  • make gate not run. Targeted: every test/*.rb whose name has yield, block, proc, lambda, enum, iter or lower (promote_* excluded), through make test-corpus TESTS=...: 1661 pass, 0 fail, on this branch rebased onto 2814693.
  • The new test fails on master (the refusal above) and passes here; its .expected matches CRuby 3.4 and 4.0.1.
  • roundhouse's tests/tiny_api.rs spinel_gate_models_run, which hit this refusal at runtime/active_record/relation.rb:1581 since 75f03a5, compiles and runs with this branch.

Summary by CodeRabbit

  • Bug Fixes
    • Yielding methods are now correctly handled when later type analysis widens their types, including when blocks are passed through methods. This improves results for affected iteration and mapping scenarios.
  • Tests
    • Added coverage for block forwarding with lazily loaded and empty collections, as well as iteration over a regular list.

…block

```ruby
class Rel
  def initialize(src)
    @src = src
    @records = nil
  end

  def preloaded(records, loaded)
    @records = records if loaded
    self
  end

  def loaded_records
    records = @records
    if records.nil?
      records = @src.dup
      @records = records
    end
    records
  end

  def map
    loaded_records.map { |x| yield x }
  end
end

class Box
  def target = cache

  def cache
    if missing_value.nil?
    end
  end

  def parts = Rel.new([1]).preloaded(target, false)
end

def doubled(rel) = rel.map { |w| w * 2 }
p doubled(Rel.new([1, 2, 3]))
```

CRuby prints `[2, 4, 6]`; Spinel refused it at compile time: "`yield` in a
block that becomes a Proc, inside a method spliced into a caller that passes
it a block" (refuse_yield_lost_block, 75f03a5). While an_phase_procs looks
at `map`, `loaded_records` answers an Array, so the block is spliced with
`map` and `map` with its caller. Only an_phase_late_widen settles what
`preloaded` stores from `target`, which has no type until then, and turns
`loaded_records` poly. A poly receiver's dispatch materializes the block as a
proc (a_block_is_lifted), where the caller's block is out of reach. In the
roundhouse tiny-api tree the same shape widens in an_phase_method_backstops
(a method reached through `method(:sym)` pinned there).

The loop in an_phase_procs that lowers a yielding method into a function
taking its block as a proc is now an_lower_yielders, and it runs again after
an_phase_nullable_returns, once the late phases have widened what they widen.
A method it did not lower the first time and does now goes through the same
lowering, mark_proc_captures included.

test/yield_in_block_lifted_by_late_poly.rb covers the late-widened receiver,
an empty one, and the sibling `each` that stays spliced.
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7f15de17-096b-41ab-9583-cea78f860a41
Commits

Reviewing files that changed from the base of the PR and between 2814693 and 89e7a67.

Files selected for processing: 3
  • src/analyze.c
  • test/yield_in_block_lifted_by_late_poly.rb
  • test/yield_in_block_lifted_by_late_poly.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

The analyzer now runs yield-method lowering after late widening and nullable-return analysis. A new Ruby test covers relations with populated and empty results, block forwarding, and iteration over an array.

Changes: Yield-method lowering

Layer / File(s) Summary
Extract and repeat yield-method lowering
src/analyze.c
an_phase_procs delegates yield-method lowering to an_lower_yielders. The helper reports whether it lowered methods. analyze_program calls it again after late widening and nullable-return analysis.
Late-widening regression scenario
test/yield_in_block_lifted_by_late_poly.rb, test/yield_in_block_lifted_by_late_poly.rb.expected
The new test exercises relation mapping, block forwarding, and array iteration. Expected output includes [2, 4, 6], [], and [4, 5].

Priority: Normal

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

Change: Bug fix

Merge Risk: Minimal · up to 89e7a

The late-widening change appears ready to merge after normal checks; no actionable risk remains identified.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (2 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 names the specific bug fixed: a yield inside a block whose receiver becomes polymorphic only after late widening, where the block is lifted into a proc and must still reach its block. The ra…
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: Title check

Explanation

The title names the specific bug fixed: a yield inside a block whose receiver becomes polymorphic only after late widening, where the block is lifted into a proc and must still reach its block. The raw summary confirms the change moves yielding-method lowering to run again after late widening and nullable-return analysis, and adds a test for this exact scenario. The title is concise, single-sentence, and specific to the primary change, so it passes. It is somewhat long (74 characters, 15 words) and dense, but it is still clear to a teammate scanning history.

Full details: Docstring Coverage

Explanation

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

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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

Warning

Some tools did not complete. Review the errors below.

🔧 Cppcheck (2.22.0)
src/analyze.c

Cppcheck timed out; analysis of this file is incomplete

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.

@matz
matz merged commit a990dd2 into matz:master Oct 25, 2026
5 checks passed
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.

2 participants