Isolate, reuse PackageFinder best candidate logic (#5971)

Split out how PackageFinder finds the best candidate, and reuse it in the
self version check, to avoid the latter duplicating (and incorrectly
implementing) the same logic.

news/5175.bugfix

@@ -0,0 +1 @@
+Make pip's self version check avoid recommending upgrades to prereleases if the currently-installed version is stable.

src/pip/_internal/index.py

@@ -55,7 +55,8 @@ if MYPY_CHECK_RUNNING:
     BuildTag = Tuple[Any, ...]  # either empty tuple or Tuple[int, str]
     CandidateSortingKey = Tuple[int, _BaseVersion, BuildTag, Optional[int]]
 
-__all__ = ['FormatControl', 'PackageFinder']
+
+__all__ = ['FormatControl', 'FoundCandidates', 'PackageFinder']
 
 
 SECURE_ORIGINS = [
@@ -254,6 +255,67 @@ def _get_html_page(link, session=None):
     return None
 
 
+class FoundCandidates(object):
+    """A collection of candidates, returned by `PackageFinder.find_candidates`.
+
+    Arguments:
+
+    * `candidates`: A sequence of all available candidates found.
+    * `specifier`: Specifier to filter applicable versions.
+    * `prereleases`: Whether prereleases should be accounted. Pass None to
+        infer from the specifier.
+    * `sort_key`: A callable used as the key function when choosing the best
+        candidate.
+    """
+    def __init__(
+        self,
+        candidates,     # type: List[InstallationCandidate]
+        specifier,      # type: specifiers.BaseSpecifier
+        prereleases,    # type: Optional[bool]
+        sort_key,       # type: Callable[[InstallationCandidate], Any]
+    ):
+        # type: (...) -> None
+        self._candidates = candidates
+        self._specifier = specifier
+        self._prereleases = prereleases
+        self._sort_key = sort_key
+
+    def iter_all(self):
+        # type: () -> Iterable[InstallationCandidate]
+        """Iterate through all candidates.
+        """
+        return iter(self._candidates)
+
+    def iter_applicable(self):
+        # type: () -> Iterable[InstallationCandidate]
+        """Iterate through candidates matching the given specifier.
+        """
+        # Filter out anything which doesn't match our specifier.
+        versions = set(self._specifier.filter(
+            # We turn the version object into a str here because otherwise
+            # when we're debundled but setuptools isn't, Python will see
+            # packaging.version.Version and
+            # pkg_resources._vendor.packaging.version.Version as different
+            # types. This way we'll use a str as a common data interchange
+            # format. If we stop using the pkg_resources provided specifier
+            # and start using our own, we can drop the cast to str().
+            [str(c.version) for c in self._candidates],
+            prereleases=self._prereleases,
+        ))
+        # Again, converting to str to deal with debundling.
+        return (c for c in self._candidates if str(c.version) in versions)
+
+    def get_best(self):
+        # type: () -> Optional[InstallationCandidate]
+        """Return the best candidate available, or None if no applicable
+        candidates are found.
+        """
+        candidates = list(self.iter_applicable())
+        if not candidates:
+            return None
+        return max(candidates, key=self._sort_key)
+
+
 class PackageFinder(object):
     """This finds packages.
 
@@ -628,6 +690,25 @@ class PackageFinder(object):
         # This is an intentional priority ordering
         return file_versions + find_links_versions + page_versions
 
+    def find_candidates(
+        self,
+        project_name,   # type: str
+        specifier=specifiers.SpecifierSet(),  # type: specifiers.BaseSpecifier
+    ):
+        """Find matches for the given project and specifier.
+
+        If given, `specifier` should implement `filter` to allow version
+        filtering (e.g. ``packaging.specifiers.SpecifierSet``).
+
+        Returns a `FoundCandidates` instance.
+        """
+        return FoundCandidates(
+            self.find_all_candidates(project_name),
+            specifier=specifier,
+            prereleases=(self.allow_all_prereleases or None),
+            sort_key=self._candidate_sort_key,
+        )
+
     def find_requirement(self, req, upgrade):
         # type: (InstallRequirement, bool) -> Optional[Link]
         """Try to find a Link matching req
@@ -636,52 +717,28 @@ class PackageFinder(object):
         Returns a Link if found,
         Raises DistributionNotFound or BestVersionAlreadyInstalled otherwise
         """
-        all_candidates = self.find_all_candidates(req.name)
-
-        # Filter out anything which doesn't match our specifier
-        compatible_versions = set(
-            req.specifier.filter(
-                # We turn the version object into a str here because otherwise
-                # when we're debundled but setuptools isn't, Python will see
-                # packaging.version.Version and
-                # pkg_resources._vendor.packaging.version.Version as different
-                # types. This way we'll use a str as a common data interchange
-                # format. If we stop using the pkg_resources provided specifier
-                # and start using our own, we can drop the cast to str().
-                [str(c.version) for c in all_candidates],
-                prereleases=(
-                    self.allow_all_prereleases
-                    if self.allow_all_prereleases else None
-                ),
-            )
-        )
-        applicable_candidates = [
-            # Again, converting to str to deal with debundling.
-            c for c in all_candidates if str(c.version) in compatible_versions
-        ]
-
-        if applicable_candidates:
-            best_candidate = max(applicable_candidates,
-                                 key=self._candidate_sort_key)
-        else:
-            best_candidate = None
+        candidates = self.find_candidates(req.name, req.specifier)
+        best_candidate = candidates.get_best()
 
+        installed_version = None    # type: Optional[_BaseVersion]
         if req.satisfied_by is not None:
             installed_version = parse_version(req.satisfied_by.version)
-        else:
-            installed_version = None
+
+        def _format_versions(cand_iter):
+            # This repeated parse_version and str() conversion is needed to
+            # handle different vendoring sources from pip and pkg_resources.
+            # If we stop using the pkg_resources provided specifier.
+            return ", ".join(sorted(
+                {str(c.version) for c in cand_iter},
+                key=parse_version,
+            )) or "none"
 
         if installed_version is None and best_candidate is None:
             logger.critical(
                 'Could not find a version that satisfies the requirement %s '
                 '(from versions: %s)',
                 req,
-                ', '.join(
-                    sorted(
-                        {str(c.version) for c in all_candidates},
-                        key=parse_version,
-                    )
-                )
+                _format_versions(candidates.iter_all()),
             )
 
             raise DistributionNotFound(
@@ -716,15 +773,14 @@ class PackageFinder(object):
                 'Installed version (%s) is most up-to-date (past versions: '
                 '%s)',
                 installed_version,
-                ', '.join(sorted(compatible_versions, key=parse_version)) or
-                "none",
+                _format_versions(candidates.iter_applicable()),
             )
             raise BestVersionAlreadyInstalled
 
         logger.debug(
             'Using version %s (newest of versions: %s)',
             best_candidate.version,
-            ', '.join(sorted(compatible_versions, key=parse_version))
+            _format_versions(candidates.iter_applicable()),
         )
         return best_candidate.location
 

src/pip/_internal/utils/outdated.py

@@ -129,12 +129,10 @@ def pip_version_check(session, options):
                 trusted_hosts=options.trusted_hosts,
                 session=session,
             )
-            all_candidates = finder.find_all_candidates("pip")
-            if not all_candidates:
+            candidate = finder.find_candidates("pip").get_best()
+            if candidate is None:
                 return
-            pypi_version = str(
-                max(all_candidates, key=lambda c: c.version).version
-            )
+            pypi_version = str(candidate.version)
 
             # save that we've performed a check
             state.save(pypi_version, current_time)

tests/unit/test_unit_outdated.py

@@ -12,6 +12,14 @@ from pip._internal.index import InstallationCandidate
 from pip._internal.utils import outdated
 
 
+class MockFoundCandidates(object):
+    def __init__(self, best):
+        self._best = best
+
+    def get_best(self):
+        return self._best
+
+
 class MockPackageFinder(object):
 
     BASE_URL = 'https://pypi.org/simple/pip-{0}.tar.gz'
@@ -28,8 +36,8 @@ class MockPackageFinder(object):
     def __init__(self, *args, **kwargs):
         pass
 
-    def find_all_candidates(self, project_name):
-        return self.INSTALLATION_CANDIDATES
+    def find_candidates(self, project_name):
+        return MockFoundCandidates(self.INSTALLATION_CANDIDATES[0])
 
 
 class MockDistribution(object):