migrator: typed: strict #21269

hyuraku · 2025-12-17T12:54:39Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

Copilot

Pull request overview

This PR upgrades the migrator.rb file from typed: true to typed: strict mode in Sorbet, adding comprehensive type signatures throughout the codebase as part of the ongoing effort to improve type safety (#17297).

Key changes:

Added type signatures to all class methods, instance methods, and error class constructors
Added type annotations for all attr_reader declarations
Used T.let with explicit types for all instance variable initializations to satisfy strict mode requirements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T12:57:16Z

Library/Homebrew/migrator.rb


  # Error for when a migration is necessary.
  class MigrationNeededError < RuntimeError
+    sig { params(oldname: BasicObject, newname: T.untyped).void }


The parameter oldname is typed as BasicObject which is overly restrictive. In the method body, string interpolation is used with oldname (line 20-21), which requires oldname to respond to to_s. However, BasicObject doesn't have to_s defined. This should be typed as String or T.untyped instead.

Copilot · 2025-12-17T12:57:16Z

Library/Homebrew/migrator.rb


  # Error for when the old name's path does not exist.
  class MigratorNoOldpathError < RuntimeError
+    sig { params(oldname: BasicObject).void }


The parameter oldname is typed as BasicObject which is overly restrictive. In the method body, string interpolation is used with oldname (line 30), which requires oldname to respond to to_s. However, BasicObject doesn't have to_s defined. This should be typed as String or T.untyped instead.

Copilot · 2025-12-17T12:57:17Z

Library/Homebrew/migrator.rb

+    @old_tap = T.let(first_tab&.tap, T.nilable(Tap))

-    raise MigratorDifferentTapsError.new(formula, oldname, old_tap) if !force && !from_same_tap_user?
+    raise MigratorDifferentTapsError.new(formula, oldname, T.must(old_tap)) if !force && !from_same_tap_user?


This T.must call is potentially unsafe. If old_tap is nil at this point (which is possible based on the type signature), this will raise an exception. The conditional check !force && !from_same_tap_user? doesn't guarantee that old_tap is non-nil. Consider checking old_tap explicitly before using T.must, or handling the nil case differently.

Yes, this T.must doesn't seem correct.

@hyuraku please update this one.

Copilot · 2025-12-17T12:57:17Z

Library/Homebrew/migrator.rb

    end
  end

+  sig { returns(T.nilable(T.any(T::Array[T.any(File, String, Pathname)], Integer))) }


This is an overly complex and broad return type signature. The method returns the result of FileUtils.mv which typically returns an array of moved files or an integer (0). This signature attempts to express all possible return types but is unnecessarily complicated. Consider using T.untyped or a simpler type like T.nilable(T::Array[String]) if the return value isn't actually used.

Copilot · 2025-12-17T12:57:17Z

Library/Homebrew/migrator.rb

+    @newname_lock = T.let(FormulaLock.new(newname), T.nilable(FormulaLock))
+    @oldname_lock = T.let(FormulaLock.new(oldname), T.nilable(FormulaLock))
+    T.must(@newname_lock).lock
+    T.must(@oldname_lock).lock


The lock instance variables are being initialized and immediately used with T.must which is unnecessary and overly complicated. Since you're initializing them in the same method, you can assign them directly without wrapping in T.let(..., T.nilable(...)) and then calling T.must. Instead, type them as non-nilable and remove the T.must calls.

- for MigratorNoOldpathError to String

MikeMcQuaid

Thanks @hyuraku looking good so far! There's still a bunch of T.must (including those I didn't comment on) where I don't think there's a sufficient guarantee that it is non-nil. You can see a few examples I've provided to provide that guarantee.

MikeMcQuaid · 2025-12-17T19:23:24Z

Library/Homebrew/formula.rb

  sig { params(fetch_head: T::Boolean).returns(T::Array[Keg]) }
  def outdated_kegs(fetch_head: false)
-    raise Migrator::MigrationNeededError.new(oldnames_to_migrate.first, name) if migration_needed?
+    raise Migrator::MigrationNeededError.new(T.must(oldnames_to_migrate.first), name) if migration_needed?


Suggested change

raise Migrator::MigrationNeededError.new(T.must(oldnames_to_migrate.first), name) if migration_needed?

raise Migrator::MigrationNeededError.new(oldnames_to_migrate.fetch(0)), name) if migration_needed?

MikeMcQuaid · 2025-12-17T19:24:15Z

Library/Homebrew/migrator.rb

+    @old_tap = T.let(first_tab&.tap, T.nilable(Tap))

-    raise MigratorDifferentTapsError.new(formula, oldname, old_tap) if !force && !from_same_tap_user?
+    raise MigratorDifferentTapsError.new(formula, oldname, T.must(old_tap)) if !force && !from_same_tap_user?


Yes, this T.must doesn't seem correct.

MikeMcQuaid · 2025-12-17T19:24:50Z

Library/Homebrew/migrator.rb

+    formula_tap_user = formula.tap&.user
    old_tap_user = nil

    new_tap = if old_tap


Suggested change

new_tap = if old_tap

new_tap = if (old_tap = self.old_tap)

and then can remove T.musts below

MikeMcQuaid · 2025-12-17T19:30:20Z

Library/Homebrew/migrator.rb

+  sig { void }
  def repin
    return unless pinned?
+    return unless old_pin_link_record


Suggested change

return unless old_pin_link_record

old_pin_link_record = self.old_pin_link_record

return unless old_pin_link_record

and then can remove T.must below

MikeMcQuaid

@hyuraku A few more T.must to cleanup.

MikeMcQuaid · 2025-12-22T09:44:31Z

Library/Homebrew/migrator.rb

+    @old_tap = T.let(first_tab&.tap, T.nilable(Tap))

-    raise MigratorDifferentTapsError.new(formula, oldname, old_tap) if !force && !from_same_tap_user?
+    raise MigratorDifferentTapsError.new(formula, oldname, T.must(old_tap)) if !force && !from_same_tap_user?


@hyuraku please update this one.

…logic

migrator: typed: strict

cc0fa75

Copilot AI review requested due to automatic review settings December 17, 2025 12:54

Copilot started reviewing on behalf of hyuraku December 17, 2025 12:55 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

hyuraku added 2 commits December 17, 2025 21:58

fix: update type

dd2ecec

- for MigratorNoOldpathError to String

fix: update type annotations in Migrator class

53fc3b4

MikeMcQuaid reviewed Dec 17, 2025

View reviewed changes

fix: improve error handling and type safety in Migrator class

dee37b0

MikeMcQuaid reviewed Dec 22, 2025

View reviewed changes

hyuraku added 2 commits December 23, 2025 00:44

fix: enhance type safety in MigratorDifferentTapsError and migration …

5ab09cb

…logic

fix: correct string interpolation in migration error message

f0f2fb4

	raise Migrator::MigrationNeededError.new(T.must(oldnames_to_migrate.first), name) if migration_needed?
	raise Migrator::MigrationNeededError.new(oldnames_to_migrate.fetch(0)), name) if migration_needed?

	return unless old_pin_link_record
	old_pin_link_record = self.old_pin_link_record
	return unless old_pin_link_record

Uh oh!

migrator: typed: strict #21269

Are you sure you want to change the base?

migrator: typed: strict #21269

Conversation

hyuraku commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants