Fix 1286 generic cross file parse - #76
Open
apiology wants to merge 14 commits into
Open
Conversation
A class defined in a gem and re-opened elsewhere via @!parse to add @Generic tags produced two pins for the same namespace/method path. ApiMap picked whichever pin loaded first - the gem's own pins load before workspace pins, so the annotation's @Generic declaration and overridden return types were silently ignored. - ApiMap#namespace_pin_for_generics prefers the pin that actually declares generics over an arbitrary .first. - ApiMap::Store#get_methods combines same-path method pins (skipping aliases, since merging an alias pin with a non-alias pin at the same path produces a pin #resolve_method_alias can't trace back to its target, which raises under SOLARGRAPH_ASSERTS=on). Fixes castwide#1286 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LtM8dkYTeQEiyhFu1NLZCB
Confirms combining many pins for the same method path completes quickly rather than hanging - related to the concern that prompted castwide#1186 and castwide#1195 (see comment in spec for why this doesn't reproduce that specific bug, and where the precise regression guard for it already lives). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LtM8dkYTeQEiyhFu1NLZCB
Pin::DuckMethod pins (created for `#method_name` duck-type tags, e.g. `@param x [#to_s]`) are constructed without a closure. Method#typify called `closure.gates` unconditionally once see_reference or typify_from_super resolved a type, raising `NoMethodError: undefined method 'gates' for nil` whenever that path was hit on such a pin. Guard it the same way other call sites in this file already do: `closure&.gates || ['']`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cj8BgHwzHKFsD51H9TfPD5
Cut docstrings and spec comments to the review's per-comment budget (2-6 lines): the combine_duplicate_method_pins and namespace_pin_for_generics docstrings, the alias-combination spec comment, and the "combines many same-path pins" spec comment, which had narrated the history of issues castwide#1186, castwide#1195, and castwide#1238 instead of stating the current constraint. Also replaces "plain" in the cross-file generics spec with what actually makes that fixture plain: no @Generic tag, no @!parse stub.
solargraph typecheck against any project using Forwardable dies before
emitting a single diagnostic:
lib/solargraph/pin/delegated_method.rb:25:in 'initialize':
either :method or :receiver is required (ArgumentError)
from ApiMap#load_with_cache -> catalog -> Store#update ->
combine_duplicate_method_pins -> Pin::Method#combine_with ->
Pin::Base#combine_with.
Pin::Base#combine_with rebuilds the merged pin with
self.class.new(**new_attrs), and new_attrs carries only generic pin
attributes (location, name, closure, comments, visibility, signatures).
Pin::DelegatedMethod#initialize requires exactly one of :method /
:receiver and receives neither, so combining two same-path
DelegatedMethod pins is structurally impossible. This went live when
castwide#1311 started minting DelegatedMethod pins for
def_delegators, which makes duplicate-path groups routine.
combine_duplicate_method_pins already skips groups containing a
Pin::MethodAlias for the same class of reason (a merged pin can't
represent the alias target); DelegatedMethod was never added to that
guard. Extend it rather than teaching DelegatedMethod to merge: a pin
constructed from a :receiver that has since resolved holds both
@receiver_chain and @resolved_method, while initialize forbids passing
both, so any combine_with override would have to discard one pin's
delegation target. When the two pins delegate to different receivers
(reopened class, source-vs-RBS duplicate) that loses information
silently. Keeping both pins preserves it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H1FEjW6nMpZrWPmeWX9miT
Method docstring was 6 prose lines over the 1-3 line budget; two spec comments restated the example name or the assertion in different words. Neither adds anything the code or the it-title doesn't already say.
…rse' into push-1288-undercover
The cross-file @!parse spec called its gem-side source plain_impl and then spent two comment lines explaining what "plain" meant. Rename it to gem_source so the name carries that, and shorten the comment. "pin" as a verb in store_spec collides with Solargraph::Pin, the domain object the surrounding spec is about; say "assert" instead. The combine_duplicate_method_pins docstring ran to seven lines counting its @PARAM and @return tags. Fold the fourth prose line into the third.
namespace_pin_for_generics carried a @type on candidates. Dropping it shows what it was covering: lib/solargraph/api_map.rb:822: Unresolved call to generics on Solargraph::Pin::Base select with an is_a? block does not narrow the returned element type, so the Array stays Array<Pin::Base>. That is a real inference gap and distinct from the flow-sensitive-local narrowing tracked in 1241, 1251, 1254 and 1296; nothing upstream covers it. Replace the cast with an @sg-ignore naming it, so the finding survives rather than being hidden. The DuckMethod example asserted only that typify did not raise. It now asserts the pin has no closure and still resolves to ::String through the core Object#to_s, which is the property that would actually regress. combine_duplicate_method_pins named neither situation that produces duplicate same-path pins; say what they are. The alias and DelegatedMethod reasoning it carried is already stated by the specs that cover those two cases.
The two Timeout.timeout(5) wrappers in store_spec dated from when Pin::Method#combine_same_type_arity_signatures could blow up exponentially. That is fixed and merged, so the wrappers guard nothing and the example is renamed for what it actually asserts: 30 same-path pins combine into one. The timeout require goes with them. namespace_pin_for_generics gains a @todo weighing merging duplicate namespace pins against picking one of them. The DuckMethod example said "core method return type", which names nothing in particular. It resolves through Object#to_s to String; say that.
A gem's plain &block signature and a workspace @!parse stub's yield-typed one only differ in the block's own type_arity, so combine_signatures_by_type_arity bucketed them as separate overloads and left picking between them to Chain::Call at every call site. Merge them into one signature before that bucketing instead, and make Callable#combine_blocks prefer the block that actually declares parameters rather than choosing arbitrarily. The merged pin is now correct on its own - hover and completion benefit too, not just inference at a call that happens to trigger dispatch_order.
The merge only fired when one side's block declared zero yielded parameters. A block documenting one parameter and a sibling documenting two hit the identical type_arity mismatch and still failed to combine. Compare declared parameter counts instead of emptiness, so any side with fewer yielded parameters loses to one with more, not just the zero case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.