Fix Pin::Base#== missing presence, add regression coverage - #1293
Fix Pin::Base#== missing presence, add regression coverage#1293apiology wants to merge 9 commits into
Conversation
raise/fail/abort are declared in RBS as returning `bot`, meaning the expression never actually produces a value and is therefore compatible with any expected type. RbsTranslator collapsed Bottom into the same 'undefined' tag used for Any, so raise-only method bodies failed typecheck with "return type could not be inferred" instead of being compared against the declared @return tag, and bot values leaking into generic resolution (e.g. Array#fetch's block form were never recognized as auto-compatible with the expected type. Fixes castwide#1276 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Ncz9vtnjnpYpotyVRG4Eq EOF )
Pin::Base#== compares #location but not #presence. combine_with results choose the earliest assignment's #location, so two combined pins covering a different number of assignments to the same variable can share #location while covering different #presence ranges - e.g. one pin combined through a variable's first reassignment, another combined through its second. Any caller keying off of #== (e.g. Array#include?) treated these as the same pin. BaseVariable#== now also compares presence, intersection_return_type, and exclude_return_type. Adds two regression specs: - spec/pin/base_variable_spec.rb: directly exercises the equality gap above - fails without the fix, passes with it. - spec/type_checker/levels/strong_spec.rb: a 13-line repro (castwide#1288 (comment)) where this equality gap, combined with in-flight flow-sensitive-typing work (castwide#1258, castwide#1282), produces a false "Unresolved call" via Chain's inference recursion guard. Not currently reachable on master alone (verified neither castwide#1258 nor castwide#1282 reproduces it in isolation, only the two combined) - kept as a standing guard so whatever future combination reintroduces the failure mode gets caught regardless of merge order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT7qJXsRVt8W7ULFmwjvLj
|
Claude: This regresses Solargraph's own self-hosted strong typecheck. Reproduces standalone on this branch ( gives 11 problems, including two not present on the parent commit ( Those lines are in # @type [Pin::Base, nil]
val1 = send(attr)
# @type [Pin::Base, nil]
val2 = other.send(attr)
raise "Expected pin for #{attr} on\n#{inspect},\ngot #{val1.inspect}" unless val1.nil? || val1.is_a?(Pin::Base)
The causal chain past that observation is not fully traced. Best guess, unconfirmed: Note: |
BaseVariable#== now compares presence (97127a9), so pins produced by independent flow-sensitive-typing facts at the same location (e.g. the two operands of x.nil? || x.is_a?(Foo)) no longer get deduplicated via Array#include? before being combined. Combining them unions their exclude_return_type sets, which can end up excluding every member of the declared type - collapsing ComplexType#exclude's result to undefined even though the type was well-defined before exclusion. That surfaced as new self-hosted strong-typecheck failures on lib/solargraph/pin/base.rb ("Unresolved call to inspect") reported at castwide#1293 (comment), introduced by 97127a9 without touching that file at all. ComplexType#exclude now treats an exclusion built from more than one excluded type as a no-op when it would otherwise remove every possible type, since that combination reflects contradictory flow facts (unreachable/defensive code) rather than a real type error. A single-source exhaustive exclusion (e.g. a variable directly assigned nil despite a non-nilable declared type) still collapses to undefined as before. Verified against the parent commit (8fda633): whole-project solargraph typecheck --level strong problem count drops from 533 to 531, an exact match after removing the two new false positives with no other diff. Full test suite: 1626 examples, 0 failures.
@todo comment only - explains that once castwide#1277 (RBS bottom type) lands, ComplexType#exclude could tag `bot` for any exhausted exclusion instead of just the multi-source case handled here, making this branch's special-casing unnecessary. The two PRs are otherwise independent: this touches only #exclude, castwide#1277 touches #qualify and elsewhere in complex_type.rb, so they merge cleanly in either order with no coordination needed.
Pin::Base#== previously ignored a variable pin's #presence, so two combine_with results covering different numbers of reassignments to the same variable could share a #location and compare equal even though they cover different value ranges. Fixed by comparing presence (and narrowed/exclude_return_type) as well. Includes the PR's own follow-up commit (ComplexType#exclude no-op for a multi-source exclusion) and latest comment-only commit. Also fixes check_gem_version.rb's `# @type [Gem::Dependency, nil]` annotation to the correct `Gem::NameTuple` (search_for_dependency actually returns Gem::NameTuple tuples, not Gem::Dependency). The presence fix surfaces 4 new `solargraph typecheck --level strong` findings (3 in pin/base.rb, 1 in check_gem_version.rb) via ComplexType#exclude's single-excluded-type case, which the PR's own multi-type fix doesn't cover. Accepted into this branch's known baseline pending an upstream fix.
Companion to ComplexType::BOT for code that needs a bare UniqueType (e.g. building a types array) rather than a full ComplexType. Must use rooted: true to stay == to ComplexType::BOT.first - #bot?/#tag/#to_s don't read @rooted, so a rooted: false construction (matching ::UNDEFINED's pattern) would look identical everywhere except #== and anything that relies on it (Array#uniq, Array#-, Set membership, pin dedup), which is exactly the kind of bug this branch's own history has already hit once.
ComplexType#exclude previously only collapsed to `undefined` for single-item exclusions (e.g. `!x.is_a?(Foo)` narrowing a declared Foo to nothing), leaving the multi-item case as a same-type no-op. Now that castwide#1277 gives RBS's bottom type its own tag, both cases can collapse to `bot` instead - it correctly signals "this code is unreachable" rather than "this type is unknown." That surfaces a second gap: Call#resolve had no path for a bot-typed receiver, so any method call chained onto one came back "Unresolved call to <method> on bot" - a false positive, since bot is a subtype of everything and the call is unreachable code anyway. Adds a bot? branch that returns a DuckMethod pin (return type bot) so downstream resolution has a real Pin::Method to work with while bot keeps propagating. Fail-first verified: reverting just the Call#resolve change while keeping the exclude change reproduces "Unresolved call to length on bot"; with both, the new strict-level spec passes.
# Conflicts: # lib/solargraph/source/chain/call.rb
The integration-branch merge of this commit surfaced (via Solargraph/strong, which runs with SOLARGRAPH_ASSERTS=on) "Closure not set on Solargraph::Pin::DuckMethod ... from :chain": the bot? DuckMethod pin never passed closure:, which is harmless until something downstream reads Pin::Base#closure, which asserts under strict mode when unset. Passes name_pin.closure through, matching the fix already applied to the integration branch's equivalent method_stack_pins code path.
…d pin # Conflicts: # lib/solargraph/source/chain/call.rb
Summary
Pin::Base#==compares#locationbut not#presence.combine_withresults choose the earliest assignment's#location, so two combined pins covering a different number of assignments to the same variable can share#locationwhile covering different#presenceranges — e.g. one pin combined through a variable's first reassignment, another combined through its second. Any caller keying off of#==(e.g.Array#include?) treated these as the same pin.BaseVariable#==now also comparespresence,intersection_return_type, andexclude_return_type.Regression coverage
spec/pin/base_variable_spec.rb: directly exercises the equality gap above — fails without the fix, passes with it.spec/type_checker/levels/strong_spec.rb: a 13-line repro (Fix generic binding through a cross-file @!parse stub #1288 (comment)) where this equality gap, combined with in-flight flow-sensitive-typing work (Narrow repeated calls to the same attr_reader-style accessor #1258, Update a parameter's flow-sensitive type after reassignment to a non-literal type #1282), produces a false "Unresolved call" viaChain's inference recursion guard. Not currently reachable on master alone — verified neither Narrow repeated calls to the same attr_reader-style accessor #1258 nor Update a parameter's flow-sensitive type after reassignment to a non-literal type #1282 reproduces it in isolation, only the two combined. Kept as a standing regression guard so whatever future combination reintroduces the failure mode gets caught, regardless of merge order.Test plan
bundle exec rspec— 1626 examples, 0 failures, 60 pending (pre-existing)bundle exec rubocopon touched files — cleanbundle exec solargraph typecheckon touched files — clean (one pre-existing warning on an unmodified line)