srb sigs translate: type parameter shadows a type name used in the same signature
Nobody has claimed this yet.
- Dominant language
- Ruby
- Stars
- 272
- Forks
- 29
- Avg merge
- 11h 35m
- Merged PRs (30d)
- 4
Description
Summary
spoom srb sigs translate converts Sorbet sig blocks into inline RBS comments (#:). When a sig declares a method type parameter whose name matches a type name referenced in the same signature, the emitted RBS is wrong: the type variable shadows whatever else that name referred to.
In Sorbet's sig DSL, T.type_parameter(:Foo) lives in a namespace of its own, separate from constants and from class-level type members. In RBS it does not — a type variable identifier is resolved ahead of anything else that could occupy the same type position. So every occurrence of that name in the translated signature binds to the type variable.
Three collision classes are affected. Only the first fails loudly. The other two silently delete a type error.
Every reproduction below has a playground link carrying
?arg=--enable-experimental-rbs-comments&arg=--parser=prism. Both flags are required — without them Sorbet ignores every#:comment and reports a misleading7017("does not have asig") instead of the real result. The same two flags are needed insorbet/configto reproduce locally.
Class 1 — collides with a user constant
# typed: strict
class Store
extend T::Sig
module Credential
module Multiple; end
end
sig do
type_parameters(:Credential)
.params(type: T.all(T::Module[T.type_parameter(:Credential)], T::Module[Credential::Multiple]))
.returns(T::Array[T.type_parameter(:Credential)])
end
def find(type)
[]
end
end
▶ Run in Sorbet playground — clean before translation
spoom srb sigs translate --from rbi --to rbs emits:
#: [Credential] (
#| (Module[Credential] & Module[Credential::Multiple]) type
#| ) -> Array[Credential]
▶ Run in Sorbet playground — 3550 + 7017
srb tc, with the caret on the :::
Failed to parse RBS signature (comma delimited type list is expected) https://srb.help/3550
The method `find` does not have a `sig` https://srb.help/7017
It is the name, not the parenthesised intersection. Changing one variable at a time:
- Remove the parentheses, keep the name → still fails, same
3550. ▶ playground - Keep the parentheses, rename the type parameter to
Cred→ passes, zero errors. ▶ playground
This class fails loudly only because the shadowed constant is dereferenced. When it is merely mentioned, the same collision goes silent — see Class 1b.
Class 1b — same, but the constant is only mentioned
sig do
type_parameters(:Credential)
.params(x: T.type_parameter(:Credential), y: Credential)
.returns(T.type_parameter(:Credential))
end
def find(x, y) = x
Translates to #: [Credential] (Credential x, Credential y) -> Credential, which parses fine.
BEFORE: Store.new.find(1, 2) → Expected `Store::Credential` but found `Integer(2)` ✓ caught
AFTER: Store.new.find(1, 2) → No errors! Great job. ✗ silent
T.reveal_type(y) → T.type_parameter(:Credential) (expected Store::Credential)
▶ playground: before · playground: after
Class 2 — collides with a class-level type_member
The shadowed entity here is a type member, not a plain constant:
# typed: strict
class Box
extend T::Sig
extend T::Generic
Elem = type_member
sig do
type_parameters(:Elem)
.params(x: T.type_parameter(:Elem), y: Elem)
.returns(T.type_parameter(:Elem))
end
def pick(x, y)
x
end
end
Translates to #: [Elem] (Elem x, Elem y) -> Elem. The method type parameter shadows the class type parameter:
BEFORE: Box[String].new.pick(1, 2) → Expected `String` but found `Integer(2)` ✓ caught
AFTER: Box[String].new.pick(1, 2) → No errors! Great job. ✗ silent
T.reveal_type(y) → T.type_parameter(:Elem) (of Box#pick)
▶ playground: before · playground: after
Class 3 — collides with a builtin
sig do
type_parameters(:String)
.params(a: T.type_parameter(:String), b: String)
.returns(T.type_parameter(:String))
end
def go(a, b) = a
Translates to #: [String] (String a, String b) -> String. The type variable swallows the builtin:
BEFORE: Q.new.go(1, 2) → Expected `String` but found `Integer(2)` ✓ caught
AFTER: Q.new.go(1, 2) → No errors! Great job. ✗ silent
T.reveal_type(b) → T.type_parameter(:String) (of Q#go)
▶ playground: before · playground: after
Expected
Detect the collision and refuse to translate that signature, reporting it — rather than emitting a signature that means something other than the input.
The check should fire when the type parameter name matches any type name referenced in the same signature, whether that resolves to a user constant, a builtin, or an in-scope type_member / type_template.
The "referenced in the same signature" qualifier is load-bearing in all three classes. A type parameter named Credential in a signature that never mentions Credential shadows nothing and is harmless, so the check should not fire on the name alone. That keeps this a real error rather than a nuisance.
Please don't auto-rename the type parameter. Renaming is safe in principle — a method type parameter name is local to its signature, and callers are observationally unaffected — but it is the wrong default here:
- It silently rewrites an identifier the author chose, which is visible in Sorbet's own diagnostics (
T.type_parameter(:Credential) (of Store#find)) and to every future reader of the file. - A renamer has to rewrite every type-variable occurrence and no constant occurrence in the same signature. Getting that boundary wrong emits a signature that parses cleanly and means something else — exactly the silent-mistyping failure this issue is about. That is a poor trade for a construct that occurred once in a 19,197-file translation.
Spoom already has precedent for both halves of this: next if sig.is_abstract && !@translate_abstract_methods skips what it can't translate faithfully, and overloads_strategy accepts :raise as one of [:translate_all, :translate_last, :raise].
Two reasonable granularities:
- Per-file — skip the file, matching the existing
rescue RBI::Errorbehaviour exactly. The collision check just needs to raise; the existing rescue reports it. - Per-signature — leave that one
sigblock, translate the rest of the file, warn. Nicer output, slightly more work.
Either way it should be non-fatal. A hard abort would mean one bad signature kills a 110k-file migration, which is worse than the bug.
Proposed error message
Following the style of raise Error, "Method \#{name}` at #{location} has multiple overloaded signatures"`:
Can't translate the signature for `Store#find` at store.rb:9.
The type parameter `Credential` collides with the constant `Credential::Multiple`
referenced in the same signature. Sorbet's `sig` DSL keeps these in separate
namespaces; RBS does not, so the type variable would shadow the constant and
silently change what the signature means.
Left as a `sig` block. Rename the type parameter to something that doesn't
collide, then re-run.
The message should name what was shadowed, since it varies across the three classes — a user shown only the type parameter name would be baffled by the builtin case.
One wording note: the existing CLI rescue hardcodes say_warning("Can't parse #{file}: ..."), which would report a collision as a parse failure. This needs either a distinct rescue arm or a separate error class.
Optional escape hatch
If maintainers want renaming available for anyone running a fully automatic large migration, the overloads_strategy precedent suggests a strategy option with :raise as the default. The objection above is to renaming being the default, not to it existing.
Impact
Found while translating Shopify's Ruby monorepo (19,197 files rewritten). Class 1 accounted for 2 of that run's 14 remaining errors.
The error count understates it. Classes 1b, 2 and 3 produce no diagnostic at all — a signature that previously caught a type error stops catching it, and the diff shows a signature being faithfully translated. Those would not appear in any error count, including the one above, so the true incidence is unknown.
Environment
spoom 1.8.7, sorbet / sorbet-static 0.6.13386, rbs 4.1.1, prism 1.9.0, Ruby 4.0.
Related
#990 — "Generic types called T can lead to TypeError". Same shadowing family, but the opposite direction (RBS→RBI, runtime NameError from an inserted extend T::Generic), and its fix does not transfer here. #990 is fixed by fully qualifying the constant; that only half works for this issue:
| Emitted reference | Result | |
|---|---|---|
Credential::Multiple — collision in first segment |
parse error 3550 |
▶ playground |
::Shop::Credential::Multiple — collision mid-path |
resolves correctly | ▶ playground |
::Store::Credential — collision in final segment |
silently becomes the type variable | ▶ playground |
Qualification clears the loud parse failure but not the silent mistyping.
That last row also looks like a Sorbet-side bug independent of spoom — a root-anchored path resolving to a type variable — and may warrant a separate upstream report.
#1010 — a different translator bug with the same shape of resolution.
Workaround
Rename the type parameter so it doesn't collide, and leave a comment saying why, or the next person to touch the signature will "fix" the name back.
# NOTE: the type parameter is `Cred`, not `Credential`: an RBS type variable shadows the
# constant of the same name, which would make `Credential::Multiple` unparseable.
#: [Cred] ((Module[Cred] & Module[Credential::Multiple]) type) -> Array[Cred]
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at the spoom srb sigs translate entry point, then inspect the existing RBI::Error rescue, sig.is_abstract handling, and overloads_strategy behavior. Done means detecting referenced-name collisions across constants, builtins, and type members or templates, reporting them without aborting the migration, and covering the three collision classes with regression tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- ruby
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 56/100