Skip to content

Fix the declared return type of RustCodeOwners.for_file - #176

Merged
dduugg merged 1 commit into
mainfrom
fix-for-file-declared-return-type
Aug 11, 2026
Merged

Fix the declared return type of RustCodeOwners.for_file#176
dduugg merged 1 commit into
mainfrom
fix-for-file-declared-return-type

Conversation

@dduugg

@dduugg dduugg commented Aug 11, 2026

Copy link
Copy Markdown
Contributor
  • I bumped the gem version (or don't need to) 💎

The bug

RustCodeOwners.for_file returns a serialized copy of the Rust Team struct:

pub struct Team {
    pub team_name: String,
    pub team_config_yml: String,
    pub reasons: Vec<String>,
}

Two places declare that as T::Hash[Symbol, String], which never matched, because reasons is an Array:

  • CodeOwnership::Private::TeamFinder.for_fileT.let(RustCodeOwners.for_file(file_path), T.nilable(T::Hash[Symbol, String]))
  • CodeOwnership.for_file_verbose — declared returns(T.nilable(T::Hash[Symbol, String]))

It went unnoticed because sorbet-runtime only checks the outer class of a collection type: T::Hash[Symbol, String].valid? is obj.is_a?(Hash), so any Hash passes and the value types are never looked at.

How it surfaced

Enabling recursive runtime typechecking in a host application — pointing each collection type's valid? at its existing recursively_valid?, as Homebrew/brew#20644 does — makes every for_file call raise:

T.let: Expected type T.nilable(T::Hash[Symbol, String]), got type Hash with value
{team_name: "...", team_config_yml: "...", reasons: [...]}

In our monolith that's 238 raises during boot alone, via CodeOwnership.for_class in an observability initializer.

The fix

Declare the actual shape, shared by both call sites:

FileOwnershipDetails = T.type_alias do
  { team_name: String, team_config_yml: String, reasons: T::Array[String] }
end

team_name is a non-optional Rust String, so it can never serialize to nil. That makes the result.nil? || result[:team_name].nil? guard and its paired T.must dead code, and the shape now enforces what the guard was defending against — both are dropped.

Specs

The stubs returned { team_name: 'Bar' }, which the shape correctly rejects; they now return the whole struct. caches nil when team_name is nil covered a state the extension cannot produce and the type no longer admits, so it's removed — caches nil when rust returns nil still covers the nil path.

bundle exec rspec → 91 examples, 0 failures, 1 pending. bundle exec srb tc → clean.

The Rust extension serializes a `Team` struct with three fields — `team_name`
and `team_config_yml` (both `String`) plus `reasons` (`Vec<String>`) — so the
`T::Hash[Symbol, String]` declared for it in `TeamFinder.for_file` and
`CodeOwnership.for_file_verbose` never matched: `reasons` is an Array.

Sorbet's runtime only checks the outer class of a collection type, so
`T::Hash[Symbol, String]` was satisfied by any Hash and the mismatch went
unnoticed. It surfaces as soon as a host application makes `valid?` descend
into elements (`recursively_valid?`), where every `for_file` call raises.

Replace both with a shape matching the struct. `team_name` is a non-optional
Rust `String`, so it can never serialize to nil: the `result[:team_name].nil?`
branch and its paired `T.must` are now dead, and the stubs in the specs return
the whole struct rather than a `team_name`-only hash.
@dduugg
dduugg requested a review from a team as a code owner August 11, 2026 20:15
@github-project-automation github-project-automation Bot moved this to Triage in Modularity Aug 11, 2026
@dduugg
dduugg merged commit b1726f5 into main Aug 11, 2026
14 checks passed
@dduugg
dduugg deleted the fix-for-file-declared-return-type branch August 11, 2026 20:45
@github-project-automation github-project-automation Bot moved this from Triage to Done in Modularity Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants