Shopify / Shopify/spoom

srb sigs translate: type parameter shadows a type name used in the same signature

Open
#1,012 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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 misleading 7017 ("does not have a sig") instead of the real result. The same two flags are needed in sorbet/config to 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 playground3550 + 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::Error behaviour exactly. The collision check just needs to raise; the existing rescue reports it.
  • Per-signature — leave that one sig block, 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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.