Fix a pre-release-exclusion bug

nex3 · nex3 · commit df57a2505274 · 2018-04-03T14:31:11.000-07:00
The special logic for excluding 1.0.0-dev from <1.0.0 was only implemented for VersionRange.allows(), not for any other methods. This brings the other methods in line with allows(). Closes flutter#20
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,9 @@
+# 1.3.4
+
+* Fix a bug where `VersionRange.allowsAll()`, `VersionRange.allowsAny()`, and
+  `VersionRange.difference()` would return incorrect results for pre-release
+  versions with the same base version number as release versions.
+
 # 1.3.3
 
 * Fix a bug where `VersionRange.difference()` with a union constraint that
diff --git a/lib/src/utils.dart b/lib/src/utils.dart
@@ -2,6 +2,9 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:collection/collection.dart';
+
+import 'version.dart';
 import 'version_range.dart';
 
 /// Returns whether [range1] is immediately next to, but not overlapping,
@@ -29,6 +32,10 @@ bool allowsHigher(VersionRange range1, VersionRange range2) {
   if (range1.max == null) return range2.max != null;
   if (range2.max == null) return false;
 
+  // `<1.0.0-dev.1` allows `1.0.0-dev.0` which is higher than any versions
+  // allowed by `<1.0.0`.
+  if (disallowedByPreRelease(range2, range1.max)) return true;
+
   var comparison = range1.max.compareTo(range2.max);
   if (comparison == 1) return true;
   if (comparison == -1) return false;
@@ -39,6 +46,7 @@ bool allowsHigher(VersionRange range1, VersionRange range2) {
 /// [range2].
 bool strictlyLower(VersionRange range1, VersionRange range2) {
   if (range1.max == null || range2.min == null) return false;
+  if (disallowedByPreRelease(range1, range2.min)) return true;
 
   var comparison = range1.max.compareTo(range2.min);
   if (comparison == -1) return true;
@@ -50,3 +58,41 @@ bool strictlyLower(VersionRange range1, VersionRange range2) {
 /// [range2].
 bool strictlyHigher(VersionRange range1, VersionRange range2) =>
     strictlyLower(range2, range1);
+
+// Returns whether [other] is disallowed by [range] because we disallow
+// pre-release versions that have the same major, minor, and patch version as
+// the max of a range, but only if neither the max nor the min is a pre-release
+// of that version.
+//
+// This ensures that `^1.2.3` doesn't include `2.0.0-pre`, while also allowing
+// both `>=2.0.0-pre.2 <2.0.0` and `>=1.2.3 <2.0.0-pre.7` to match
+// `2.0.0-pre.5`.
+//
+// It's worth noting that this is different than [NPM's semantics][]. NPM
+// disallows **all** pre-release versions unless their major, minor, and
+// patch numbers match those of a prerelease min or max. This ensures that
+// no prerelease versions will ever be selected if the user doesn't
+// explicitly allow them.
+//
+// [NPM's semantics]: https://www.npmjs.org/doc/misc/semver.html#prerelease-tags
+//
+// Instead, we ensure that release versions will always be preferred over
+// prerelease versions by ordering the release versions first in
+// [Version.prioritize]. This means that constraints like `any` or
+// `>1.2.3` can still match prerelease versions if they're the only things
+// available.
+bool disallowedByPreRelease(VersionRange range, Version other) {
+  var maxIsReleaseOfOther = !range.includeMax &&
+      !range.max.isPreRelease &&
+      other.isPreRelease &&
+      _equalsWithoutPreRelease(other, range.max);
+  var minIsPreReleaseOfOther = range.min != null &&
+      range.min.isPreRelease &&
+      _equalsWithoutPreRelease(other, range.min);
+  return maxIsReleaseOfOther && !minIsPreReleaseOfOther;
+}
+
+bool _equalsWithoutPreRelease(Version version1, Version version2) =>
+    version1.major == version2.major &&
+    version1.minor == version2.minor &&
+    version1.patch == version2.patch;
diff --git a/lib/src/version_range.dart b/lib/src/version_range.dart
@@ -90,44 +90,12 @@ class VersionRange implements Comparable<VersionRange>, VersionConstraint {
     if (max != null) {
       if (other > max) return false;
       if (!includeMax && other == max) return false;
-
-      // Disallow pre-release versions that have the same major, minor, and
-      // patch version as the max, but only if neither the max nor the min is a
-      // pre-release of that version. This ensures that "^1.2.3" doesn't include
-      // "2.0.0-pre", while also allowing both ">=2.0.0-pre.2 <2.0.0" and
-      // ">=1.2.3 <2.0.0-pre.7" to match "2.0.0-pre.5".
-      //
-      // It's worth noting that this is different than [NPM's semantics][]. NPM
-      // disallows **all** pre-release versions unless their major, minor, and
-      // patch numbers match those of a prerelease min or max. This ensures that
-      // no prerelease versions will ever be selected if the user doesn't
-      // explicitly allow them.
-      //
-      // [NPM's semantics]: https://www.npmjs.org/doc/misc/semver.html#prerelease-tags
-      //
-      // Instead, we ensure that release versions will always be preferred over
-      // prerelease versions by ordering the release versions first in
-      // [Version.prioritize]. This means that constraints like "any" or
-      // ">1.2.3" can still match prerelease versions if they're the only things
-      // available.
-      var maxIsReleaseOfOther = !includeMax &&
-          !max.isPreRelease &&
-          other.isPreRelease &&
-          _equalsWithoutPreRelease(other, max);
-      var minIsPreReleaseOfOther = min != null &&
-          min.isPreRelease &&
-          _equalsWithoutPreRelease(other, min);
-      if (maxIsReleaseOfOther && !minIsPreReleaseOfOther) return false;
+      if (disallowedByPreRelease(this, other)) return false;
     }
 
     return true;
   }
 
-  bool _equalsWithoutPreRelease(Version version1, Version version2) =>
-      version1.major == version2.major &&
-      version1.minor == version2.minor &&
-      version1.patch == version2.patch;
-
   bool allowsAll(VersionConstraint other) {
     if (other.isEmpty) return true;
     if (other is Version) return allows(other);
diff --git a/test/version_range_test.dart b/test/version_range_test.dart
@@ -228,6 +228,50 @@ main() {
           range.allowsAll(new VersionRange(min: v123, max: v234).union(v140)),
           isFalse);
     });
+
+    group('pre-release versions', () {
+      test('of inclusive min are excluded', () {
+        var range = new VersionRange(min: v123, includeMin: true);
+
+        expect(
+            range.allowsAll(new VersionConstraint.parse('>1.2.4-dev')), isTrue);
+        expect(range.allowsAll(new VersionConstraint.parse('>1.2.3-dev')),
+            isFalse);
+      });
+
+      test('of non-pre-release max are excluded', () {
+        var range = new VersionRange(max: v234);
+
+        expect(range.allowsAll(new VersionConstraint.parse('<2.3.3')), isTrue);
+        expect(range.allowsAll(new VersionConstraint.parse('<2.3.4-dev')),
+            isFalse);
+      });
+
+      test(
+          'of non-pre-release max are included if min is a pre-release of the '
+          'same version', () {
+        var range =
+            new VersionRange(min: new Version.parse('2.3.4-dev.0'), max: v234);
+
+        expect(
+            range.allowsAll(
+                new VersionConstraint.parse('>2.3.4-dev.0 <2.3.4-dev.1')),
+            isTrue);
+      });
+
+      test('of pre-release max are included', () {
+        var range = new VersionRange(max: new Version.parse('2.3.4-dev.2'));
+
+        expect(range.allowsAll(new VersionConstraint.parse('<2.3.4-dev.1')),
+            isTrue);
+        expect(range.allowsAll(new VersionConstraint.parse('<2.3.4-dev.2')),
+            isTrue);
+        expect(range.allowsAll(new VersionConstraint.parse('<=2.3.4-dev.2')),
+            isFalse);
+        expect(range.allowsAll(new VersionConstraint.parse('<2.3.4-dev.3')),
+            isFalse);
+      });
+    });
   });
 
   group('allowsAny()', () {
@@ -325,6 +369,52 @@ main() {
           range.allowsAny(new VersionRange(min: v234, max: v300).union(v010)),
           isFalse);
     });
+
+    group('pre-release versions', () {
+      test('of inclusive min are excluded', () {
+        var range = new VersionRange(min: v123, includeMin: true);
+
+        expect(
+            range.allowsAny(new VersionConstraint.parse('<1.2.4-dev')), isTrue);
+        expect(range.allowsAny(new VersionConstraint.parse('<1.2.3-dev')),
+            isFalse);
+      });
+
+      test('of non-pre-release max are excluded', () {
+        var range = new VersionRange(max: v234);
+
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.3')), isTrue);
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4-dev')),
+            isFalse);
+      });
+
+      test(
+          'of non-pre-release max are included if min is a pre-release of the '
+          'same version', () {
+        var range =
+            new VersionRange(min: new Version.parse('2.3.4-dev.0'), max: v234);
+
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4-dev.1')),
+            isTrue);
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4')), isFalse);
+
+        expect(range.allowsAny(new VersionConstraint.parse('<2.3.4-dev.1')),
+            isTrue);
+        expect(range.allowsAny(new VersionConstraint.parse('<2.3.4-dev')),
+            isFalse);
+      });
+
+      test('of pre-release max are included', () {
+        var range = new VersionConstraint.parse('<2.3.4-dev.2');
+
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4-dev.1')),
+            isTrue);
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4-dev.2')),
+            isFalse);
+        expect(range.allowsAny(new VersionConstraint.parse('>2.3.4-dev.3')),
+            isFalse);
+      });
+    });
   });
 
   group('intersect()', () {
@@ -614,6 +704,13 @@ main() {
                   [v003, new VersionRange(min: v010)])),
           equals(VersionConstraint.empty));
     });
+
+    test("with a range with a pre-release min, returns the original", () {
+      expect(
+          new VersionRange(max: v200)
+              .difference(new VersionConstraint.parse(">=2.0.0-dev")),
+          equals(new VersionRange(max: v200)));
+    });
   });
 
   test('isEmpty', () {
