Keep the generic return type when a method takes a block - #1274
Conversation
Pin::Callable#arity_matches? rejected any call missing a block whenever the method signature had block info attached, even when that info came from a bare &block formal parameter with no @yield tags. Ruby never requires callers to pass a block for such a parameter, so this caused the sole matching signature to be discarded, skipping generic resolution and leaving the return type as unresolved generic<T>. Add Pin::Callable#block_required?, true only for RBS-sourced signatures with a non-optional block ({ ... } vs ?{ ... }), and gate the arity check on it instead of bare block presence. YARD-derived signatures have no way to express a required block, so they default to false. Fixes castwide#1265 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TNnFsUN4Uryo6Xqh7xv2sr
Adds Pin::Callable#block_required? (default false), true only for
RBS-sourced signatures with a non-optional block (`{ ... }`, not
`?{ ... }`). Pin::Callable#arity_matches? previously rejected a call
missing a block whenever the signature declared *any* block, even an
optional one - so a bare `&block`/@yield-tag signature with a @Generic
return type would get skipped by overload resolution for a callsite
with no block, falling through to a less specific overload and losing
the generic. Now it only rejects when block_required? is true.
Conflict in lib/solargraph/rbs_map/conversions.rb: HEAD didn't know
about the new block_required: keyword yet; took the incoming side,
which wires overload.method_type.block&.required into the new
Pin::Signature parameter (mirroring the equivalent, non-conflicting
change already applied to rbs_translator.rb#to_signature).
Verified: spec/source/chain/call_spec.rb, spec/rbs_map/conversions_spec.rb,
spec/rbs_translator_spec.rb (59 examples, 0 failures, 3 pending), and a
broader safety net - spec/type_checker, spec/source,
spec/source_map/clip_spec.rb, spec/pin (792 examples, 0 failures, 25
pending) - all passing locally.
|
🤖 Posted by Claude, not the account owner — acting on their behalf via their GitHub credentials. Re-tested this fix against Minimal reproduction# typed: false
# frozen_string_literal: true
class SingleFetch
# @param data [Hash{String => String}]
# @return [String]
def value(data)
data.fetch('key')
end
endNo block param, no BisectionSame file, same
Observed patternAcross the 50 new failures in Guess, not confirmed by reading the diff: the fix for #1265 changed the return-type substitution/binding step so it stops discarding a generic binding when a block param is present. Happy to share the full |
|
🤖 Posted by Claude, not the account owner — acting on their behalf via their GitHub credentials. This reproduces, but it's a stale on-disk pin cache, not a regression in this PR's diff.
Confirmed directly: populated the cache on the commit before this PR, switched to the commit with this PR merged under the same Opened a follow-up so this class of bug can't recur silently on future PRs that don't bump VERSION: folds a digest of solargraph's own |
castwide/solargraph#1274 (fixes #1265) is still open upstream, but its commit is already an ancestor of our pinned fork revision, and this call shape still fails -- confirmed via strip-and-observe, including ruling out the ensure clause as the cause. Follow-on gap building on PR 1274, matching the pr-1231-follow-on precedent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1KB8X6cDo6QtyYv1RwJzd
|
🤖 Posted by Claude, not the account owner — acting on their behalf via their GitHub credentials. Found a related gap while auditing Reproductionclass Repro
# @generic T
# @yieldreturn [generic<T>]
# @return [generic<T>]
def call
yield
end
endReproduced on |
UniqueType#resolve_generics was resolving generic<T> against the receiver's binder type whenever definitions.generics was non-empty, without checking whether those generics belonged to the enclosing namespace or to the method/signature itself. For a method with a method-scoped @Generic T combined with @yieldreturn [generic<T>], Chain::Call#yield_pins produces a proxied Signature pin whose closure is the outer Signature (generics: ["T"]), not a Pin::Namespace. Since the class itself carries no type params to bind against, resolution fell into the UNDEFINED branch, making TypeChecker report "return type could not be inferred" even though @Generic T is legitimately unresolved at this point (it's bound per call site, not per receiver. Guard resolve_generics to skip when definitions is a Pin::Callable (Method/Signature), leaving such types unresolved instead of eagerly erasing them -- matching the already-passing behavior for a plain generic-typed parameter referenced from a method body. Reported in castwide#1274 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KuQJXiNek2mabhwLUtBgYY EOF )
…d with no explicit block param
|
Ready for review |
|
Claude: Written by Claude and posted using @apiology's GitHub credentials. Confirming this fixes the reported symptom: with this branch in our baseline, both arms of #1265's repro ( The helper underneath them still doesn't resolve, though — class Foo
# @return [Integer]
def foo_method
1
end
end
class Repro
# @generic T
# @param clazz [Class<generic<T>>]
# @return [generic<T>]
def create_object(clazz)
clazz.new
end
# @param clazz [Class<Foo>]
# @return [Foo]
def create_concrete(clazz)
clazz.new
end
# @return [Integer]
def use_it
create_object(Foo).foo_method
end
end
That accounts for 7 suppressions in our codebase, separate from the 14 tied to #1265 itself. |
ComplexType::TypeMethods#namespace/#namespace_type/#scope treated any Class<X>/Module<X> subtype as a concrete namespace, including when X is itself an unresolved generic<T> placeholder. That produced the bogus namespace tag "generic<T>", which ApiMap#get_method_stack could not find methods for, so Chain::Call#resolve bailed out before ever reaching the existing Class#new/reduce_class_type special-casing. Treat Class<generic<T>>/Module<generic<T>> like a bare Class/Module (no subtype) instead, so lookups go through Class's own instance-scope #new. ApiMap#get_methods synthesis of Class#new from #initialize also needed a matching guard: for a bare Class namespace there is no concrete #initialize to find, and looking one up recursed back into the same Class#new pin forever. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018JKw9HkJuhpHWoMmqKdDMm
…eclares a block param # Conflicts: # lib/solargraph/complex_type/type_methods.rb
|
Claude: Written by Claude and posted using @apiology's GitHub credentials. clazz.new(:foo, **args) # Too many arguments to Class#initialize |
…c<T>" This reverts commit 05aad7a.
…ed Class#initialize arity check) # Conflicts: # lib/solargraph/complex_type/type_methods.rb
Chain::Call#inferred_pins tried block-taking overload signatures before no-block ones unconditionally, on the assumption that Callable#arity_matches? would reject a block-taking overload when the call site passed no block. That assumption held until commit 922d073 (castwide#1274) made block_required? default false for YARD-derived signatures, since YARD has no syntax to mark a block mandatory. A YARD @!override with two @overload tags differing only by block presence now resolves a no-block call site to the block overload return type (often an unresolved generic) regardless of declaration order. Fix: only try block-taking overloads first when the call site actually passed a block; try no-block overloads first otherwise. Regression spec added to strong_spec.rb. Extracted the overload-ordering logic into a new dispatch_order helper so inferred_pins stays under the Metrics/AbcSize, Metrics/PerceivedComplexity, and Metrics/BlockLength thresholds the added branching pushed it over.
Vince's pending review on PR 1274 asked to shorten two docstrings past the 2-line budget, and to add a spec showing what type we derive when a caller captures a method-scoped generic's block return value, not just that no error is raised. - unique_type.rb: trim the resolve_generics comment to 2 lines. - callable.rb: trim the @PARAM block_required doc to 2 lines. - strong_spec.rb: add a spec asserting `x = Repro.new.call { 123 }` infers Integer, via a deliberately mismatched @type declaration (same idiom as the existing "complains on bad @type assignment" example in this file).
Reorder so the comment states the action before the rationale, per review. Also replaces the ignore marker's reason. The previous wording blamed uniq, but the block param is untyped because Array#flatten is declared in core RBS as returning a bare Array: arr.flatten infers to ::Array where arr.compact infers to ::Array<::String>. With no element type on the receiver there is nothing for uniq's block param to bind to. No open PR covers this. The nearest two address different shapes: castwide#1326 is a method-level generic bound by an argument alongside a block, and castwide#1274 is a generic return type lost when the declaring method takes a block. Left as prose rather than citing a URL that does not match.
Problem: Adding an unused
&blockparameter to a generic method stops its generic from binding, so ordinary arguments are rejected and the call infers asgeneric<T>rather than the type passed in.Deleting
&unusedand changing nothing else makes it resolve, so any generic helper that takes a block is unusable with no indication why.Solution: Signatures now record whether a block is mandatory -- only RBS can express that, as
{ ... }versus?{ ... }-- and a blockless call is rejected only when it is, so a bare&blockno longer discards the signature whileArray#each-style Enumerator overloads still disambiguate.Method-scoped generics are then left unresolved against the receiver, because they bind per call site rather than to the namespace's type parameters.
Fixes #1265
Generated with Claude Code
https://claude.ai/code/session_01TNnFsUN4Uryo6Xqh7xv2sr