From 63e13c7c941953663d9e18f41e2e4f7ba1055889 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 14 Aug 2022 15:06:17 -0700 Subject: [PATCH 1/7] Use strict optional checking in req_install.py Suggested by pradyunsg in #11374 Since half of the API of this class depends on self.req not being None, it seems like we should just prevent users from passing None here. However, I wasn't able to make that change. Rather than sprinkle asserts everywhere, I added "checked" properties. I find this less ad hoc and easier to adapt if e.g. we're able to make self.req never None in the future. There are now some code paths where we have asserts that we didn't before. I relied on other type hints in pip's code base to be accurate. If that is not the case and we actually relied on some function being able to accept None when not typed as such, we may hit these asserts. But hopefully tests would catch such a thing. --- src/pip/_internal/req/req_install.py | 47 +++++++++++++++++----------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 88d481dfe5c..650c41ab62f 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -1,6 +1,3 @@ -# The following comment should be removed at some point in the future. -# mypy: strict-optional=False - import functools import logging import os @@ -192,7 +189,7 @@ def __str__(self) -> str: s = redact_auth_from_url(self.link.url) else: s = "" - if self.satisfied_by is not None: + if self.satisfied_by is not None and self.satisfied_by.location is not None: s += " in {}".format(display_path(self.satisfied_by.location)) if self.comes_from: if isinstance(self.comes_from, str): @@ -219,6 +216,11 @@ def format_debug(self) -> str: state=", ".join(state), ) + @property + def checked_req(self) -> Requirement: + assert self.req is not None + return self.req + # Things that are valid for all kinds of requirements? @property def name(self) -> Optional[str]: @@ -226,6 +228,11 @@ def name(self) -> Optional[str]: return None return self.req.name + @property + def checked_name(self) -> str: + assert self.req is not None + return self.req.name + @functools.lru_cache() # use cached_property in python 3.8+ def supports_pyproject_editable(self) -> bool: if not self.use_pep517: @@ -240,7 +247,7 @@ def supports_pyproject_editable(self) -> bool: @property def specifier(self) -> SpecifierSet: - return self.req.specifier + return self.checked_req.specifier @property def is_pinned(self) -> bool: @@ -290,7 +297,7 @@ def hashes(self, trust_internet: bool = True) -> Hashes: """ good_hashes = self.hash_options.copy() link = self.link if trust_internet else self.original_link - if link and link.hash: + if link and link.hash and link.hash_name: good_hashes.setdefault(link.hash_name, []).append(link.hash) return Hashes(good_hashes) @@ -300,6 +307,7 @@ def from_path(self) -> Optional[str]: return None s = str(self.req) if self.comes_from: + comes_from: Optional[str] if isinstance(self.comes_from, str): comes_from = self.comes_from else: @@ -331,7 +339,7 @@ def ensure_build_location( # When parallel builds are enabled, add a UUID to the build directory # name so multiple builds do not interfere with each other. - dir_name: str = canonicalize_name(self.name) + dir_name: str = canonicalize_name(self.checked_name) if parallel_builds: dir_name = f"{dir_name}_{uuid.uuid4().hex}" @@ -375,7 +383,7 @@ def _set_requirement(self) -> None: def warn_on_mismatching_name(self) -> None: metadata_name = canonicalize_name(self.metadata["Name"]) - if canonicalize_name(self.req.name) == metadata_name: + if canonicalize_name(self.checked_name) == metadata_name: # Everything is fine. return @@ -437,6 +445,7 @@ def is_wheel(self) -> bool: # Things valid for sdists @property def unpacked_source_directory(self) -> str: + assert self.source_dir, f"No source dir for {self}" return os.path.join( self.source_dir, self.link and self.link.subdirectory_fragment or "" ) @@ -514,7 +523,7 @@ def prepare_metadata(self) -> None: Under PEP 517 and PEP 660, call the backend hook to prepare the metadata. Under legacy processing, call setup.py egg-info. """ - assert self.source_dir + assert self.source_dir, f"No source dir for {self}" details = self.name or f"from {self.link}" if self.use_pep517: @@ -564,7 +573,7 @@ def get_dist(self) -> BaseDistribution: return get_directory_distribution(self.metadata_directory) elif self.local_file_path and self.is_wheel: return get_wheel_distribution( - FilesystemWheel(self.local_file_path), canonicalize_name(self.name) + FilesystemWheel(self.local_file_path), canonicalize_name(self.checked_name) ) raise AssertionError( f"InstallRequirement {self} has no metadata directory and no wheel: " @@ -572,9 +581,9 @@ def get_dist(self) -> BaseDistribution: ) def assert_source_matches_version(self) -> None: - assert self.source_dir + assert self.source_dir, f"No source dir for {self}" version = self.metadata["version"] - if self.req.specifier and version not in self.req.specifier: + if self.req and version not in self.specifier: logger.warning( "Requested %s, but installing version %s", self, @@ -669,7 +678,7 @@ def _clean_zip_name(name: str, prefix: str) -> str: path = os.path.join(parentdir, path) name = _clean_zip_name(path, rootdir) - return self.name + "/" + name + return self.checked_name + "/" + name def archive(self, build_dir: Optional[str]) -> None: """Saves archive to provided build_dir. @@ -750,7 +759,7 @@ def install( pycompile: bool = True, ) -> None: scheme = get_scheme( - self.name, + self.checked_name, user=use_user_site, home=home, root=root, @@ -766,7 +775,7 @@ def install( prefix=prefix, home=home, use_user_site=use_user_site, - name=self.name, + name=self.checked_name, setup_py_path=self.setup_py_path, isolated=self.isolated, build_env=self.build_env, @@ -788,7 +797,7 @@ def install( self.original_link_is_in_wheel_cache, ) install_wheel( - self.name, + self.checked_name, self.local_file_path, scheme=scheme, req_description=str(self.req), @@ -815,7 +824,7 @@ def install( self.legacy_install_reason is not None and self.legacy_install_reason.emit_before_install ): - self.legacy_install_reason.emit_deprecation(self.name) + self.legacy_install_reason.emit_deprecation(self.checked_name) success = install_legacy( install_options=install_options, global_options=global_options, @@ -827,7 +836,7 @@ def install( scheme=scheme, setup_py_path=self.setup_py_path, isolated=self.isolated, - req_name=self.name, + req_name=self.checked_name, build_env=self.build_env, unpacked_source_directory=self.unpacked_source_directory, req_description=str(self.req), @@ -846,7 +855,7 @@ def install( and self.legacy_install_reason is not None and self.legacy_install_reason.emit_after_success ): - self.legacy_install_reason.emit_deprecation(self.name) + self.legacy_install_reason.emit_deprecation(self.checked_name) def check_invalid_constraint_type(req: InstallRequirement) -> str: From 44ffeb28fa702eace8d5eef45ceeb0ab70ad4e34 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 14 Aug 2022 15:18:30 -0700 Subject: [PATCH 2/7] news --- news/85F7E260-68FF-4C1E-A2CB-CF8634829D2D.trivial.rst | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 news/85F7E260-68FF-4C1E-A2CB-CF8634829D2D.trivial.rst diff --git a/news/85F7E260-68FF-4C1E-A2CB-CF8634829D2D.trivial.rst b/news/85F7E260-68FF-4C1E-A2CB-CF8634829D2D.trivial.rst new file mode 100644 index 00000000000..e69de29bb2d From ef6152dff3b602331d70480e2603140a6795ae93 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 14 Aug 2022 15:20:03 -0700 Subject: [PATCH 3/7] black --- src/pip/_internal/req/req_install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 650c41ab62f..d0e3826300b 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -573,7 +573,8 @@ def get_dist(self) -> BaseDistribution: return get_directory_distribution(self.metadata_directory) elif self.local_file_path and self.is_wheel: return get_wheel_distribution( - FilesystemWheel(self.local_file_path), canonicalize_name(self.checked_name) + FilesystemWheel(self.local_file_path), + canonicalize_name(self.checked_name), ) raise AssertionError( f"InstallRequirement {self} has no metadata directory and no wheel: " From 156ba98d5081f82d9cbadc7de58a3b88cfdb626c Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Mon, 15 Aug 2022 15:48:26 -0700 Subject: [PATCH 4/7] inline asserts --- src/pip/_internal/req/req_install.py | 42 +++++++++++++--------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index d0e3826300b..e2e24556a68 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -216,11 +216,6 @@ def format_debug(self) -> str: state=", ".join(state), ) - @property - def checked_req(self) -> Requirement: - assert self.req is not None - return self.req - # Things that are valid for all kinds of requirements? @property def name(self) -> Optional[str]: @@ -228,11 +223,6 @@ def name(self) -> Optional[str]: return None return self.req.name - @property - def checked_name(self) -> str: - assert self.req is not None - return self.req.name - @functools.lru_cache() # use cached_property in python 3.8+ def supports_pyproject_editable(self) -> bool: if not self.use_pep517: @@ -247,7 +237,8 @@ def supports_pyproject_editable(self) -> bool: @property def specifier(self) -> SpecifierSet: - return self.checked_req.specifier + assert self.req is not None + return self.req.specifier @property def is_pinned(self) -> bool: @@ -255,7 +246,8 @@ def is_pinned(self) -> bool: For example, some-package==1.2 is pinned; some-package>1.2 is not. """ - specifiers = self.specifier + assert self.req is not None + specifiers = self.req.specifier return len(specifiers) == 1 and next(iter(specifiers)).operator in {"==", "==="} def match_markers(self, extras_requested: Optional[Iterable[str]] = None) -> bool: @@ -339,7 +331,7 @@ def ensure_build_location( # When parallel builds are enabled, add a UUID to the build directory # name so multiple builds do not interfere with each other. - dir_name: str = canonicalize_name(self.checked_name) + dir_name: str = canonicalize_name(self.req.name) if parallel_builds: dir_name = f"{dir_name}_{uuid.uuid4().hex}" @@ -382,8 +374,9 @@ def _set_requirement(self) -> None: ) def warn_on_mismatching_name(self) -> None: + assert self.req is not None metadata_name = canonicalize_name(self.metadata["Name"]) - if canonicalize_name(self.checked_name) == metadata_name: + if canonicalize_name(self.req.name) == metadata_name: # Everything is fine. return @@ -572,9 +565,10 @@ def get_dist(self) -> BaseDistribution: if self.metadata_directory: return get_directory_distribution(self.metadata_directory) elif self.local_file_path and self.is_wheel: + assert self.req is not None return get_wheel_distribution( FilesystemWheel(self.local_file_path), - canonicalize_name(self.checked_name), + canonicalize_name(self.req.name), ) raise AssertionError( f"InstallRequirement {self} has no metadata directory and no wheel: " @@ -584,7 +578,7 @@ def get_dist(self) -> BaseDistribution: def assert_source_matches_version(self) -> None: assert self.source_dir, f"No source dir for {self}" version = self.metadata["version"] - if self.req and version not in self.specifier: + if self.req and version not in self.req.specifier: logger.warning( "Requested %s, but installing version %s", self, @@ -677,9 +671,10 @@ def _clean_zip_name(name: str, prefix: str) -> str: name = name.replace(os.path.sep, "/") return name + assert self.req is not None path = os.path.join(parentdir, path) name = _clean_zip_name(path, rootdir) - return self.checked_name + "/" + name + return self.req.name + "/" + name def archive(self, build_dir: Optional[str]) -> None: """Saves archive to provided build_dir. @@ -759,8 +754,9 @@ def install( use_user_site: bool = False, pycompile: bool = True, ) -> None: + assert self.req is not None scheme = get_scheme( - self.checked_name, + self.req.name, user=use_user_site, home=home, root=root, @@ -776,7 +772,7 @@ def install( prefix=prefix, home=home, use_user_site=use_user_site, - name=self.checked_name, + name=self.req.name, setup_py_path=self.setup_py_path, isolated=self.isolated, build_env=self.build_env, @@ -798,7 +794,7 @@ def install( self.original_link_is_in_wheel_cache, ) install_wheel( - self.checked_name, + self.req.name, self.local_file_path, scheme=scheme, req_description=str(self.req), @@ -825,7 +821,7 @@ def install( self.legacy_install_reason is not None and self.legacy_install_reason.emit_before_install ): - self.legacy_install_reason.emit_deprecation(self.checked_name) + self.legacy_install_reason.emit_deprecation(self.req.name) success = install_legacy( install_options=install_options, global_options=global_options, @@ -837,7 +833,7 @@ def install( scheme=scheme, setup_py_path=self.setup_py_path, isolated=self.isolated, - req_name=self.checked_name, + req_name=self.req.name, build_env=self.build_env, unpacked_source_directory=self.unpacked_source_directory, req_description=str(self.req), @@ -856,7 +852,7 @@ def install( and self.legacy_install_reason is not None and self.legacy_install_reason.emit_after_success ): - self.legacy_install_reason.emit_deprecation(self.checked_name) + self.legacy_install_reason.emit_deprecation(self.req.name) def check_invalid_constraint_type(req: InstallRequirement) -> str: From f1b4be9972ddbbfc477db3cff4857eb1c2c0d144 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Mon, 15 Aug 2022 15:49:12 -0700 Subject: [PATCH 5/7] code review --- src/pip/_internal/req/req_install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index e2e24556a68..52cfb739691 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -289,7 +289,8 @@ def hashes(self, trust_internet: bool = True) -> Hashes: """ good_hashes = self.hash_options.copy() link = self.link if trust_internet else self.original_link - if link and link.hash and link.hash_name: + if link and link.hash: + assert link.hash_name is not None good_hashes.setdefault(link.hash_name, []).append(link.hash) return Hashes(good_hashes) From 339a31b5fc3957710372091fdee1f0c63148a9f6 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Mon, 19 Jun 2023 16:52:22 -0700 Subject: [PATCH 6/7] fix up merge issue --- src/pip/_internal/req/req_install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 95e9f307ad7..2e54463227c 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -812,7 +812,7 @@ def install( assert self.local_file_path install_wheel( - self.name, + self.req.name, self.local_file_path, scheme=scheme, req_description=str(self.req), From 0d82e5d932f7c9366b720bbb1fd4b54e7269c27a Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Tue, 20 Jun 2023 01:16:58 -0700 Subject: [PATCH 7/7] fix specifier bug --- src/pip/_internal/req/req_install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/req/req_install.py b/src/pip/_internal/req/req_install.py index 2e54463227c..542d6c78f96 100644 --- a/src/pip/_internal/req/req_install.py +++ b/src/pip/_internal/req/req_install.py @@ -608,7 +608,7 @@ def get_dist(self) -> BaseDistribution: def assert_source_matches_version(self) -> None: assert self.source_dir, f"No source dir for {self}" version = self.metadata["version"] - if self.req and version not in self.req.specifier: + if self.req and self.req.specifier and version not in self.req.specifier: logger.warning( "Requested %s, but installing version %s", self,