Split find_matches into generation and sorting

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

@@ -101,6 +101,14 @@ 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):
     def __init__(
         self,

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

@@ -42,8 +42,6 @@ if MYPY_CHECK_RUNNING:
 
 
 class Factory(object):
-    _allowed_strategies = {"eager", "only-if-needed", "to-satisfy-only"}
-
     def __init__(
         self,
         finder,  # type: PackageFinder
@@ -52,11 +50,9 @@ class Factory(object):
         force_reinstall,  # type: bool
         ignore_installed,  # type: bool
         ignore_requires_python,  # type: bool
-        upgrade_strategy,  # type: str
         py_version_info=None,  # type: Optional[Tuple[int, ...]]
     ):
         # type: (...) -> None
-        assert upgrade_strategy in self._allowed_strategies
 
         self.finder = finder
         self.preparer = preparer
@@ -64,9 +60,6 @@ class Factory(object):
         self._make_install_req_from_spec = make_install_req
         self._force_reinstall = force_reinstall
         self._ignore_requires_python = ignore_requires_python
-        self._upgrade_strategy = upgrade_strategy
-
-        self.root_reqs = set()  # type: Set[str]
 
         self._link_candidate_cache = {}  # type: Cache[LinkCandidate]
         self._editable_candidate_cache = {}  # type: Cache[EditableCandidate]
@@ -118,23 +111,27 @@ class Factory(object):
             return ExtrasCandidate(base, extras)
         return base
 
-    def _eligible_for_upgrade(self, dist_name):
-        # type: (str) -> bool
-        if self._upgrade_strategy == "eager":
-            return True
-        elif self._upgrade_strategy == "only-if-needed":
-            return (dist_name in self.root_reqs)
-        return False
-
     def iter_found_candidates(self, ireq, extras):
         # type: (InstallRequirement, Set[str]) -> Iterator[Candidate]
         name = canonicalize_name(ireq.req.name)
-        if not self._force_reinstall:
+        seen_versions = set()
-            installed_dist = self._installed_dists.get(name)
+
-            can_upgrade = self._eligible_for_upgrade(name)
+        # Yield the installed version, if it matches, unless the user
-        else:
+        # specified `--force-reinstall`, when we want the version from
-            installed_dist = None
+        # the index instead.
-            can_upgrade = False
+        if not self._force_reinstall and name in self._installed_dists:
+            installed_dist = self._installed_dists[name]
+            installed_version = installed_dist.parsed_version
+            if ireq.req.specifier.contains(
+                installed_version,
+                prereleases=True
+            ):
+                seen_versions.add(installed_version)
+                yield self._make_candidate_from_dist(
+                    dist=installed_dist,
+                    extras=extras,
+                    parent=ireq,
+                )
 
         found = self.finder.find_best_candidate(
             project_name=ireq.req.name,
@@ -142,40 +139,18 @@ class Factory(object):
             hashes=ireq.hashes(trust_internet=False),
         )
         for ican in found.iter_applicable():
-            if (installed_dist is not None and
+            if ican.version not in seen_versions:
-                    installed_dist.parsed_version == ican.version):
+                seen_versions.add(ican.version)
-                if can_upgrade:
+                yield self._make_candidate_from_link(
-                    yield self._make_candidate_from_dist(
+                    link=ican.link,
-                        dist=installed_dist,
+                    extras=extras,
-                        extras=extras,
+                    parent=ireq,
-                        parent=ireq,
+                    name=name,
-                    )
+                    version=ican.version,
-                continue
+                )
-            yield self._make_candidate_from_link(
-                link=ican.link,
-                extras=extras,
-                parent=ireq,
-                name=name,
-                version=ican.version,
-            )
-
-        # Return installed distribution if it matches the specifier. This is
-        # done last so the resolver will prefer it over downloading links.
-        if can_upgrade or installed_dist is None:
-            return
-        installed_version = installed_dist.parsed_version
-        if ireq.req.specifier.contains(installed_version, prereleases=True):
-            yield self._make_candidate_from_dist(
-                dist=installed_dist,
-                extras=extras,
-                parent=ireq,
-            )
 
     def make_requirement_from_install_req(self, ireq):
         # type: (InstallRequirement) -> Requirement
-        if ireq.is_direct and ireq.name:
-            self.root_reqs.add(canonicalize_name(ireq.name))
-
         if ireq.link:
             # TODO: Get name and version from ireq, if possible?
             #       Specifically, this might be needed in "name @ URL"

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

@@ -3,14 +3,35 @@ from pip._vendor.resolvelib.providers import AbstractProvider
 
 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, Tuple, Union
+    from typing import Any, Dict, Optional, Sequence, Set, Tuple, Union
 
     from pip._internal.req.req_install import InstallRequirement
+    from pip._vendor.packaging.version import _BaseVersion
 
     from .base import Requirement, Candidate
     from .factory import Factory
 
+# Notes on the relationship between the provider, the factory, and the
+# candidate and requirement classes.
+#
+# The provider is a direct implementation of the resolvelib class. Its role
+# is to deliver the API that resolvelib expects.
+#
+# Rather than work with completely abstract "requirement" and "candidate"
+# concepts as resolvelib does, pip has concrete classes implementing these two
+# ideas. The API of Requirement and Candidate objects are defined in the base
+# classes, but essentially map fairly directly to the equivalent provider
+# methods. In particular, `find_matches` and `is_satisfied_by` are
+# requirement methods, and `get_dependencies` is a candidate method.
+#
+# The factory is the interface to pip's internal mechanisms. It is stateless,
+# and is created by the resolver and held as a property of the provider. It is
+# responsible for creating Requirement and Candidate objects, and provides
+# services to those objects (access to pip's finder and preparer).
+
 
 class PipProvider(AbstractProvider):
     def __init__(
@@ -18,11 +39,66 @@ class PipProvider(AbstractProvider):
         factory,  # type: Factory
         constraints,  # type: Dict[str, SpecifierSet]
         ignore_dependencies,  # type: bool
+        upgrade_strategy,  # type: str
+        roots,  # type: Set[str]
     ):
         # type: (...) -> None
         self._factory = factory
         self._constraints = constraints
         self._ignore_dependencies = ignore_dependencies
+        self._upgrade_strategy = upgrade_strategy
+        self.roots = roots
+
+    def sort_matches(self, matches):
+        # type: (Sequence[Candidate]) -> Sequence[Candidate]
+
+        # The requirement is responsible for returning a sequence of potential
+        # candidates, one per version. The provider handles the logic of
+        # deciding the order in which these candidates should be passed to
+        # the resolver.
+
+        # The `matches` argument is a sequence of candidates, one per version,
+        # which are potential options to be installed. The requirement will
+        # have already sorted out whether to give us an already-installed
+        # candidate or a version from PyPI (i.e., it will deal with options
+        # like --force-reinstall and --ignore-installed).
+
+        # We now work out the correct order.
+        #
+        # 1. If no other considerations apply, later versions take priority.
+        # 2. An already installed distribution is preferred over any other,
+        #    unless the user has requested an upgrade.
+        #    Upgrades are allowed when:
+        #    * The --upgrade flag is set, and
+        #      - The project was specified on the command line, or
+        #      - The project is a dependency and the "eager" upgrade strategy
+        #        was requested.
+
+        def _eligible_for_upgrade(name):
+            # type: (str) -> bool
+            if self._upgrade_strategy == "eager":
+                return True
+            elif self._upgrade_strategy == "only-if-needed":
+                print(name, self.roots)
+                return (name in self.roots)
+            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
+
+            return 1
+
+        def key(c):
+            # type: (Candidate) -> Tuple[int, _BaseVersion]
+            return (keep_installed(c), c.version)
+
+        return sorted(matches, key=key)
 
     def get_install_requirement(self, c):
         # type: (Candidate) -> Optional[InstallRequirement]
@@ -45,7 +121,8 @@ class PipProvider(AbstractProvider):
     def find_matches(self, requirement):
         # type: (Requirement) -> Sequence[Candidate]
         constraint = self._constraints.get(requirement.name, SpecifierSet())
-        return requirement.find_matches(constraint)
+        matches = requirement.find_matches(constraint)
+        return self.sort_matches(matches)
 
     def is_satisfied_by(self, requirement, candidate):
         # type: (Requirement, Candidate) -> bool

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

@@ -32,6 +32,8 @@ logger = logging.getLogger(__name__)
 
 
 class Resolver(BaseResolver):
+    _allowed_strategies = {"eager", "only-if-needed", "to-satisfy-only"}
+
     def __init__(
         self,
         preparer,  # type: RequirementPreparer
@@ -47,6 +49,9 @@ class Resolver(BaseResolver):
         py_version_info=None,  # type: Optional[Tuple[int, ...]]
     ):
         super(Resolver, self).__init__()
+
+        assert upgrade_strategy in self._allowed_strategies
+
         self.factory = Factory(
             finder=finder,
             preparer=preparer,
@@ -54,23 +59,17 @@ class Resolver(BaseResolver):
             force_reinstall=force_reinstall,
             ignore_installed=ignore_installed,
             ignore_requires_python=ignore_requires_python,
-            upgrade_strategy=upgrade_strategy,
             py_version_info=py_version_info,
         )
         self.ignore_dependencies = ignore_dependencies
+        self.upgrade_strategy = upgrade_strategy
         self._result = None  # type: Optional[Result]
 
     def resolve(self, root_reqs, check_supported_wheels):
         # type: (List[InstallRequirement], bool) -> RequirementSet
 
-        # The factory should not have retained state from any previous usage.
-        # In theory this could only happen if self was reused to do a second
-        # resolve, which isn't something we do at the moment. We assert here
-        # in order to catch the issue if that ever changes.
-        # The persistent state that we care about is `root_reqs`.
-        assert len(self.factory.root_reqs) == 0, "Factory is being re-used"
-
         constraints = {}  # type: Dict[str, SpecifierSet]
+        roots = set()
         requirements = []
         for req in root_reqs:
             if req.constraint:
@@ -82,6 +81,8 @@ class Resolver(BaseResolver):
                 else:
                     constraints[name] = req.specifier
             else:
+                if req.is_direct and req.name:
+                    roots.add(canonicalize_name(req.name))
                 requirements.append(
                     self.factory.make_requirement_from_install_req(req)
                 )
@@ -90,6 +91,8 @@ class Resolver(BaseResolver):
             factory=self.factory,
             constraints=constraints,
             ignore_dependencies=self.ignore_dependencies,
+            upgrade_strategy=self.upgrade_strategy,
+            roots=roots,
         )
         reporter = BaseReporter()
         resolver = RLResolver(provider, reporter)

tests/unit/resolution_resolvelib/conftest.py

@@ -55,7 +55,6 @@ def factory(finder, preparer):
         force_reinstall=False,
         ignore_installed=False,
         ignore_requires_python=False,
-        upgrade_strategy="to-satisfy-only",
         py_version_info=None,
     )
 
@@ -66,4 +65,6 @@ def provider(factory):
         factory=factory,
         constraints={},
         ignore_dependencies=False,
+        upgrade_strategy="to-satisfy-only",
+        roots=set(),
     )