Skip to content

Commit

Permalink
Use strict optional checking in req_install.py (pypa#11379)
Browse files Browse the repository at this point in the history
* Use strict optional checking in req_install.py

Suggested by pradyunsg in pypa#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.

* news

* black

* inline asserts

* code review

* fix up merge issue

* fix specifier bug
  • Loading branch information
hauntsaninja authored and cosmicexplorer committed Aug 9, 2023
1 parent 667f2e5 commit 6e1847b
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 13 deletions.
Empty file.
33 changes: 20 additions & 13 deletions src/pip/_internal/req/req_install.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -244,6 +241,7 @@ def supports_pyproject_editable(self) -> bool:

@property
def specifier(self) -> SpecifierSet:
assert self.req is not None
return self.req.specifier

@property
Expand All @@ -257,7 +255,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:
Expand Down Expand Up @@ -305,6 +304,7 @@ def hashes(self, trust_internet: bool = True) -> Hashes:
else:
link = None
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)

Expand All @@ -314,6 +314,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:
Expand Down Expand Up @@ -345,7 +346,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.req.name)
if parallel_builds:
dir_name = f"{dir_name}_{uuid.uuid4().hex}"

Expand Down Expand Up @@ -388,6 +389,7 @@ 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.req.name) == metadata_name:
# Everything is fine.
Expand Down Expand Up @@ -457,6 +459,7 @@ def is_wheel_from_cache(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 ""
)
Expand Down Expand Up @@ -543,7 +546,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:
Expand Down Expand Up @@ -592,18 +595,20 @@ 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.name)
FilesystemWheel(self.local_file_path),
canonicalize_name(self.req.name),
)
raise AssertionError(
f"InstallRequirement {self} has no metadata directory and no wheel: "
f"can't make a distribution."
)

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 self.req.specifier and version not in self.req.specifier:
logger.warning(
"Requested %s, but installing version %s",
self,
Expand Down Expand Up @@ -696,9 +701,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.name + "/" + name
return self.req.name + "/" + name

def archive(self, build_dir: Optional[str]) -> None:
"""Saves archive to provided build_dir.
Expand Down Expand Up @@ -777,8 +783,9 @@ def install(
use_user_site: bool = False,
pycompile: bool = True,
) -> None:
assert self.req is not None
scheme = get_scheme(
self.name,
self.req.name,
user=use_user_site,
home=home,
root=root,
Expand All @@ -792,7 +799,7 @@ def install(
prefix=prefix,
home=home,
use_user_site=use_user_site,
name=self.name,
name=self.req.name,
setup_py_path=self.setup_py_path,
isolated=self.isolated,
build_env=self.build_env,
Expand All @@ -805,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),
Expand Down

0 comments on commit 6e1847b

Please sign in to comment.