Simplify type parameters for Gem::Version (dependabot#9232)

The upstream types for `Gem::Version.initialize` and `Gem::Version.new` are quite complex. They accept `T.any(String, Integer, Float, Gem::Version, NilClass)`. For our use cases, these are too broad, and we see some issues with `SystemStackError` due to Sorbet's runtime type checking inserted at each level of our class hierarchy i.e. `Ecosystem::Version` -> `Dependabot::Version` -> `Gem::Version`. Sorbet does have documentation on how to [narrow the types of a child method][1]. The documentation suggests altering the parent class to be generic, and specifying the generic type in child classes. Unfortunately, it's not feasible to alter the root class, `Gem::Version`. Another approach might be to use one of Sorbet's escape hatches `override(allow_incompatible: true)`[^3]. But this disables the type checking entirely, and might hide other issues. The approach I'm taking in this change is to instead write our own shim to override the accepted types. [^1]: https://github.com/sorbet/sorbet/blob/15df026a4ffe45871809885a26142bddac5cbdde/rbi/stdlib/rubygems.rbi#L2635-L2646 [^2]: https://github.com/sorbet/sorbet/blob/15df026a4ffe45871809885a26142bddac5cbdde/rbi/stdlib/rubygems.rbi#L2738-L2749 [^3]: https://sorbet.org/docs/override-checking#use-overrideallow_incompatible-true [1]: https://sorbet.org/docs/override-checking#what-if-i-really-want-the-child-method-to-narrow-the-type Co-authored-by: AbdulFattaah Popoola <abdulapopoola@github.com>
Qureshi2131 · Mar 7, 2024 · 0661469 · 0661469
1 parent 4c75356
commit 0661469
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 173 deletions.
diff --git a/common/lib/dependabot/version.rb b/common/lib/dependabot/version.rb
@@ -10,59 +10,22 @@ class Version < Gem::Version
 
     abstract!
 
-    sig do
-      override
-        .overridable
-        .params(
-          version: T.any(
-            String,
-            Integer,
-            Float,
-            Gem::Version,
-            NilClass
-          )
-        )
-        .void
-    end
+    VersionParameter = T.type_alias { T.nilable(T.any(String, Integer, Gem::Version)) }
+
+    sig { override.overridable.params(version: VersionParameter).void }
     def initialize(version)
       @original_version = T.let(version.to_s, String)
 
-      T.unsafe(super(version))
+      super
     end
 
-    sig do
-      override
-        .overridable
-        .params(
-          version: T.any(
-            String,
-            Integer,
-            Float,
-            Gem::Version,
-            NilClass
-          )
-        )
-        .returns(Dependabot::Version)
-    end
+    sig { override.overridable.params(version: VersionParameter).returns(Dependabot::Version) }
     def self.new(version)
       T.cast(super, Dependabot::Version)
     end
 
     # Opt-in to Rubygems 4 behavior
-    sig do
-      override
-        .overridable
-        .params(
-          version: T.any(
-            String,
-            Integer,
-            Float,
-            Gem::Version,
-            NilClass
-          )
-        )
-        .returns(T::Boolean)
-    end
+    sig { override.overridable.params(version: VersionParameter).returns(T::Boolean) }
     def self.correct?(version)
       return false if version.nil?
 

diff --git a/github_actions/lib/dependabot/github_actions/version.rb b/github_actions/lib/dependabot/github_actions/version.rb
@@ -11,63 +11,25 @@ module GithubActions
     class Version < Dependabot::Version
       extend T::Sig
 
-      sig do
-        override
-          .overridable
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .void
-      end
+      sig { override.params(version: VersionParameter).void }
       def initialize(version)
         version = Version.remove_leading_v(version)
         super
       end
 
-      sig do
-        params(
-          version: T.any(
-            String,
-            Integer,
-            Float,
-            Gem::Version,
-            NilClass
-          )
-        ).returns(
-          T.any(
-            String,
-            Integer,
-            Float,
-            Gem::Version,
-            NilClass
-          )
-        )
+      sig { override.params(version: VersionParameter).returns(Dependabot::GithubActions::Version) }
+      def self.new(version)
+        T.cast(super, Dependabot::GithubActions::Version)
       end
+
+      sig { params(version: VersionParameter).returns(VersionParameter) }
       def self.remove_leading_v(version)
         return version unless version.to_s.match?(/\Av([0-9])/)
 
         version.to_s.delete_prefix("v")
       end
 
-      sig do
-        override
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .returns(T::Boolean)
-      end
+      sig { override.params(version: VersionParameter).returns(T::Boolean) }
       def self.correct?(version)
         version = Version.remove_leading_v(version)
         super

diff --git a/npm_and_yarn/lib/dependabot/npm_and_yarn/version.rb b/npm_and_yarn/lib/dependabot/npm_and_yarn/version.rb
@@ -21,20 +21,7 @@ class Version < Dependabot::Version
       VERSION_PATTERN = T.let(Gem::Version::VERSION_PATTERN + '(\+[0-9a-zA-Z\-.]+)?', String)
       ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})?\s*\z/
 
-      sig do
-        override
-          .overridable
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .returns(T::Boolean)
-      end
+      sig { override.params(version: VersionParameter).returns(T::Boolean) }
       def self.correct?(version)
         version = version.gsub(/^v/, "") if version.is_a?(String)
 
@@ -43,7 +30,7 @@ def self.correct?(version)
         version.to_s.match?(ANCHORED_VERSION_PATTERN)
       end
 
-      sig { params(version: T.nilable(T.any(String, Gem::Version))).returns(T.nilable(T.any(String, Gem::Version))) }
+      sig { params(version: VersionParameter).returns(VersionParameter) }
       def self.semver_for(version)
         # The next two lines are to guard against improperly formatted
         # versions in a lockfile, such as an empty string or additional
@@ -55,19 +42,7 @@ def self.semver_for(version)
         version
       end
 
-      sig do
-        override
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .void
-      end
+      sig { override.params(version: VersionParameter).void }
       def initialize(version)
         @version_string = T.let(version.to_s, String)
         version = version.gsub(/^v/, "") if version.is_a?(String)
@@ -77,19 +52,7 @@ def initialize(version)
         super(T.must(version))
       end
 
-      sig do
-        override
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .returns(Dependabot::NpmAndYarn::Version)
-      end
+      sig { override.params(version: VersionParameter).returns(Dependabot::NpmAndYarn::Version) }
       def self.new(version)
         T.cast(super, Dependabot::NpmAndYarn::Version)
       end

diff --git a/nuget/lib/dependabot/nuget/version.rb b/nuget/lib/dependabot/nuget/version.rb
@@ -17,21 +17,26 @@ class Version < Dependabot::Version
       VERSION_PATTERN = T.let(Gem::Version::VERSION_PATTERN + '(\+[0-9a-zA-Z\-.]+)?', String)
       ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})?\s*\z/
 
-      sig { override.params(version: T.nilable(T.any(String, Integer, Float, Gem::Version))).returns(T::Boolean) }
+      sig { override.params(version: VersionParameter).returns(T::Boolean) }
       def self.correct?(version)
         return false if version.nil?
 
         version.to_s.match?(ANCHORED_VERSION_PATTERN)
       end
 
-      sig { override.params(version: T.nilable(T.any(String, Integer, Float, Gem::Version))).void }
+      sig { override.params(version: VersionParameter).void }
       def initialize(version)
         version = version.to_s.split("+").first || ""
         @version_string = T.let(version, String)
 
         super
       end
 
+      sig { override.params(version: VersionParameter).returns(Dependabot::Nuget::Version) }
+      def self.new(version)
+        T.cast(super, Dependabot::Nuget::Version)
+      end
+
       sig { returns(String) }
       def to_s
         @version_string

diff --git a/pub/lib/dependabot/pub/version.rb b/pub/lib/dependabot/pub/version.rb
@@ -25,27 +25,19 @@ class Version < Dependabot::Version
       sig { returns(String) }
       attr_reader :build_info
 
-      sig do
-        override
-          .overridable
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .void
-      end
+      sig { override.params(version: VersionParameter).void }
       def initialize(version)
         @version_string = T.let(version.to_s, String)
         version, @build_info = version.to_s.split("+") if version.to_s.include?("+")
 
         super(T.must(version))
       end
 
+      sig { override.params(version: VersionParameter).returns(Dependabot::Pub::Version) }
+      def self.new(version)
+        T.cast(super, Dependabot::Pub::Version)
+      end
+
       sig { override.returns(String) }
       def to_s
         @version_string
@@ -56,20 +48,7 @@ def inspect # :nodoc:
         "#<#{self.class} #{@version_string}>"
       end
 
-      sig do
-        override
-          .overridable
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .returns(T::Boolean)
-      end
+      sig { override.params(version: VersionParameter).returns(T::Boolean) }
       def self.correct?(version)
         return false if version.nil?
 

diff --git a/sorbet/rbi/shims/rubygems.rbi b/sorbet/rbi/shims/rubygems.rbi
@@ -0,0 +1,34 @@
+# typed: strong
+# frozen_string_literal: true
+
+module Gem
+  class Version
+    sig do
+      params(
+        version: T.nilable(
+          T.any(
+            String,
+            Integer,
+            Gem::Version
+          )
+        )
+      )
+        .returns(Gem::Version)
+    end
+    def self.new(version); end
+
+    sig do
+      params(
+        version: T.nilable(
+          T.any(
+            String,
+            Integer,
+            Gem::Version
+          )
+        )
+      )
+        .void
+    end
+    def initialize(version); end
+  end
+end
diff --git a/terraform/lib/dependabot/terraform/version.rb b/terraform/lib/dependabot/terraform/version.rb
@@ -14,25 +14,17 @@ module Terraform
     class Version < Dependabot::Version
       extend T::Sig
 
-      sig do
-        override
-          .overridable
-          .params(
-            version: T.any(
-              String,
-              Integer,
-              Float,
-              Gem::Version,
-              NilClass
-            )
-          )
-          .void
-      end
+      sig { override.params(version: VersionParameter).void }
       def initialize(version)
         @version_string = T.let(version.to_s, String)
         super
       end
 
+      sig { override.params(version: VersionParameter).returns(Dependabot::Terraform::Version) }
+      def self.new(version)
+        T.cast(super, Dependabot::Terraform::Version)
+      end
+
       sig { override.returns(String) }
       def to_s
         @version_string