From 5eb82f3d405dcee61b0826be5021ac79e42cee7c Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Fri, 14 Aug 2026 13:37:21 -0400 Subject: [PATCH 1/2] Add a CompoundStatement parent chain; derive closure as a fallback Region now tracks compound_statement (the nearest enclosing CompoundStatement pin - an if/when/while/until/rescue/&&/||/||= body, a method/block body, or a namespace body), threaded through Region#update the same way closure already is. Every construct that creates a CompoundStatement-family pin, or previously only threaded conditional_boundary with no corresponding pin, now sets this pointer, giving every CompoundStatement pin a real link to its immediate parent instead of only the coarser closure chain (which already skips non-scope-forming branches like if-bodies). Pin::Base#closure becomes @closure || , kept strictly as a fallback behind the stored value - hand-built pins that pass closure: directly and have no derivable chain (send_node.rb's synthetic attr_reader/attr_writer pins, args_node.rb, etc.) are untouched. Every pin built through Region-threaded node processors still passes closure: explicitly today, so this is a no-behavior-change infra addition, verified by a new spec asserting the derived value agrees with the stored one across nested if/while/block structures. Pin::CompoundStatement also gains its own combine_with/ combine_compound_statement for incremental-reparse merging, mirroring BaseVariable#combine_closure's location-based tiebreak rather than reusing choose_pin_attr_with_same_name (unsuitable since bare CompoundStatement pins all share name == ''). BaseVariable also gains a compound_statement reader, threaded from lvasgn_node.rb, unused by any override logic yet - preparation for a follow-up that rewrites override_assignments?/definite_reaches? to walk this chain instead of comparing conditional_override_boundary Ranges, removing that duplicate bookkeeping. See the discussion on https://github.com/castwide/solargraph/pull/1282 for the fix this builds on and the design rationale for this follow-up. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk --- .../parser_gem/node_processors/and_node.rb | 10 ++- .../parser_gem/node_processors/block_node.rb | 3 +- .../parser_gem/node_processors/def_node.rb | 3 +- .../parser_gem/node_processors/defs_node.rb | 3 +- .../parser_gem/node_processors/if_node.rb | 15 +++- .../parser_gem/node_processors/lvasgn_node.rb | 1 + .../node_processors/namespace_node.rb | 3 +- .../parser_gem/node_processors/or_node.rb | 10 ++- .../parser_gem/node_processors/orasgn_node.rb | 11 ++- .../node_processors/resbody_node.rb | 14 +++- .../parser_gem/node_processors/until_node.rb | 6 +- .../parser_gem/node_processors/when_node.rb | 6 +- .../parser_gem/node_processors/while_node.rb | 6 +- lib/solargraph/parser/region.rb | 23 +++++- lib/solargraph/pin/base.rb | 20 +++++ lib/solargraph/pin/base_variable.rb | 15 ++++ lib/solargraph/pin/compound_statement.rb | 45 ++++++++++- spec/pin/compound_statement_spec.rb | 77 +++++++++++++++++++ 18 files changed, 249 insertions(+), 22 deletions(-) create mode 100644 spec/pin/compound_statement_spec.rb diff --git a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb index 633474a29..7e1da26b1 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb @@ -12,7 +12,15 @@ def process # any assignment there isn't guaranteed to have executed lhs, rhs = node.children NodeProcessor.process(lhs, region, pins, locals, ivars) - NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs)), pins, locals, ivars) + # not pushed onto `pins` - see resbody_node.rb for why + rhs_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(rhs), + closure: region.closure, + compound_statement: region.compound_statement, + node: rhs, + source: :parser + ) + NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs), compound_statement: rhs_cs), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb index 0e87cf935..232cf0682 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb @@ -20,6 +20,7 @@ def process block_pin = Solargraph::Pin::Block.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, node: node, context: context, receiver: node.children[0], @@ -31,7 +32,7 @@ def process # a block's body may execute zero or multiple times (e.g. # Enumerable#each), so an assignment inside it is never # guaranteed to have executed - process_children region.update(closure: block_pin, conditional_boundary: Range.from_node(node)) + process_children region.update(closure: block_pin, conditional_boundary: Range.from_node(node), compound_statement: block_pin) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb index f45f5544d..b6e6137d6 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb @@ -15,6 +15,7 @@ def process methpin = Solargraph::Pin::Method.new( location: get_node_location(node), closure: region.closure, + compound_statement: region.compound_statement, name: name, context: method_context, comments: comments_for(node), @@ -51,7 +52,7 @@ def process else pins.push methpin end - process_children region.update(closure: methpin, scope: methpin.scope) + process_children region.update(closure: methpin, scope: methpin.scope, compound_statement: methpin) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb index 09679c7f7..9690fcf87 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb @@ -22,6 +22,7 @@ def process pins.push Solargraph::Pin::Method.new( location: loc, closure: closure, + compound_statement: region.compound_statement, name: node.children[1].to_s, comments: comments_for(node), scope: :class, @@ -29,7 +30,7 @@ def process node: node, source: :parser ) - process_children region.update(closure: pins.last, scope: :class) + process_children region.update(closure: pins.last, scope: :class, compound_statement: pins.last) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb index 0f3a4800c..db303d733 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb @@ -17,6 +17,7 @@ def process pins.push Solargraph::Pin::CompoundStatement.new( location: get_node_location(condition_node), closure: region.closure, + compound_statement: region.compound_statement, node: condition_node, source: :parser ) @@ -24,26 +25,32 @@ def process end then_node = node.children[1] if then_node - pins.push Solargraph::Pin::CompoundStatement.new( + # @sg-ignore Need to add nil check here + then_cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(then_node), closure: region.closure, + compound_statement: region.compound_statement, node: then_node, source: :parser ) + pins.push then_cs # @sg-ignore Need to add nil check here - NodeProcessor.process(then_node, region.update(conditional_boundary: Range.from_node(then_node)), pins, locals, ivars) + NodeProcessor.process(then_node, region.update(conditional_boundary: Range.from_node(then_node), compound_statement: then_cs), pins, locals, ivars) end else_node = node.children[2] if else_node - pins.push Solargraph::Pin::CompoundStatement.new( + # @sg-ignore Need to add nil check here + else_cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(else_node), closure: region.closure, + compound_statement: region.compound_statement, node: else_node, source: :parser ) + pins.push else_cs # @sg-ignore Need to add nil check here - NodeProcessor.process(else_node, region.update(conditional_boundary: Range.from_node(else_node)), pins, locals, ivars) + NodeProcessor.process(else_node, region.update(conditional_boundary: Range.from_node(else_node), compound_statement: else_cs), pins, locals, ivars) end true diff --git a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb index 6d0f97f7f..7887a8ce5 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb @@ -21,6 +21,7 @@ def process presence: presence, definite: region.conditional_boundary.nil?, conditional_override_boundary: region.conditional_boundary, + compound_statement: region.compound_statement, source: :parser ) process_children diff --git a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb index 0acbf7ee0..24a1a3577 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb @@ -20,6 +20,7 @@ def process type: node.type, location: loc, closure: region.closure, + compound_statement: region.compound_statement, name: name, comments: comments, visibility: :public, @@ -36,7 +37,7 @@ def process source: :parser ) end - process_children region.update(closure: nspin, visibility: :public) + process_children region.update(closure: nspin, visibility: :public, compound_statement: nspin) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb index de85f87b4..c6a8ecdd2 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb @@ -12,7 +12,15 @@ def process # any assignment there isn't guaranteed to have executed lhs, rhs = node.children NodeProcessor.process(lhs, region, pins, locals, ivars) - NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs)), pins, locals, ivars) + # not pushed onto `pins` - see resbody_node.rb for why + rhs_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(rhs), + closure: region.closure, + compound_statement: region.compound_statement, + node: rhs, + source: :parser + ) + NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs), compound_statement: rhs_cs), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb index 85f161bf4..fb3196402 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb @@ -10,7 +10,16 @@ def process new_node = node.updated(node.children[0].type, node.children[0].children + [node.children[1]]) # `x ||= y` only assigns when x is falsy/undefined, so # it's never a guaranteed override of x's prior type - NodeProcessor.process(new_node, region.update(conditional_boundary: Range.from_node(node)), pins, locals, ivars) + # + # not pushed onto `pins` - see resbody_node.rb for why + asgn_cs = Solargraph::Pin::CompoundStatement.new( + location: get_node_location(node), + closure: region.closure, + compound_statement: region.compound_statement, + node: node, + source: :parser + ) + NodeProcessor.process(new_node, region.update(conditional_boundary: Range.from_node(node), compound_statement: asgn_cs), pins, locals, ivars) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb index f88b2c7e4..b9a07e343 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb @@ -30,8 +30,20 @@ def process source: :parser ) end + # not pushed onto `pins` - and/or/orasgn/resbody bodies are + # too common to warrant a pin per occurrence, so only the + # pointer is needed for the compound_statement chain # @sg-ignore Need to add nil check here - NodeProcessor.process(node.children[2], region.update(conditional_boundary: Range.from_node(node.children[2])), pins, locals, ivars) + rescue_body_cs = Solargraph::Pin::CompoundStatement.new( + # @sg-ignore Need to add nil check here + location: node.children[2] ? get_node_location(node.children[2]) : nil, + closure: region.closure, + compound_statement: region.compound_statement, + node: node.children[2], + source: :parser + ) + # @sg-ignore Need to add nil check here + NodeProcessor.process(node.children[2], region.update(conditional_boundary: Range.from_node(node.children[2]), compound_statement: rescue_body_cs), pins, locals, ivars) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb index 9a9d276bf..a431f8180 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb @@ -13,14 +13,16 @@ def process # until statement doesn't create a closure - e.g., # variables created inside can be seen from outside as # well - pins.push Solargraph::Pin::Until.new( + until_pin = Solargraph::Pin::Until.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, node: node, comments: comments_for(node), source: :parser ) - process_children region.update(conditional_boundary: Range.from_node(node)) + pins.push until_pin + process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: until_pin) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb index 60ddb5f18..d1090fca5 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb @@ -8,13 +8,15 @@ class WhenNode < Parser::NodeProcessor::Base include ParserGem::NodeMethods def process - pins.push Solargraph::Pin::CompoundStatement.new( + cs = Solargraph::Pin::CompoundStatement.new( location: get_node_location(node), closure: region.closure, + compound_statement: region.compound_statement, node: node, source: :parser ) - process_children region.update(conditional_boundary: Range.from_node(node)) + pins.push cs + process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: cs) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb index df7841332..97eebe178 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb @@ -17,14 +17,16 @@ def process # while statement doesn't create a closure - e.g., # variables created inside can be seen from outside as # well - pins.push Solargraph::Pin::While.new( + while_pin = Solargraph::Pin::While.new( location: location, closure: region.closure, + compound_statement: region.compound_statement, node: node, comments: comments_for(node), source: :parser ) - process_children region.update(conditional_boundary: Range.from_node(node)) + pins.push while_pin + process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: while_pin) end end end diff --git a/lib/solargraph/parser/region.rb b/lib/solargraph/parser/region.rb index 0dd3d9334..17fe727b3 100644 --- a/lib/solargraph/parser/region.rb +++ b/lib/solargraph/parser/region.rb @@ -32,16 +32,30 @@ class Region # @return [Range, nil] attr_reader :conditional_boundary + # The nearest enclosing CompoundStatement pin (an if/when/while/ + # rescue/&&/||/||= body, a method/block body, or a namespace + # body) - a series of statements/expressions where a later one + # executing implies the earlier ones in the same series + # executed too. Every Closure is also a CompoundStatement, so + # this is a superset of the `closure` chain: it additionally + # includes branch bodies that aren't scopes. + # + # @return [Pin::CompoundStatement] + attr_reader :compound_statement + # @param source [Source] # @param closure [Pin::Closure, nil] # @param scope [Symbol, nil] # @param visibility [Symbol] # @param lvars [Array] # @param conditional_boundary [Range, nil] + # @param compound_statement [Pin::CompoundStatement, nil] def initialize source: Solargraph::Source.load_string(''), closure: nil, - scope: nil, visibility: :public, lvars: [], conditional_boundary: nil + scope: nil, visibility: :public, lvars: [], conditional_boundary: nil, + compound_statement: nil @source = source @closure = closure || Pin::Namespace.new(name: '', location: source.location, source: :parser) + @compound_statement = compound_statement || @closure @scope = scope @visibility = visibility @lvars = lvars @@ -68,15 +82,18 @@ def namespace_pin # @param visibility [Symbol, nil] # @param lvars [Array, nil] # @param conditional_boundary [Range, nil] + # @param compound_statement [Pin::CompoundStatement, nil] # @return [Region] - def update closure: nil, scope: nil, visibility: nil, lvars: nil, conditional_boundary: nil + def update closure: nil, scope: nil, visibility: nil, lvars: nil, conditional_boundary: nil, + compound_statement: nil Region.new( source: source, closure: closure || self.closure, scope: scope || self.scope, visibility: visibility || self.visibility, lvars: lvars || self.lvars, - conditional_boundary: conditional_boundary || self.conditional_boundary + conditional_boundary: conditional_boundary || self.conditional_boundary, + compound_statement: compound_statement || self.compound_statement ) end diff --git a/lib/solargraph/pin/base.rb b/lib/solargraph/pin/base.rb index f7ae58d38..b4a611cf8 100644 --- a/lib/solargraph/pin/base.rb +++ b/lib/solargraph/pin/base.rb @@ -75,6 +75,7 @@ def assert_location_provided # @return [Pin::Closure, nil] def closure + @closure ||= derive_closure_from_compound_statement unless @closure Solargraph.assert_or_log(:closure, "Closure not set on #{self.class} #{name.inspect} from #{source.inspect}") @@ -731,6 +732,25 @@ def equality_fields private + # Fallback for pins with no directly-assigned @closure: walk the + # CompoundStatement parent chain (present only on + # CompoundStatement-family pins - Closure, While, Until, etc.) + # until an ancestor is_a?(Closure). Every pin built through + # Region-threaded node processors already gets an explicit + # closure:, so this only matters for a pin constructed purely + # from a compound_statement chain with no closure: override. + # + # @return [Pin::Closure, nil] + def derive_closure_from_compound_statement + return nil unless is_a?(CompoundStatement) + + # @sg-ignore flow sensitive typing doesn't narrow self past an is_a? guard + cs = compound_statement + # @sg-ignore flow sensitive typing doesn't narrow self past an is_a? guard + cs = cs.compound_statement while cs && !cs.is_a?(Closure) + cs + end + # @return [void] def parse_comments # HACK: Avoid a NoMethodError on nil with empty overload tags diff --git a/lib/solargraph/pin/base_variable.rb b/lib/solargraph/pin/base_variable.rb index 7ef5aa42c..3cfeac93e 100644 --- a/lib/solargraph/pin/base_variable.rb +++ b/lib/solargraph/pin/base_variable.rb @@ -20,6 +20,16 @@ class BaseVariable < Base # @return [Range, nil] attr_reader :conditional_override_boundary + # The CompoundStatement pin this variable's (re)assignment was + # made within - i.e. Region#compound_statement at the point of + # assignment. Not yet consulted by any override logic (that's + # conditional_override_boundary's job today); threaded through + # now so a future chain-walk-based override check has the data + # already flowing. + # + # @return [Pin::CompoundStatement, nil] + attr_reader :compound_statement + # @param return_type [ComplexType, nil] # @param assignment [Parser::AST::Node, nil] First assignment # that was made to this variable @@ -67,11 +77,15 @@ class BaseVariable < Base # position inside this range may still treat the assignment # as an override rather than merely unioning it with earlier # possible types. + # @param compound_statement [Pin::CompoundStatement, nil] The + # CompoundStatement this variable's (re)assignment was made + # within. # @param [Hash{Symbol => Object}] splat def initialize assignment: nil, assignments: [], mass_assignment: nil, presence: nil, return_type: nil, intersection_return_type: nil, exclude_return_type: nil, definite: true, conditional_override_boundary: nil, + compound_statement: nil, **splat super(**splat) @assignments = (assignment.nil? ? [] : [assignment]) + assignments @@ -83,6 +97,7 @@ def initialize assignment: nil, assignments: [], mass_assignment: nil, @presence = presence @definite = definite @conditional_override_boundary = conditional_override_boundary + @compound_statement = compound_statement end # @param presence [Range] diff --git a/lib/solargraph/pin/compound_statement.rb b/lib/solargraph/pin/compound_statement.rb index 39d9cf2d5..dec7ab994 100644 --- a/lib/solargraph/pin/compound_statement.rb +++ b/lib/solargraph/pin/compound_statement.rb @@ -44,11 +44,54 @@ module Pin class CompoundStatement < Pin::Base attr_reader :node + # The immediately enclosing CompoundStatement, if any - nil only + # for the synthetic root Namespace Region creates for top-level + # code. Since Closure < CompoundStatement, walking this chain + # until an ancestor is_a?(Closure) is how Base#closure is + # derived when a pin has no directly-assigned @closure. + # + # @return [Pin::CompoundStatement, nil] + attr_reader :compound_statement + # @param node [Parser::AST::Node, nil] + # @param compound_statement [Pin::CompoundStatement, nil] # @param [Hash{Symbol => Object}] splat - def initialize node: nil, **splat + def initialize node: nil, compound_statement: nil, **splat super(**splat) @node = node + @compound_statement = compound_statement + end + + # @param other [self] + # @param attrs [Hash{Symbol => Object}] + # @return [self] + def combine_with other, attrs = {} + new_attrs = { compound_statement: combine_compound_statement(other) }.merge(attrs) + super(other, new_attrs) + end + + # Bare CompoundStatement pins (if/when/rescue/&&/||/||= bodies) + # all share name == '', so the same-name-assertion in + # Base#choose_pin_attr_with_same_name (used by #combine_closure) + # would be meaningless noise here - pick by location instead, + # mirroring BaseVariable#combine_closure. + # + # @param other [self] + # @return [Pin::CompoundStatement, nil] + def combine_compound_statement other + return compound_statement if compound_statement == other.compound_statement + return compound_statement || other.compound_statement if compound_statement.nil? || other.compound_statement.nil? + + # @sg-ignore flow sensitive typing needs to handle attrs + if compound_statement.location.nil? || other.compound_statement.location.nil? + # @sg-ignore flow sensitive typing needs to handle attrs + return compound_statement.location.nil? ? other.compound_statement : compound_statement + end + + # @sg-ignore flow sensitive typing needs to handle attrs + return compound_statement if compound_statement.location <= other.compound_statement.location + + other.compound_statement end end end diff --git a/spec/pin/compound_statement_spec.rb b/spec/pin/compound_statement_spec.rb new file mode 100644 index 000000000..4cc9013a0 --- /dev/null +++ b/spec/pin/compound_statement_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +describe Solargraph::Pin::CompoundStatement do + # Every pin built through Region-threaded node processors still gets + # an explicit `closure:`, so `Pin::Base#closure` returns the stored + # value, not the derived one - the derivation only kicks in as a + # fallback. These specs check the two would agree anyway, so a + # future node processor that updates one threading (closure: or + # compound_statement:) without the other gets caught here instead + # of silently drifting. + def derive_closure pin + cs = pin.compound_statement + cs = cs.compound_statement while cs && !cs.is_a?(Solargraph::Pin::Closure) + cs + end + + it 'agrees with the stored closure for compound statements nested in a method, if, and while' do + source_map = Solargraph::SourceMap.load_string(%( + class Foo + def bar(flag) + if flag + while flag + local = 1 + end + end + end + end + )) + + compound_statement_pins = source_map.pins.select { |pin| pin.is_a?(described_class) } + expect(compound_statement_pins).not_to be_empty + + compound_statement_pins.each do |pin| + expect(derive_closure(pin)).to eq(pin.closure), "mismatch for #{pin.inspect}" + end + end + + it 'agrees with the stored closure for compound statements nested in a block' do + source_map = Solargraph::SourceMap.load_string(%( + class Foo + def bar + [1].each do |i| + if i + local = i + end + end + end + end + )) + + compound_statement_pins = source_map.pins.select { |pin| pin.is_a?(described_class) } + expect(compound_statement_pins).not_to be_empty + + compound_statement_pins.each do |pin| + expect(derive_closure(pin)).to eq(pin.closure), "mismatch for #{pin.inspect}" + end + end + + it 'derives the enclosing method as closure for a bare CompoundStatement built only with compound_statement:' do + source_map = Solargraph::SourceMap.load_string(%( + class Foo + def bar + 1 + end + end + )) + method_pin = source_map.pins.find { |pin| pin.is_a?(Solargraph::Pin::Method) && pin.name == 'bar' } + + bare_pin = described_class.new( + location: method_pin.location, + compound_statement: method_pin, + source: :parser + ) + + expect(bare_pin.closure).to eq(method_pin) + end +end From e32406566775b3b58fee2ba6aedb44c18ccec652 Mon Sep 17 00:00:00 2001 From: Vince Broz Date: Fri, 14 Aug 2026 14:35:37 -0400 Subject: [PATCH 2/2] Rewrite override eligibility to walk the compound_statement chain BaseVariable#definite_reaches? no longer compares a query Location against a separately-stored conditional_override_boundary Range. Instead it checks whether the location falls within this pin's own compound_statement's location range - the CompoundStatement pin already carries that range, and since a nested CompoundStatement's location is always a subrange of its parent's, this single containment check already accounts for arbitrarily nested branches without needing to walk the chain further. This removes the duplicate bookkeeping the original PR 1282 fix introduced: Region#conditional_boundary (a Range) and BaseVariable#conditional_override_boundary are gone, along with the Range.from_node(...) computation every conditional-construct node processor performed to populate them - that range is now read directly off the compound_statement pin instead of being computed a second time. lvasgn_node.rb's `definite` computation goes back to a plain Region#conditional boolean rather than `conditional_boundary.nil?` (and was briefly, incorrectly, tried as `compound_statement.is_a? (Closure)` during this rewrite - reverted because a block's body pin IS a Closure, for variable-scoping purposes, despite running zero or many times, which is exactly the case `conditional_boundary`/`conditional` exists to distinguish). Every closure-creating node processor (def_node.rb, defs_node.rb, namespace_node.rb) now explicitly resets `conditional: false` for its body, since entering a fresh method/namespace scope always runs its body top-to-bottom regardless of how the closure itself was reached, unlike a block. Added: - A loop-ordering regression test confirming a reassignment inside a while body doesn't affect a reference textually before it. - combine_with specs for Pin::CompoundStatement covering the location-based tiebreak and the nil-vs-non-nil case. Verified: full suite (1638 examples, 0 failures), typecheck self-check diffed against the pre-fix baseline (587 problems vs. 591 baseline - net fewer, since deleting the Range.from_node calls also removed several instances of the pre-existing nilable-AST-child pattern already tolerated throughout these files). Combines what were originally staged as two follow-up PRs into one - see https://github.com/castwide/solargraph/pull/1282 for the base fix and design discussion. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YbhZvdCv7xdziXyKJiPuGk --- .../parser_gem/node_processors/and_node.rb | 2 +- .../parser_gem/node_processors/block_node.rb | 2 +- .../parser_gem/node_processors/def_node.rb | 2 +- .../parser_gem/node_processors/defs_node.rb | 2 +- .../parser_gem/node_processors/if_node.rb | 4 +- .../parser_gem/node_processors/lvasgn_node.rb | 3 +- .../node_processors/namespace_node.rb | 2 +- .../parser_gem/node_processors/or_node.rb | 2 +- .../parser_gem/node_processors/orasgn_node.rb | 2 +- .../node_processors/resbody_node.rb | 2 +- .../parser_gem/node_processors/until_node.rb | 2 +- .../parser_gem/node_processors/when_node.rb | 2 +- .../parser_gem/node_processors/while_node.rb | 2 +- lib/solargraph/parser/region.rb | 42 ++++++++-------- lib/solargraph/pin/base_variable.rb | 50 ++++++++----------- spec/pin/compound_statement_spec.rb | 24 +++++++++ spec/type_checker/levels/strong_spec.rb | 17 +++++++ 17 files changed, 99 insertions(+), 63 deletions(-) diff --git a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb index 7e1da26b1..40acf6354 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/and_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/and_node.rb @@ -20,7 +20,7 @@ def process node: rhs, source: :parser ) - NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs), compound_statement: rhs_cs), pins, locals, ivars) + NodeProcessor.process(rhs, region.update(compound_statement: rhs_cs, conditional: true), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb index 232cf0682..cf210cb5d 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/block_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/block_node.rb @@ -32,7 +32,7 @@ def process # a block's body may execute zero or multiple times (e.g. # Enumerable#each), so an assignment inside it is never # guaranteed to have executed - process_children region.update(closure: block_pin, conditional_boundary: Range.from_node(node), compound_statement: block_pin) + process_children region.update(closure: block_pin, compound_statement: block_pin, conditional: true) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb index b6e6137d6..c93f0f80f 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/def_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/def_node.rb @@ -52,7 +52,7 @@ def process else pins.push methpin end - process_children region.update(closure: methpin, scope: methpin.scope, compound_statement: methpin) + process_children region.update(closure: methpin, scope: methpin.scope, compound_statement: methpin, conditional: false) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb index 9690fcf87..70f058334 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/defs_node.rb @@ -30,7 +30,7 @@ def process node: node, source: :parser ) - process_children region.update(closure: pins.last, scope: :class, compound_statement: pins.last) + process_children region.update(closure: pins.last, scope: :class, compound_statement: pins.last, conditional: false) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb index db303d733..6120a6ed6 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/if_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/if_node.rb @@ -35,7 +35,7 @@ def process ) pins.push then_cs # @sg-ignore Need to add nil check here - NodeProcessor.process(then_node, region.update(conditional_boundary: Range.from_node(then_node), compound_statement: then_cs), pins, locals, ivars) + NodeProcessor.process(then_node, region.update(compound_statement: then_cs, conditional: true), pins, locals, ivars) end else_node = node.children[2] @@ -50,7 +50,7 @@ def process ) pins.push else_cs # @sg-ignore Need to add nil check here - NodeProcessor.process(else_node, region.update(conditional_boundary: Range.from_node(else_node), compound_statement: else_cs), pins, locals, ivars) + NodeProcessor.process(else_node, region.update(compound_statement: else_cs, conditional: true), pins, locals, ivars) end true diff --git a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb index 7887a8ce5..c3eb6dfac 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/lvasgn_node.rb @@ -19,8 +19,7 @@ def process assignment: node.children[1], comments: comments_for(node), presence: presence, - definite: region.conditional_boundary.nil?, - conditional_override_boundary: region.conditional_boundary, + definite: !region.conditional, compound_statement: region.compound_statement, source: :parser ) diff --git a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb index 24a1a3577..a38762a57 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/namespace_node.rb @@ -37,7 +37,7 @@ def process source: :parser ) end - process_children region.update(closure: nspin, visibility: :public, compound_statement: nspin) + process_children region.update(closure: nspin, visibility: :public, compound_statement: nspin, conditional: false) end private diff --git a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb index c6a8ecdd2..e64270045 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/or_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/or_node.rb @@ -20,7 +20,7 @@ def process node: rhs, source: :parser ) - NodeProcessor.process(rhs, region.update(conditional_boundary: Range.from_node(rhs), compound_statement: rhs_cs), pins, locals, ivars) + NodeProcessor.process(rhs, region.update(compound_statement: rhs_cs, conditional: true), pins, locals, ivars) FlowSensitiveTyping.new(locals, ivars, diff --git a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb index fb3196402..271286645 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/orasgn_node.rb @@ -19,7 +19,7 @@ def process node: node, source: :parser ) - NodeProcessor.process(new_node, region.update(conditional_boundary: Range.from_node(node), compound_statement: asgn_cs), pins, locals, ivars) + NodeProcessor.process(new_node, region.update(compound_statement: asgn_cs, conditional: true), pins, locals, ivars) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb index b9a07e343..c5f699e09 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/resbody_node.rb @@ -43,7 +43,7 @@ def process source: :parser ) # @sg-ignore Need to add nil check here - NodeProcessor.process(node.children[2], region.update(conditional_boundary: Range.from_node(node.children[2]), compound_statement: rescue_body_cs), pins, locals, ivars) + NodeProcessor.process(node.children[2], region.update(compound_statement: rescue_body_cs, conditional: true), pins, locals, ivars) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb index a431f8180..47a2d3570 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/until_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/until_node.rb @@ -22,7 +22,7 @@ def process source: :parser ) pins.push until_pin - process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: until_pin) + process_children region.update(compound_statement: until_pin, conditional: true) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb index d1090fca5..74a887791 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/when_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/when_node.rb @@ -16,7 +16,7 @@ def process source: :parser ) pins.push cs - process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: cs) + process_children region.update(compound_statement: cs, conditional: true) end end end diff --git a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb index 97eebe178..866ad0303 100644 --- a/lib/solargraph/parser/parser_gem/node_processors/while_node.rb +++ b/lib/solargraph/parser/parser_gem/node_processors/while_node.rb @@ -26,7 +26,7 @@ def process source: :parser ) pins.push while_pin - process_children region.update(conditional_boundary: Range.from_node(node), compound_statement: while_pin) + process_children region.update(compound_statement: while_pin, conditional: true) end end end diff --git a/lib/solargraph/parser/region.rb b/lib/solargraph/parser/region.rb index 17fe727b3..fbf34a069 100644 --- a/lib/solargraph/parser/region.rb +++ b/lib/solargraph/parser/region.rb @@ -21,17 +21,6 @@ class Region # @return [Array] attr_reader :lvars - # The source range of the nearest enclosing construct that may - # be skipped at runtime (e.g., an if/while/until body), meaning - # an assignment made at the current position isn't guaranteed - # to have executed at a later position - except at a position - # that itself falls within this same range, where the - # assignment is still guaranteed to dominate. nil if the - # current position isn't inside any such construct. - # - # @return [Range, nil] - attr_reader :conditional_boundary - # The nearest enclosing CompoundStatement pin (an if/when/while/ # rescue/&&/||/||= body, a method/block body, or a namespace # body) - a series of statements/expressions where a later one @@ -43,23 +32,36 @@ class Region # @return [Pin::CompoundStatement] attr_reader :compound_statement + # True if the current position may be skipped, or run zero or + # multiple times, at runtime - e.g. inside an if/while/until/ + # rescue/&&/||/||= body, or inside a block body (which, despite + # its Block pin being a Closure like Method/Namespace, may run + # zero or many times depending on the method it's passed to, + # unlike a method/namespace body which always runs exactly once + # when reached). Not derivable from `compound_statement.is_a? + # (Closure)` alone for that reason - Block is the case where + # "is a Closure" and "unconditionally executes" diverge. + # + # @return [Boolean] + attr_reader :conditional + # @param source [Source] # @param closure [Pin::Closure, nil] # @param scope [Symbol, nil] # @param visibility [Symbol] # @param lvars [Array] - # @param conditional_boundary [Range, nil] # @param compound_statement [Pin::CompoundStatement, nil] + # @param conditional [Boolean] def initialize source: Solargraph::Source.load_string(''), closure: nil, - scope: nil, visibility: :public, lvars: [], conditional_boundary: nil, - compound_statement: nil + scope: nil, visibility: :public, lvars: [], + compound_statement: nil, conditional: false @source = source @closure = closure || Pin::Namespace.new(name: '', location: source.location, source: :parser) @compound_statement = compound_statement || @closure @scope = scope @visibility = visibility @lvars = lvars - @conditional_boundary = conditional_boundary + @conditional = conditional end # @return [String, nil] @@ -81,19 +83,19 @@ def namespace_pin # @param scope [Symbol, nil] # @param visibility [Symbol, nil] # @param lvars [Array, nil] - # @param conditional_boundary [Range, nil] # @param compound_statement [Pin::CompoundStatement, nil] + # @param conditional [Boolean, nil] # @return [Region] - def update closure: nil, scope: nil, visibility: nil, lvars: nil, conditional_boundary: nil, - compound_statement: nil + def update closure: nil, scope: nil, visibility: nil, lvars: nil, + compound_statement: nil, conditional: nil Region.new( source: source, closure: closure || self.closure, scope: scope || self.scope, visibility: visibility || self.visibility, lvars: lvars || self.lvars, - conditional_boundary: conditional_boundary || self.conditional_boundary, - compound_statement: compound_statement || self.compound_statement + compound_statement: compound_statement || self.compound_statement, + conditional: conditional.nil? ? self.conditional : conditional ) end diff --git a/lib/solargraph/pin/base_variable.rb b/lib/solargraph/pin/base_variable.rb index 3cfeac93e..38d409b34 100644 --- a/lib/solargraph/pin/base_variable.rb +++ b/lib/solargraph/pin/base_variable.rb @@ -17,15 +17,10 @@ class BaseVariable < Base # @return [Boolean] attr_reader :definite - # @return [Range, nil] - attr_reader :conditional_override_boundary - # The CompoundStatement pin this variable's (re)assignment was # made within - i.e. Region#compound_statement at the point of - # assignment. Not yet consulted by any override logic (that's - # conditional_override_boundary's job today); threaded through - # now so a future chain-walk-based override check has the data - # already flowing. + # assignment. Used by #definite_reaches? to decide whether a + # non-definite assignment still dominates a given reference. # # @return [Pin::CompoundStatement, nil] attr_reader :compound_statement @@ -68,23 +63,17 @@ class BaseVariable < Base # reassignment's type may safely override a variable's # previously declared/inferred type instead of merely being # unioned with it. - # @param conditional_override_boundary [Range, nil] When - # `definite` is false because this assignment is inside a - # conditional branch or loop, the source range of that - # construct's body - i.e., the extent within which this - # assignment, though not globally guaranteed, is still - # guaranteed to dominate any reference. A reference at a - # position inside this range may still treat the assignment - # as an override rather than merely unioning it with earlier - # possible types. # @param compound_statement [Pin::CompoundStatement, nil] The # CompoundStatement this variable's (re)assignment was made - # within. + # within. When `definite` is false, a reference whose location + # falls within this pin's own range may still treat the + # assignment as an override rather than merely unioning it + # with earlier possible types - see #definite_reaches?. # @param [Hash{Symbol => Object}] splat def initialize assignment: nil, assignments: [], mass_assignment: nil, presence: nil, return_type: nil, intersection_return_type: nil, exclude_return_type: nil, - definite: true, conditional_override_boundary: nil, + definite: true, compound_statement: nil, **splat super(**splat) @@ -96,7 +85,6 @@ def initialize assignment: nil, assignments: [], mass_assignment: nil, @exclude_return_type = exclude_return_type @presence = presence @definite = definite - @conditional_override_boundary = conditional_override_boundary @compound_statement = compound_statement end @@ -127,7 +115,7 @@ def reset_generated! # @param location [Location, nil] The position being resolved, # if known - used to decide whether a not-globally-definite # `other` should still override us because the position falls - # within `other`'s conditional_override_boundary. + # within `other`'s compound_statement. def combine_with other, attrs = {}, location: nil new_assignments = combine_assignments(other, location) new_attrs = attrs.merge({ @@ -402,7 +390,7 @@ def within_own_assignment? other_loc # @param other [self] # @param location [Location, nil] The position being resolved, # if known - lets a conditional `other` still override us when - # `location` falls inside `other`'s conditional_override_boundary. + # `location` falls within `other`'s compound_statement. # @return [Boolean] def override_assignments? other, location = nil (other.definite || other.definite_reaches?(location)) && other.closure == closure && @@ -413,18 +401,24 @@ def override_assignments? other, location = nil # True if this pin's assignment, though not globally definite, # is still guaranteed to dominate `location` - i.e., `location` - # falls inside the conditional construct's body that this - # assignment was made in, so no earlier branch exit could have - # skipped it by the time `location` is reached. + # falls within the CompoundStatement body (an if/while/until/ + # rescue/&&/||/||= branch) this assignment was made in, so no + # earlier branch exit could have skipped it by the time + # `location` is reached. A nested CompoundStatement's location + # is always a subrange of its parent's, so this single + # containment check already accounts for arbitrarily nested + # branches without walking the compound_statement chain further. # # @param location [Location, nil] # @return [Boolean] def definite_reaches? location - boundary = conditional_override_boundary - return false unless location && boundary + cs = compound_statement + return false unless location && cs&.location&.range - location.filename == self.location&.filename && - boundary.contain?(location.range.start) + # @sg-ignore flow sensitive typing needs to handle attrs + cs.location.filename == location.filename && + # @sg-ignore flow sensitive typing needs to handle attrs + cs.location.range.contain?(location.range.start) end private diff --git a/spec/pin/compound_statement_spec.rb b/spec/pin/compound_statement_spec.rb index 4cc9013a0..5a6a1e0d9 100644 --- a/spec/pin/compound_statement_spec.rb +++ b/spec/pin/compound_statement_spec.rb @@ -74,4 +74,28 @@ def bar expect(bare_pin.closure).to eq(method_pin) end + + describe '#combine_with' do + let(:earlier_location) { Solargraph::Location.new('test.rb', Solargraph::Range.from_to(1, 0, 3, 0)) } + let(:later_location) { Solargraph::Location.new('test.rb', Solargraph::Range.from_to(5, 0, 7, 0)) } + + it 'prefers the compound_statement with the earlier location' do + earlier_cs = described_class.new(location: earlier_location, source: :parser) + later_cs = described_class.new(location: later_location, source: :parser) + pin1 = described_class.new(location: earlier_location, compound_statement: earlier_cs, source: :parser) + pin2 = described_class.new(location: later_location, compound_statement: later_cs, source: :parser) + + expect(pin1.combine_with(pin2).compound_statement).to eq(earlier_cs) + expect(pin2.combine_with(pin1).compound_statement).to eq(earlier_cs) + end + + it 'prefers a non-nil compound_statement over a nil one' do + cs = described_class.new(location: earlier_location, source: :parser) + pin1 = described_class.new(location: earlier_location, compound_statement: nil, source: :parser) + pin2 = described_class.new(location: earlier_location, compound_statement: cs, source: :parser) + + expect(pin1.combine_with(pin2).compound_statement).to eq(cs) + expect(pin2.combine_with(pin1).compound_statement).to eq(cs) + end + end end diff --git a/spec/type_checker/levels/strong_spec.rb b/spec/type_checker/levels/strong_spec.rb index 728b52d7c..195e862c1 100644 --- a/spec/type_checker/levels/strong_spec.rb +++ b/spec/type_checker/levels/strong_spec.rb @@ -956,6 +956,23 @@ def conditional_reassign(str, num, flag) expect(checker.problems.map(&:message)).to eq([]) end + it 'does not let a loop-body reassignment override a reference textually before it' do + checker = type_checker(%( + # @param str [String] + # @param num [Integer] + # @param flag [Boolean] + # @return [void] + def loop_reassign(str, num, flag) + local = num + while flag + local.abs + local = str + end + end + )) + expect(checker.problems.map(&:message)).to eq([]) + end + it 'updates a local variable type after reassignment to a different literal type' do checker = type_checker(%( # @return [void]