Address review comments

Author: pfmoore

src/pip/_internal/resolution/resolvelib/base.py

@@ -44,6 +44,11 @@ def version(self):
         # type: () -> _BaseVersion
         raise NotImplementedError("Override in subclass")
 
+    @property
+    def is_installed(self):
+        # type: () -> bool
+        raise NotImplementedError("Override in subclass")
+
     def get_dependencies(self):
         # type: () -> Sequence[Requirement]
         raise NotImplementedError("Override in subclass")

src/pip/_internal/resolution/resolvelib/candidates.py

@@ -101,15 +101,10 @@ def make_install_req_from_dist(dist, parent):
     return ireq
 
 
-def is_already_installed(cand):
-    # type: (Candidate) -> bool
-    # For an ExtrasCandidate, we check the base
-    if isinstance(cand, ExtrasCandidate):
-        cand = cand.base
-    return isinstance(cand, AlreadyInstalledCandidate)
-
-
 class _InstallRequirementBackedCandidate(Candidate):
+    # These are not installed
+    is_installed = False
+
     def __init__(
         self,
         link,          # type: Link
@@ -279,6 +274,8 @@ def _prepare_abstract_distribution(self):
 
 
 class AlreadyInstalledCandidate(Candidate):
+    is_installed = True
+
     def __init__(
         self,
         dist,  # type: Distribution
@@ -400,6 +397,11 @@ def version(self):
         # type: () -> _BaseVersion
         return self.base.version
 
+    @property
+    def is_installed(self):
+        # type: () -> _BaseVersion
+        return self.base.is_installed
+
     def get_dependencies(self):
         # type: () -> Sequence[Requirement]
         factory = self.base._factory
@@ -436,6 +438,8 @@ def get_install_requirement(self):
 
 
 class RequiresPythonCandidate(Candidate):
+    is_installed = False
+
     def __init__(self, py_version_info):
         # type: (Optional[Tuple[int, ...]]) -> None
         if py_version_info is not None:

src/pip/_internal/resolution/resolvelib/factory.py

@@ -114,6 +114,12 @@ def _make_candidate_from_link(
     def iter_found_candidates(self, ireq, extras):
         # type: (InstallRequirement, Set[str]) -> Iterator[Candidate]
         name = canonicalize_name(ireq.req.name)
+
+        # We use this to ensure that we only yield a single candidate for
+        # each version (the finder's preferred one for that version). The
+        # requirement needs to return only one candidate per version, so we
+        # implement that logic here so that requirements using this helper
+        # don't all have to do the same thing later.
         seen_versions = set()
 
         # Yield the installed version, if it matches, unless the user

src/pip/_internal/resolution/resolvelib/provider.py

@@ -3,8 +3,6 @@
 
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
-from .candidates import is_already_installed
-
 if MYPY_CHECK_RUNNING:
     from typing import Any, Dict, Optional, Sequence, Set, Tuple, Union
 
@@ -40,16 +38,16 @@ def __init__(
         constraints,  # type: Dict[str, SpecifierSet]
         ignore_dependencies,  # type: bool
         upgrade_strategy,  # type: str
-        roots,  # type: Set[str]
+        user_requested,  # type: Set[str]
     ):
         # type: (...) -> None
         self._factory = factory
         self._constraints = constraints
         self._ignore_dependencies = ignore_dependencies
         self._upgrade_strategy = upgrade_strategy
-        self.roots = roots
+        self.user_requested = user_requested
 
-    def sort_matches(self, matches):
+    def _sort_matches(self, matches):
         # type: (Sequence[Candidate]) -> Sequence[Candidate]
 
         # The requirement is responsible for returning a sequence of potential
@@ -76,28 +74,36 @@ def sort_matches(self, matches):
 
         def _eligible_for_upgrade(name):
             # type: (str) -> bool
+            """Are upgrades allowed for this project?
+
+            This checks the upgrade strategy, and whether the project was one
+            that the user specified in the command line, in order to decide
+            whether we should upgrade if there's a newer version available.
+
+            (Note that we don't need access to the `--upgrade` flag, because
+            an upgrade strategy of "to-satisfy-only" means that `--upgrade`
+            was not specified).
+            """
             if self._upgrade_strategy == "eager":
                 return True
             elif self._upgrade_strategy == "only-if-needed":
-                return (name in self.roots)
+                return (name in self.user_requested)
             return False
 
-        def keep_installed(c):
-            # type: (Candidate) -> int
-            """Give priority to an installed version?"""
-            if not is_already_installed(c):
-                return 0
-
-            if _eligible_for_upgrade(c.name):
-                return 0
+        def sort_key(c):
+            # type: (Candidate) -> Tuple[int, _BaseVersion]
+            """Return a sort key for the matches.
 
-            return 1
+            The highest priority should be given to installed candidates that
+            are not eligible for upgrade. We use the integer value in the first
+            part of the key to sort these before other candidates.
+            """
+            if c.is_installed and not _eligible_for_upgrade(c.name):
+                return (1, c.version)
 
-        def key(c):
-            # type: (Candidate) -> Tuple[int, _BaseVersion]
-            return (keep_installed(c), c.version)
+            return (0, c.version)
 
-        return sorted(matches, key=key)
+        return sorted(matches, key=sort_key)
 
     def get_install_requirement(self, c):
         # type: (Candidate) -> Optional[InstallRequirement]
@@ -121,7 +127,7 @@ def find_matches(self, requirement):
         # type: (Requirement) -> Sequence[Candidate]
         constraint = self._constraints.get(requirement.name, SpecifierSet())
         matches = requirement.find_matches(constraint)
-        return self.sort_matches(matches)
+        return self._sort_matches(matches)
 
     def is_satisfied_by(self, requirement, candidate):
         # type: (Requirement, Candidate) -> bool

src/pip/_internal/resolution/resolvelib/requirements.py

@@ -76,6 +76,10 @@ def name(self):
 
     def find_matches(self, constraint):
         # type: (SpecifierSet) -> Sequence[Candidate]
+
+        # We should only return one candidate per version, but
+        # iter_found_candidates does that for us, so we don't need
+        # to do anything special here.
         return [
             c
             for c in self._factory.iter_found_candidates(

src/pip/_internal/resolution/resolvelib/resolver.py

@@ -69,7 +69,7 @@ def resolve(self, root_reqs, check_supported_wheels):
         # type: (List[InstallRequirement], bool) -> RequirementSet
 
         constraints = {}  # type: Dict[str, SpecifierSet]
-        roots = set()
+        user_requested = set()
         requirements = []
         for req in root_reqs:
             if req.constraint:
@@ -82,7 +82,7 @@ def resolve(self, root_reqs, check_supported_wheels):
                     constraints[name] = req.specifier
             else:
                 if req.is_direct and req.name:
-                    roots.add(canonicalize_name(req.name))
+                    user_requested.add(canonicalize_name(req.name))
                 requirements.append(
                     self.factory.make_requirement_from_install_req(req)
                 )
@@ -92,7 +92,7 @@ def resolve(self, root_reqs, check_supported_wheels):
             constraints=constraints,
             ignore_dependencies=self.ignore_dependencies,
             upgrade_strategy=self.upgrade_strategy,
-            roots=roots,
+            user_requested=user_requested,
         )
         reporter = BaseReporter()
         resolver = RLResolver(provider, reporter)

tests/unit/resolution_resolvelib/conftest.py

@@ -66,5 +66,5 @@ def provider(factory):
         constraints={},
         ignore_dependencies=False,
         upgrade_strategy="to-satisfy-only",
-        roots=set(),
+        user_requested=set(),
     )