Merge pull request #2153 from nathanwilliams-ct/nathan/enum-static-type-checking
Static Type Checking: Enums
diff --git a/doc/source/hacking/using_the_testsuite.rst b/doc/source/hacking/using_the_testsuite.rst
index 418bdb7..9302b01 100644
--- a/doc/source/hacking/using_the_testsuite.rst
+++ b/doc/source/hacking/using_the_testsuite.rst
@@ -190,6 +190,13 @@
.. _contributing_formatting_code:
+Running Static Type Checkers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Static Type Checking is performed separately from testing. In order to run the static type checking step which
+consists of running the ``mypy`` tool, run the following::
+
+ tox -e mypy
+
Formatting code
~~~~~~~~~~~~~~~
Similar to linting, code formatting is also done via a ``tox`` environment. To
@@ -197,6 +204,8 @@
tox -e format
+In CI `tox -e format-check` is used to ensure formatting has been run.
+
Observing coverage
~~~~~~~~~~~~~~~~~~
Once you have run the tests using `tox` (or `detox`), some coverage reports will
diff --git a/src/buildstream/_elementproxy.py b/src/buildstream/_elementproxy.py
index 3425b6c..dc7f030 100644
--- a/src/buildstream/_elementproxy.py
+++ b/src/buildstream/_elementproxy.py
@@ -95,7 +95,7 @@
sandbox: "Sandbox",
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True
@@ -120,7 +120,7 @@
selection: Optional[Sequence["Element"]] = None,
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True
@@ -168,7 +168,7 @@
sandbox: "Sandbox",
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True,
diff --git a/src/buildstream/_overlapcollector.py b/src/buildstream/_overlapcollector.py
index 79f62b1..d1f5dc0 100644
--- a/src/buildstream/_overlapcollector.py
+++ b/src/buildstream/_overlapcollector.py
@@ -66,7 +66,7 @@
# location (str): The Sandbox relative location this session was created for
#
@contextmanager
- def session(self, action: str, location: Optional[str]):
+ def session(self, action: OverlapAction, location: Optional[str]):
assert self._session is None, "Stage session already started"
if location is None:
@@ -108,13 +108,13 @@
# location (str): The Sandbox relative location this session was created for
#
class OverlapCollectorSession:
- def __init__(self, element: "Element", action: str, location: str):
+ def __init__(self, element: "Element", action: OverlapAction, location: str):
# The Element we are staging for, on which we'll issue warnings
self._element = element # type: Element
# The OverlapAction for this session
- self._action = action # type: str
+ self._action = action # type: OverlapAction
# The Sandbox relative directory this session was created for
self._location = location # type: str
diff --git a/src/buildstream/_pipeline.py b/src/buildstream/_pipeline.py
index 8a8a9e9..da54de2 100644
--- a/src/buildstream/_pipeline.py
+++ b/src/buildstream/_pipeline.py
@@ -44,7 +44,7 @@
# Yields:
# Elements in the scope of the specified target elements
#
-def dependencies(targets: List[Element], scope: int, *, recurse: bool = True) -> Iterator[Element]:
+def dependencies(targets: List[Element], scope: _Scope, *, recurse: bool = True) -> Iterator[Element]:
# Keep track of 'visited' in this scope, so that all targets
# share the same context.
visited = (BitMap(), BitMap())
@@ -73,7 +73,12 @@
# A list of Elements appropriate for the specified selection mode
#
def get_selection(
- context: Context, targets: List[Element], mode: str, *, silent: bool = True, depth_sort: bool = False
+ context: Context,
+ targets: List[Element],
+ mode: _PipelineSelection,
+ *,
+ silent: bool = True,
+ depth_sort: bool = False
) -> List[Element]:
def redirect_and_log() -> List[Element]:
# Redirect and log if permitted
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index 913bc3f..9197107 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -153,7 +153,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
except_targets: Iterable[str] = (),
load_artifacts: bool = False,
connect_artifact_cache: bool = False,
@@ -259,7 +259,7 @@
def shell(
self,
target: str,
- scope: int,
+ scope: _Scope,
prompt: Callable[[Element], str],
*,
unique_id: Optional[str] = None,
@@ -382,7 +382,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
ignore_junction_targets: bool = False,
artifact_remotes: Iterable[RemoteSpec] = (),
source_remotes: Iterable[RemoteSpec] = (),
@@ -456,7 +456,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
except_targets: Iterable[str] = (),
source_remotes: Iterable[RemoteSpec] = (),
ignore_project_source_remotes: bool = False,
@@ -579,7 +579,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
ignore_junction_targets: bool = False,
artifact_remotes: Iterable[RemoteSpec] = (),
ignore_project_artifact_remotes: bool = False,
@@ -633,7 +633,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
ignore_junction_targets: bool = False,
artifact_remotes: Iterable[RemoteSpec] = (),
ignore_project_artifact_remotes: bool = False,
@@ -688,7 +688,7 @@
*,
location: Optional[str] = None,
force: bool = False,
- selection: str = _PipelineSelection.RUN,
+ selection: _PipelineSelection = _PipelineSelection.RUN,
integrate: bool = True,
hardlinks: bool = False,
compression: str = "",
@@ -1668,7 +1668,7 @@
self,
targets: Iterable[str],
*,
- selection: str = _PipelineSelection.NONE,
+ selection: _PipelineSelection = _PipelineSelection.NONE,
except_targets: Iterable[str] = (),
ignore_junction_targets: bool = False,
dynamic_plan: bool = False,
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 6ac1ff1..209b9e9 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -90,7 +90,7 @@
from .sandbox import _SandboxFlags, SandboxCommandError
from .sandbox._config import SandboxConfig
from .sandbox._sandboxremote import SandboxRemote
-from .types import _Scope, _CacheBuildTrees, _KeyStrength, OverlapAction, _DisplayKey
+from .types import _HostMount, _Scope, _CacheBuildTrees, _KeyStrength, OverlapAction, _DisplayKey
from ._artifact import Artifact
from ._elementproxy import ElementProxy
from ._elementsources import ElementSources
@@ -604,7 +604,7 @@
sandbox: "Sandbox",
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True,
@@ -664,7 +664,7 @@
selection: Optional[Sequence["Element"]] = None,
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True,
@@ -864,7 +864,7 @@
# Yields:
# (Element): The dependencies in `scope`, in deterministic staging order
#
- def _dependencies(self, scope, *, recurse=True, visited=None):
+ def _dependencies(self, scope: _Scope, *, recurse=True, visited=None):
# The format of visited is (BitMap(), BitMap()), with the first BitMap
# containing element that have been visited for the `_Scope.BUILD` case
@@ -971,7 +971,7 @@
sandbox: "Sandbox",
*,
path: Optional[str] = None,
- action: str = OverlapAction.WARNING,
+ action: OverlapAction = OverlapAction.WARNING,
include: Optional[List[str]] = None,
exclude: Optional[List[str]] = None,
orphans: bool = True,
@@ -2060,7 +2060,16 @@
# usebuildtree (bool): Use the buildtree as its source
#
# Returns: Exit code
- def _shell(self, scope=None, *, mounts=None, isolate=False, prompt=None, command=None, usebuildtree=False):
+ def _shell(
+ self,
+ scope: _Scope | None = None,
+ *,
+ mounts: List[_HostMount] | None = None,
+ isolate: bool = False,
+ prompt: str | None = None,
+ command: List[str] | None = None,
+ usebuildtree: bool = False,
+ ):
with self._prepare_sandbox(scope, shell=True, usebuildtree=usebuildtree) as sandbox:
environment = sandbox._get_configured_environment() or self.get_environment()
diff --git a/src/buildstream/types.py b/src/buildstream/types.py
index a21e1dc..bd69dd2 100644
--- a/src/buildstream/types.py
+++ b/src/buildstream/types.py
@@ -35,6 +35,9 @@
:class:`enum.Enum` attributes accesses can be really slow, and slow down the execution noticeably.
This reimplementation doesn't suffer the same problems, but also does not reimplement everything.
+
+ For mypy Enum static type checking support, all FastEnum should be stubbed as inheriting from enum.Enum.
+ Use `stubgen src/buildstream/types.py --include-docstrings` to generate the stubs, add `from enum import Enum` and replace all `(FastEnum)` with `(Enum)`
"""
name = None
@@ -61,7 +64,7 @@
try:
return cls._value_to_entry[value]
except KeyError:
- if type(value) is cls: # pylint: disable=unidiomatic-typecheck
+ if isinstance(value, cls): # pylint: disable=unidiomatic-typecheck
return value
raise ValueError("Unknown enum value: {}".format(value))
@@ -172,7 +175,6 @@
# Defines the scope of dependencies to include for a given element
# when iterating over the dependency graph in APIs like
# Element._dependencies().
-#
class _Scope(FastEnum):
# All elements which the given element depends on, following
@@ -384,7 +386,7 @@
alias_node: MappingNode = node.get_mapping("aliases")
for alias, uris in alias_node.items():
- assert type(uris) is SequenceNode # pylint: disable=unidiomatic-typecheck
+ assert isinstance(uris, SequenceNode) # pylint: disable=unidiomatic-typecheck
aliases[alias] = uris.as_str_list()
return cls(name, aliases)
diff --git a/src/buildstream/types.pyi b/src/buildstream/types.pyi
new file mode 100644
index 0000000..8a75b9f
--- /dev/null
+++ b/src/buildstream/types.pyi
@@ -0,0 +1,177 @@
+"""
+Foundation type stubs
+================
+
+See src/buildstream/types.py
+
+These stubs are used to replace FastEnum with Enum when doing static type checking.
+
+Buildstream implements a custom `FastEnum`[1].
+mypy only supports `enum.Enum` (and it's official variations) when it comes to doing static type checks on enums [2].
+We can't subclass Enum in FastEnum to make mypy happy, because Enum doesn't allow subclassing [3].
+So we end up with a bunch of `str`, `int` etc around the codebase,
+instead of the appropriate Enum classes like `OverlapAction` or `_Scope` which are usually recorded separately in the doc strings.
+This means we don't get proper static type checking on our Enums :( .
+With stub files [4][5] we can lie to mypy about what type the Enum classes are[6],
+so the type hints around the codebase are now correct be corrected.
+Now we can have proper static type checking on the custom Enums, Yay!
+
+So
+
+```
+class OverlapAction(FastEnum):
+ ERROR: str
+ WARNING: str
+ IGNORE: str
+```
+
+gets stubbed as
+
+```
+from enum import Enum
+class OverlapAction(Enum):
+ ERROR: str
+ WARNING: str
+ IGNORE: str
+```
+
+[1] src/buildstream/types.py#L32
+[2] https://mypy.readthedocs.io/en/stable/literal_types.html#enums
+[3] https://docs.python.org/3/howto/enum.html#restricted-enum-subclassing
+[4] https://typing.python.org/en/latest/guides/writing_stubs.html
+[5] https://mypy.readthedocs.io/en/stable/stubgen.html
+[6] https://github.com/python/mypy/issues/3217
+
+"""
+
+from ._types import MetaFastEnum as MetaFastEnum
+from .node import MappingNode as MappingNode, SequenceNode as SequenceNode
+from _typeshed import Incomplete
+from typing import Any
+from enum import Enum
+
+class FastEnum(metaclass=MetaFastEnum):
+ """
+ A reimplementation of a subset of the `Enum` functionality, which is far quicker than `Enum`.
+
+ :class:`enum.Enum` attributes accesses can be really slow, and slow down the execution noticeably.
+ This reimplementation doesn't suffer the same problems, but also does not reimplement everything.
+
+ For mypy Enum static type checking support, all FastEnum should be stubbed as inheriting from enum.Enum.
+ Use `stubgen src/buildstream/types.py --include-docstrings` to generate the stubs, add `from enum import Enum` and replace all `(FastEnum)` with `(Enum)`
+ """
+
+ name: Incomplete
+ value: Incomplete
+ @classmethod
+ def values(cls):
+ """Get all the possible values for the enum.
+
+ Returns:
+ list: the list of all possible values for the enum
+ """
+
+ def __new__(cls, value): ...
+ def __eq__(self, other): ...
+ def __ne__(self, other): ...
+ def __hash__(self): ...
+ def __reduce__(self): ...
+
+class CoreWarnings:
+ """CoreWarnings()
+
+ Some common warnings which are raised by core functionalities within BuildStream are found in this class.
+ """
+
+ OVERLAPS: str
+ UNSTAGED_FILES: str
+ REF_NOT_IN_TRACK: str
+ UNALIASED_URL: str
+ UNAVAILABLE_SOURCE_INFO: str
+
+class OverlapAction(Enum):
+ """OverlapAction()
+
+ Defines what action to take when files staged into the sandbox overlap.
+
+ .. note::
+
+ This only dictates what happens when functions such as
+ :func:`Element.stage_artifact() <buildstream.element.Element.stage_artifact>` and
+ :func:`Element.stage_dependency_artifacts() <buildstream.element.Element.stage_dependency_artifacts>`
+ are called multiple times in an Element's :func:`Element.stage() <buildstream.element.Element.stage>`
+ implementation, and the files staged from one function call result in overlapping files staged
+ from previous invocations.
+
+ If multiple staged elements overlap eachother within a single call to
+ :func:`Element.stage_dependency_artifacts() <buildstream.element.Element.stage_dependency_artifacts>`,
+ then the :ref:`overlap whitelist <public_overlap_whitelist>` will be ovserved, and warnings will
+ be issued for overlapping files, which will be fatal warnings if
+ :attr:`CoreWarnings.OVERLAPS <buildstream.types.CoreWarnings.OVERLAPS>` is specified
+ as a :ref:`fatal warning <configurable_warnings>`.
+ """
+
+ ERROR: str
+ WARNING: str
+ IGNORE: str
+
+class _Scope(Enum):
+ ALL: int
+ BUILD: int
+ RUN: int
+ NONE: int
+
+class _KeyStrength(Enum):
+ STRONG: int
+ WEAK: int
+
+class _DisplayKey:
+ full: str
+ brief: str
+ strict: bool
+ def __init__(self, full: str, brief: str, strict: bool) -> None: ...
+
+class _SchedulerErrorAction(Enum):
+ CONTINUE: str
+ QUIT: str
+ TERMINATE: str
+
+class _CacheBuildTrees(Enum):
+ ALWAYS: str
+ AUTO: str
+ NEVER: str
+
+class _SourceUriPolicy(Enum):
+ ALL: str
+ ALIASES: str
+ MIRRORS: str
+ USER: str
+
+class _PipelineSelection(Enum):
+ NONE: str
+ REDIRECT: str
+ ALL: str
+ BUILD: str
+ RUN: str
+
+class _ProjectInformation:
+ project: Incomplete
+ provenance: Incomplete
+ duplicates: Incomplete
+ internal: Incomplete
+ def __init__(self, project, provenance_node, duplicates, internal) -> None: ...
+
+class _HostMount:
+ path: str
+ host_path: str
+ optional: bool
+ def __init__(self, path: str, host_path: str | None = None, optional: bool = False) -> None: ...
+
+class _SourceMirror:
+ name: str
+ aliases: dict[str, list[str]]
+ def __init__(self, name: str, aliases: dict[str, list[str]]) -> None: ...
+ @classmethod
+ def new_from_node(cls, node: MappingNode) -> _SourceMirror: ...
+
+SourceRef = None | int | str | list[Any] | dict[str, Any]
