Skip to content

Commit

Permalink
pyre: correctly type functions passed to @contextlib.contextmanager.
Browse files Browse the repository at this point in the history
Summary:
This seems to be a fairly common mistake: the `contextlib.contextmanager` decorator is applied to functions that return an `Iterator`, and turns them into ContextManagers. The decorated functions should *not* be typed to return ContextManager.

This changes typing so that it can be properly applied.

Test Plan: CI

Differential Revision: D50840616

fbshipit-source-id: 404f1d39b5bda77abd399006dac11f1e0111824c
  • Loading branch information
Flameeyes authored and facebook-github-bot committed Nov 2, 2023
1 parent 7bb401c commit bc1756a
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 20 deletions.
17 changes: 4 additions & 13 deletions antlir/rpm/storage/cli_object_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import uuid
from abc import abstractmethod
from contextlib import contextmanager
from typing import ContextManager, List, Mapping, NamedTuple
from typing import Iterator, List, Mapping, NamedTuple

from antlir.common import check_popen_returncode, get_logger

Expand Down Expand Up @@ -79,7 +79,7 @@ def _make_storage_id(cls) -> str:
return str(uuid.uuid4()).replace("-", "")

@contextmanager
def writer(self) -> ContextManager[StorageOutput]:
def writer(self) -> Iterator[StorageOutput]:
sid = self._make_storage_id()
path = self._path_for_storage_id(sid)
log_prefix = f"{self.__class__.__name__}"
Expand Down Expand Up @@ -136,16 +136,12 @@ def get_id_and_release_resources():
# pyre-fixme[6]: Expected `ContextManager[typing.Any]` for 2nd
# param but got `() -> Any`.
with _CommitCallback(self, get_id_and_release_resources) as commit:
# pyre-fixme[7]: Expected
# `ContextManager[antlir.rpm.storage.storage.StorageOutput]`
# but got `Generator[antlir.rpm.storage.storage.StorageOutput,
# None, None]`.
# pyre-fixme[6]: Expected `IO[typing.Any]` for 1st param but got
# `Optional[typing.IO[typing.Any]]`.
yield StorageOutput(output=proc.stdin, commit_callback=commit)

@contextmanager
def reader(self, sid: str) -> ContextManager[StorageInput]:
def reader(self, sid: str) -> Iterator[StorageInput]:
# We currently waste significant time per read waiting for CLIs
# to start, which is terrible for small reads (most system
# RPMs are small).
Expand All @@ -157,9 +153,6 @@ def reader(self, sid: str) -> ContextManager[StorageInput]:
stdout=subprocess.PIPE,
) as proc:
log.debug(f"{log_prefix} - Started {path} GET proc")
# pyre-fixme[7]: Expected
# `ContextManager[antlir.rpm.storage.storage.StorageInput]` but got
# `Generator[antlir.rpm.storage.storage.StorageInput, None, None]`.
# pyre-fixme[6]: Expected `IO[typing.Any]` for 1st param but got
# `Optional[typing.IO[typing.Any]]`.
yield StorageInput(input=proc.stdout)
Expand All @@ -169,11 +162,9 @@ def reader(self, sid: str) -> ContextManager[StorageInput]:
check_popen_returncode(proc)

@contextmanager
def remover(self) -> ContextManager[_StorageRemover]:
def remover(self) -> Iterator[_StorageRemover]:
rm = _StorageRemover(storage=self, procs=[])
try:
# pyre-fixme[7]: Expected `ContextManager[_StorageRemover]` but got
# `Generator[_StorageRemover, None, None]`.
yield rm
finally:
last_ex = None # We'll re-raise the last exception.
Expand Down
10 changes: 3 additions & 7 deletions antlir/rpm/storage/filesystem_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import stat
import uuid
from contextlib import contextmanager
from typing import AnyStr, ContextManager
from typing import AnyStr, Iterator

from antlir.fs_utils import Path

Expand Down Expand Up @@ -43,7 +43,7 @@ def _path_for_storage_id(self, sid: str) -> str:
return self.base_dir / sid[:3] / sid[3:6] / sid[6:9] / sid[9:]

@contextmanager
def writer(self) -> ContextManager[StorageOutput]:
def writer(self) -> Iterator[StorageOutput]:
sid = str(uuid.uuid4()).replace("-", "")
sid_path = self._path_for_storage_id(sid)
try:
Expand Down Expand Up @@ -75,15 +75,11 @@ def get_id_and_release_resources():
# pyre-fixme[6]: Expected `ContextManager[typing.Any]` for 2nd
# param but got `() -> Any`.
with _CommitCallback(self, get_id_and_release_resources) as commit:
# pyre-fixme[7]: Expected `ContextManager[StorageOutput]` but
# got `Generator[StorageOutput, None, None]`.
yield StorageOutput(output=outfile, commit_callback=commit)

@contextmanager
def reader(self, sid: str) -> ContextManager[StorageInput]:
def reader(self, sid: str) -> Iterator[StorageInput]:
with open(self._path_for_storage_id(self.strip_key(sid)), "rb") as inp:
# pyre-fixme[7]: Expected `ContextManager[StorageInput]` but got
# `Generator[StorageInput, None, None]`.
yield StorageInput(input=inp)

def remove(self, sid: str) -> None:
Expand Down

0 comments on commit bc1756a

Please sign in to comment.