From 5970638803840297c2daaca1635c9a98453d740c Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Fri, 8 May 2026 11:55:13 +0900 Subject: [ruby/rubygems] Move overrides off SpecSet onto LazySpecification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpecSet previously kept its own @overrides and a with_overrides setter that had to be chained on every SpecSet.new(...) site (~13 sites in Definition alone). Two Codex review rounds both flagged forgotten chains in different SpecSet construction paths, which is exactly the class of bug the chain pattern invites: it is purely "remember to write" with no compiler help. Move the override list to LazySpecification#overrides instead. The LazySpec is the natural carrier — it is the value object the resolver and install paths already pass around, and choose_compatible already read overrides off it. Override.attach(specs, overrides) is added as the dual of Override.find_for so Definition (after lockfile load) and Resolver (after solve_versions) can populate the overrides on every LazySpec they hand out, and LazySpecification.from_spec carries the list forward when one LazySpec spawns another. Generic spec types (StubSpecification, plain Gem::Specification, RemoteSpecification) are intentionally ignored so generic metadata callers (SelfManager, materialize-time strict checks) keep their current strict semantics. SpecSet drops attr_accessor :overrides, the @overrides initialisation, the with_overrides cascade, and reverts SpecSet#valid? to the strict matches_current_metadata? check. Every SpecSet.new(...) site in Definition stops chaining .with_overrides — the LazySpecs already carry the context. https://github.com/ruby/rubygems/commit/fc1e8d4d7e Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/bundler/definition.rb | 17 +++++++++-------- lib/bundler/lazy_specification.rb | 1 + lib/bundler/override.rb | 10 ++++++++++ lib/bundler/resolver.rb | 1 + lib/bundler/spec_set.rb | 19 +++---------------- spec/bundler/bundler/override_spec.rb | 27 +++++++++++++++++++++++++++ spec/bundler/bundler/spec_set_spec.rb | 25 ------------------------- 7 files changed, 51 insertions(+), 49 deletions(-) diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index 45c6e9a693..6f5ab29b15 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -111,12 +111,13 @@ module Bundler @locked_bundler_version = @locked_gems.bundler_version @locked_ruby_version = @locked_gems.ruby_version @locked_deps = @locked_gems.dependencies - @originally_locked_specs = SpecSet.new(@locked_gems.specs).with_overrides(@overrides) + Override.attach(@locked_gems.specs, @overrides) + @originally_locked_specs = SpecSet.new(@locked_gems.specs) @originally_locked_sources = @locked_gems.sources @locked_checksums = @locked_gems.checksums if @unlocking_all - @locked_specs = SpecSet.new([]).with_overrides(@overrides) + @locked_specs = SpecSet.new([]) @locked_sources = [] else @locked_specs = @originally_locked_specs @@ -137,7 +138,7 @@ module Bundler @most_specific_locked_platform = nil @platforms = [] @locked_deps = {} - @locked_specs = SpecSet.new([]).with_overrides(@overrides) + @locked_specs = SpecSet.new([]) @locked_sources = [] @originally_locked_specs = @locked_specs @originally_locked_sources = @locked_sources @@ -338,11 +339,11 @@ module Bundler elsif no_resolve_needed? if deleted_deps.any? Bundler.ui.debug "Some dependencies were deleted, using a subset of the resolution from the lockfile" - SpecSet.new(filter_specs(@locked_specs, @dependencies - deleted_deps)).with_overrides(@overrides) + SpecSet.new(filter_specs(@locked_specs, @dependencies - deleted_deps)) else Bundler.ui.debug "Found no changes, using resolution from the lockfile" if @removed_platforms.any? || @locked_gems.may_include_redundant_platform_specific_gems? - SpecSet.new(filter_specs(@locked_specs, @dependencies)).with_overrides(@overrides) + SpecSet.new(filter_specs(@locked_specs, @dependencies)) else @locked_specs end @@ -506,7 +507,7 @@ module Bundler def normalize_platforms resolve.normalize_platforms!(current_dependencies, platforms) - @resolve = SpecSet.new(resolve.for(current_dependencies, @platforms)).with_overrides(@overrides) + @resolve = SpecSet.new(resolve.for(current_dependencies, @platforms)) end def add_platform(platform) @@ -762,7 +763,7 @@ module Bundler local_platform_needed_for_resolvability = @most_specific_non_local_locked_platform && !@platforms.include?(Bundler.local_platform) @platforms << Bundler.local_platform if local_platform_needed_for_resolvability - result = SpecSet.new(resolver.start).with_overrides(@overrides) + result = SpecSet.new(resolver.start) @resolved_bundler_version = result.find {|spec| spec.name == "bundler" }&.version @@ -788,7 +789,7 @@ module Bundler result.add_originally_invalid_platforms!(platforms, @originally_invalid_platforms) end - SpecSet.new(result.for(dependencies, @platforms | [Gem::Platform::RUBY])).with_overrides(@overrides) + SpecSet.new(result.for(dependencies, @platforms | [Gem::Platform::RUBY])) end def precompute_source_requirements_for_indirect_dependencies? diff --git a/lib/bundler/lazy_specification.rb b/lib/bundler/lazy_specification.rb index 86b29ee115..0da621d21f 100644 --- a/lib/bundler/lazy_specification.rb +++ b/lib/bundler/lazy_specification.rb @@ -30,6 +30,7 @@ module Bundler lazy_spec.dependencies = s.runtime_dependencies lazy_spec.required_ruby_version = s.required_ruby_version lazy_spec.required_rubygems_version = s.required_rubygems_version + lazy_spec.overrides = s.overrides if s.is_a?(LazySpecification) lazy_spec end diff --git a/lib/bundler/override.rb b/lib/bundler/override.rb index 7b71415290..0e0ec59fd7 100644 --- a/lib/bundler/override.rb +++ b/lib/bundler/override.rb @@ -9,6 +9,16 @@ module Bundler overrides.find {|o| o.target == :all && o.field == field } end + # Attach the given overrides onto every LazySpecification in `specs` so + # downstream consumers (LazySpecification#choose_compatible, the install- + # time compatibility check, etc.) can read the override list off the spec + # itself. Non-LazySpec entries (StubSpecification, Gem::Specification, ...) + # are left untouched. + def self.attach(specs, overrides) + return if overrides.nil? || overrides.empty? + specs.each {|s| s.overrides = overrides if s.is_a?(LazySpecification) } + end + attr_reader :target, :field, :operation, :source_location def initialize(target, field, operation, source_location: nil) diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index fb69becd69..096a349249 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -82,6 +82,7 @@ module Bundler solver = PubGrub::VersionSolver.new(source: self, root: root, strategy: Strategy.new(self), logger: logger) result = solver.solve resolved_specs = result.flat_map {|package, version| version.to_specs(package, @most_specific_locked_platform) } + Override.attach(resolved_specs, @base.overrides) SpecSet.new(resolved_specs).specs_with_additional_variants_from(@base.locked_specs) rescue PubGrub::SolveFailure => e incompatibility = e.incompatibility diff --git a/lib/bundler/spec_set.rb b/lib/bundler/spec_set.rb index 72e4d9c1dd..e8d990d207 100644 --- a/lib/bundler/spec_set.rb +++ b/lib/bundler/spec_set.rb @@ -7,21 +7,8 @@ module Bundler include Enumerable include TSort - attr_accessor :overrides - def initialize(specs) @specs = specs - @overrides = [] - end - - def with_overrides(overrides) - @overrides = overrides || [] - # Only LazySpecification carries an overrides accessor. Avoid - # respond_to?(:overrides=) here because RemoteSpecification#respond_to? - # forwards to _remote_specification, which would force-load the - # backing gemspec to answer the question. - @specs.each {|s| s.overrides = @overrides if s.is_a?(LazySpecification) } - self end def for(dependencies, platforms = [nil], legacy_platforms = [nil], skips: []) @@ -138,7 +125,7 @@ module Bundler def materialize(deps) materialize_dependencies(deps) - SpecSet.new(materialized_specs).with_overrides(@overrides) + SpecSet.new(materialized_specs) end # Materialize for all the specs in the spec set, regardless of what platform they're for @@ -159,7 +146,7 @@ module Bundler def incomplete_specs_for_platform(deps, platform) return [] if @specs.empty? - validation_set = self.class.new(@specs).with_overrides(@overrides) + validation_set = self.class.new(@specs) validation_set.for(deps, [platform]) validation_set.incomplete_specs end @@ -242,7 +229,7 @@ module Bundler end def valid?(s) - s.matches_current_metadata_with_overrides?(@overrides) && valid_dependencies?(s) + s.matches_current_metadata? && valid_dependencies?(s) end def to_s diff --git a/spec/bundler/bundler/override_spec.rb b/spec/bundler/bundler/override_spec.rb index d1a5568bd8..ad8be75520 100644 --- a/spec/bundler/bundler/override_spec.rb +++ b/spec/bundler/bundler/override_spec.rb @@ -34,6 +34,33 @@ RSpec.describe "MatchMetadata override-aware checks" do end end +RSpec.describe "LazySpecification override propagation" do + let(:overrides) { [Bundler::Override.new("rails", :required_ruby_version, :ignore_upper)] } + + it "carries overrides forward from a source LazySpec via from_spec" do + src = Bundler::LazySpecification.new("rails", "8.0", Gem::Platform::RUBY) + src.overrides = overrides + derived = Bundler::LazySpecification.from_spec(src) + expect(derived.overrides).to eq(overrides) + end + + it "does not call respond_to? on the source spec, avoiding gemspec lazy load" do + # If from_spec used respond_to?(:overrides), a RemoteSpec source would + # force-load the backing gemspec. Use a stand-in object whose + # respond_to? raises to prove it is never asked. + src = Object.new + src.define_singleton_method(:name) { "rails" } + src.define_singleton_method(:version) { Gem::Version.new("8.0") } + src.define_singleton_method(:platform) { Gem::Platform::RUBY } + src.define_singleton_method(:source) { nil } + src.define_singleton_method(:runtime_dependencies) { [] } + src.define_singleton_method(:required_ruby_version) { Gem::Requirement.default } + src.define_singleton_method(:required_rubygems_version) { Gem::Requirement.default } + src.define_singleton_method(:respond_to?) {|*| raise "from_spec must not call respond_to?" } + expect { Bundler::LazySpecification.from_spec(src) }.not_to raise_error + end +end + RSpec.describe Bundler::Override do describe ".find_for" do it "returns the matching override by target and field" do diff --git a/spec/bundler/bundler/spec_set_spec.rb b/spec/bundler/bundler/spec_set_spec.rb index dce86793b9..3e630555e6 100644 --- a/spec/bundler/bundler/spec_set_spec.rb +++ b/spec/bundler/bundler/spec_set_spec.rb @@ -93,29 +93,4 @@ RSpec.describe Bundler::SpecSet do end end - describe "#with_overrides" do - it "defaults to an empty override list" do - expect(described_class.new([]).overrides).to eq([]) - end - - it "stores the overrides supplied" do - override = Bundler::Override.new("rails", :version, ">= 8.0") - expect(described_class.new([]).with_overrides([override]).overrides).to eq([override]) - end - - it "treats nil as an empty override list" do - set = described_class.new([]) - override = Bundler::Override.new("rails", :version, ">= 8.0") - set.with_overrides([override]) - set.with_overrides(nil) - expect(set.overrides).to eq([]) - end - - it "cascades overrides to contained specs that accept them" do - lazy = Bundler::LazySpecification.new("rails", "8.0", Gem::Platform::RUBY) - override = Bundler::Override.new("rails", :version, ">= 8.0") - described_class.new([lazy]).with_overrides([override]) - expect(lazy.overrides).to eq([override]) - end - end end -- cgit v1.2.3