Implement PEP 685 extra normalization in resolver

news/11649.bugfix.rst

@@ -0,0 +1,5 @@
+Normalize extras according to :pep:`685` from package metadata in the resolver
+for comparison. This ensures extras are correctly compared and merged as long
+as the package providing the extra(s) is built with values normalized according
+to the standard. Note, however, that this *does not* solve cases where the
+package itself contains unnormalized extra values in the metadata.

src/pip/_internal/metadata/__init__.py

@@ -9,7 +9,7 @@
 from .base import BaseDistribution, BaseEnvironment, FilesystemWheel, MemoryWheel, Wheel
 
 if TYPE_CHECKING:
-    from typing import Protocol
+    from typing import Literal, Protocol
 else:
     Protocol = object
 
@@ -50,6 +50,7 @@ def _should_use_importlib_metadata() -> bool:
 
 
 class Backend(Protocol):
+    NAME: 'Literal["importlib", "pkg_resources"]'
     Distribution: Type[BaseDistribution]
     Environment: Type[BaseEnvironment]
 

src/pip/_internal/metadata/base.py

@@ -24,7 +24,7 @@
 
 from pip._vendor.packaging.requirements import Requirement
 from pip._vendor.packaging.specifiers import InvalidSpecifier, SpecifierSet
-from pip._vendor.packaging.utils import NormalizedName
+from pip._vendor.packaging.utils import NormalizedName, canonicalize_name
 from pip._vendor.packaging.version import LegacyVersion, Version
 
 from pip._internal.exceptions import NoneMetadataError
@@ -37,7 +37,6 @@
 from pip._internal.utils.compat import stdlib_pkgs  # TODO: Move definition here.
 from pip._internal.utils.egg_link import egg_link_path_from_sys_path
 from pip._internal.utils.misc import is_local, normalize_path
-from pip._internal.utils.packaging import safe_extra
 from pip._internal.utils.urls import url_to_path
 
 from ._json import msg_to_json
@@ -460,6 +459,19 @@ def iter_provided_extras(self) -> Iterable[str]:
 
         For modern .dist-info distributions, this is the collection of
         "Provides-Extra:" entries in distribution metadata.
+
+        The return value of this function is not particularly useful other than
+        display purposes due to backward compatibility issues and the extra
+        names being poorly normalized prior to PEP 685. If you want to perform
+        logic operations on extras, use :func:`is_extra_provided` instead.
+        """
+        raise NotImplementedError()
+
+    def is_extra_provided(self, extra: str) -> bool:
+        """Check whether an extra is provided by this distribution.
+
+        This is needed mostly for compatibility issues with pkg_resources not
+        following the extra normalization rules defined in PEP 685.
         """
         raise NotImplementedError()
 
@@ -537,10 +549,11 @@ def _iter_egg_info_extras(self) -> Iterable[str]:
         """Get extras from the egg-info directory."""
         known_extras = {""}
         for entry in self._iter_requires_txt_entries():
-            if entry.extra in known_extras:
+            extra = canonicalize_name(entry.extra)
+            if extra in known_extras:
                 continue
-            known_extras.add(entry.extra)
-            yield entry.extra
+            known_extras.add(extra)
+            yield extra
 
     def _iter_egg_info_dependencies(self) -> Iterable[str]:
         """Get distribution dependencies from the egg-info directory.
@@ -556,10 +569,11 @@ def _iter_egg_info_dependencies(self) -> Iterable[str]:
         all currently available PEP 517 backends, although not standardized.
         """
         for entry in self._iter_requires_txt_entries():
-            if entry.extra and entry.marker:
-                marker = f'({entry.marker}) and extra == "{safe_extra(entry.extra)}"'
-            elif entry.extra:
-                marker = f'extra == "{safe_extra(entry.extra)}"'
+            extra = canonicalize_name(entry.extra)
+            if extra and entry.marker:
+                marker = f'({entry.marker}) and extra == "{extra}"'
+            elif extra:
+                marker = f'extra == "{extra}"'
             elif entry.marker:
                 marker = entry.marker
             else:

src/pip/_internal/metadata/importlib/__init__.py

@@ -1,4 +1,6 @@
 from ._dists import Distribution
 from ._envs import Environment
 
-__all__ = ["Distribution", "Environment"]
+__all__ = ["NAME", "Distribution", "Environment"]
+
+NAME = "importlib"

src/pip/_internal/metadata/importlib/_dists.py

@@ -27,7 +27,6 @@
     Wheel,
 )
 from pip._internal.utils.misc import normalize_path
-from pip._internal.utils.packaging import safe_extra
 from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.utils.wheel import parse_wheel, read_wheel_metadata_file
 
@@ -208,12 +207,16 @@ def _metadata_impl(self) -> email.message.Message:
         return cast(email.message.Message, self._dist.metadata)
 
     def iter_provided_extras(self) -> Iterable[str]:
-        return (
-            safe_extra(extra) for extra in self.metadata.get_all("Provides-Extra", [])
+        return self.metadata.get_all("Provides-Extra", [])
+
+    def is_extra_provided(self, extra: str) -> bool:
+        return any(
+            canonicalize_name(provided_extra) == canonicalize_name(extra)
+            for provided_extra in self.metadata.get_all("Provides-Extra", [])
         )
 
     def iter_dependencies(self, extras: Collection[str] = ()) -> Iterable[Requirement]:
-        contexts: Sequence[Dict[str, str]] = [{"extra": safe_extra(e)} for e in extras]
+        contexts: Sequence[Dict[str, str]] = [{"extra": e} for e in extras]
         for req_string in self.metadata.get_all("Requires-Dist", []):
             req = Requirement(req_string)
             if not req.marker:

src/pip/_internal/metadata/pkg_resources.py

@@ -24,8 +24,12 @@
     Wheel,
 )
 
+__all__ = ["NAME", "Distribution", "Environment"]
+
 logger = logging.getLogger(__name__)
 
+NAME = "pkg_resources"
+
 
 class EntryPoint(NamedTuple):
     name: str
@@ -212,12 +216,16 @@ def _metadata_impl(self) -> email.message.Message:
 
     def iter_dependencies(self, extras: Collection[str] = ()) -> Iterable[Requirement]:
         if extras:  # pkg_resources raises on invalid extras, so we sanitize.
-            extras = frozenset(extras).intersection(self._dist.extras)
+            extras = frozenset(pkg_resources.safe_extra(e) for e in extras)
+            extras = extras.intersection(self._dist.extras)
         return self._dist.requires(extras)
 
     def iter_provided_extras(self) -> Iterable[str]:
         return self._dist.extras
 
+    def is_extra_provided(self, extra: str) -> bool:
+        return pkg_resources.safe_extra(extra) in self._dist.extras
+
 
 class Environment(BaseEnvironment):
     def __init__(self, ws: pkg_resources.WorkingSet) -> None:

src/pip/_internal/req/req_install.py

@@ -128,7 +128,7 @@ def __init__(
         if extras:
             self.extras = extras
         elif req:
-            self.extras = {safe_extra(extra) for extra in req.extras}
+            self.extras = req.extras
         else:
             self.extras = set()
         if markers is None and req:
@@ -272,7 +272,12 @@ def match_markers(self, extras_requested: Optional[Iterable[str]] = None) -> boo
             extras_requested = ("",)
         if self.markers is not None:
             return any(
-                self.markers.evaluate({"extra": extra}) for extra in extras_requested
+                self.markers.evaluate({"extra": extra})
+                # TODO: Remove these two variants when packaging is upgraded to
+                # support the marker comparison logic specified in PEP 685.
+                or self.markers.evaluate({"extra": safe_extra(extra)})
+                or self.markers.evaluate({"extra": canonicalize_name(extra)})
+                for extra in extras_requested
             )
         else:
             return True

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

@@ -1,7 +1,7 @@
 from typing import FrozenSet, Iterable, Optional, Tuple, Union
 
 from pip._vendor.packaging.specifiers import SpecifierSet
-from pip._vendor.packaging.utils import NormalizedName, canonicalize_name
+from pip._vendor.packaging.utils import NormalizedName
 from pip._vendor.packaging.version import LegacyVersion, Version
 
 from pip._internal.models.link import Link, links_equivalent
@@ -12,11 +12,11 @@
 CandidateVersion = Union[LegacyVersion, Version]
 
 
-def format_name(project: str, extras: FrozenSet[str]) -> str:
+def format_name(project: NormalizedName, extras: FrozenSet[NormalizedName]) -> str:
     if not extras:
         return project
-    canonical_extras = sorted(canonicalize_name(e) for e in extras)
-    return "{}[{}]".format(project, ",".join(canonical_extras))
+    extras_expr = ",".join(sorted(extras))
+    return f"{project}[{extras_expr}]"
 
 
 class Constraint:

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

@@ -429,7 +429,15 @@ def __init__(
         extras: FrozenSet[str],
     ) -> None:
         self.base = base
-        self.extras = extras
+        self.extras = frozenset(canonicalize_name(e) for e in extras)
+        # If any extras are requested in their non-normalized forms, keep track
+        # of their raw values. This is needed when we look up dependencies
+        # since PEP 685 has not been implemented for marker-matching, and using
+        # the non-normalized extra for lookup ensures the user can select a
+        # non-normalized extra in a package with its non-normalized form.
+        # TODO: Remove this attribute when packaging is upgraded to support the
+        # marker comparison logic specified in PEP 685.
+        self._unnormalized_extras = extras.difference(self.extras)
 
     def __str__(self) -> str:
         name, rest = str(self.base).split(" ", 1)
@@ -480,6 +488,50 @@ def is_editable(self) -> bool:
     def source_link(self) -> Optional[Link]:
         return self.base.source_link
 
+    def _warn_invalid_extras(
+        self,
+        requested: FrozenSet[str],
+        valid: FrozenSet[str],
+    ) -> None:
+        """Emit warnings for invalid extras being requested.
+
+        This emits a warning for each requested extra that is not in the
+        candidate's ``Provides-Extra`` list.
+        """
+        invalid_extras_to_warn = frozenset(
+            extra
+            for extra in requested
+            if extra not in valid
+            # If an extra is requested in an unnormalized form, skip warning
+            # about the normalized form being missing.
+            and extra in self.extras
+        )
+        if not invalid_extras_to_warn:
+            return
+        for extra in sorted(invalid_extras_to_warn):
+            logger.warning(
+                "%s %s does not provide the extra '%s'",
+                self.base.name,
+                self.version,
+                extra,
+            )
+
+    def _calculate_valid_requested_extras(self) -> FrozenSet[str]:
+        """Get a list of valid extras requested by this candidate.
+
+        The user (or upstream dependant) may have specified extras that the
+        candidate doesn't support. Any unsupported extras are dropped, and each
+        cause a warning to be logged here.
+        """
+        requested_extras = self.extras.union(self._unnormalized_extras)
+        valid_extras = frozenset(
+            extra
+            for extra in requested_extras
+            if self.base.dist.is_extra_provided(extra)
+        )
+        self._warn_invalid_extras(requested_extras, valid_extras)
+        return valid_extras
+
     def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requirement]]:
         factory = self.base._factory
 
@@ -489,18 +541,7 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen
         if not with_requires:
             return
 
-        # The user may have specified extras that the candidate doesn't
-        # support. We ignore any unsupported extras here.
-        valid_extras = self.extras.intersection(self.base.dist.iter_provided_extras())
-        invalid_extras = self.extras.difference(self.base.dist.iter_provided_extras())
-        for extra in sorted(invalid_extras):
-            logger.warning(
-                "%s %s does not provide the extra '%s'",
-                self.base.name,
-                self.version,
-                extra,
-            )
-
+        valid_extras = self._calculate_valid_requested_extras()
         for r in self.base.dist.iter_dependencies(valid_extras):
             requirement = factory.make_requirement_from_spec(
                 str(r), self.base._ireq, valid_extras

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

@@ -112,7 +112,7 @@ def __init__(
         self._editable_candidate_cache: Cache[EditableCandidate] = {}
         self._installed_candidate_cache: Dict[str, AlreadyInstalledCandidate] = {}
         self._extras_candidate_cache: Dict[
-            Tuple[int, FrozenSet[str]], ExtrasCandidate
+            Tuple[int, FrozenSet[NormalizedName]], ExtrasCandidate
         ] = {}
 
         if not ignore_installed:
@@ -138,9 +138,11 @@ def _fail_if_link_is_unsupported_wheel(self, link: Link) -> None:
         raise UnsupportedWheel(msg)
 
     def _make_extras_candidate(
-        self, base: BaseCandidate, extras: FrozenSet[str]
+        self,
+        base: BaseCandidate,
+        extras: FrozenSet[str],
     ) -> ExtrasCandidate:
-        cache_key = (id(base), extras)
+        cache_key = (id(base), frozenset(canonicalize_name(e) for e in extras))
         try:
             candidate = self._extras_candidate_cache[cache_key]
         except KeyError:

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

@@ -43,7 +43,7 @@ class SpecifierRequirement(Requirement):
     def __init__(self, ireq: InstallRequirement) -> None:
         assert ireq.link is None, "This is a link, not a specifier"
         self._ireq = ireq
-        self._extras = frozenset(ireq.extras)
+        self._extras = frozenset(canonicalize_name(e) for e in ireq.extras)
 
     def __str__(self) -> str:
         return str(self._ireq.req)