From 4cf6c3197d6b467a11136e7bff87aef001eab1db Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sat, 25 Jul 2020 16:45:45 -0600 Subject: [PATCH 1/2] Simplify Collection. Collection and its subclasses are now just new-types for tuple removing the previously awkward `dependencies` field exposed for the engine. The engine is updated to handle these Collections as Iterable Python objects when attempting to collect their contained values. --- src/python/pants/base/specs.py | 6 ++--- src/python/pants/engine/collection.py | 34 ++++++++++++------------ src/rust/engine/src/externs/interface.rs | 4 +-- src/rust/engine/src/externs/mod.rs | 34 +++++++++++++++++++++++- src/rust/engine/src/intrinsics.rs | 2 +- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/python/pants/base/specs.py b/src/python/pants/base/specs.py index 77a5418b05b..3b13c97c8ec 100644 --- a/src/python/pants/base/specs.py +++ b/src/python/pants/base/specs.py @@ -414,14 +414,12 @@ class FilesystemSpecs(Collection[FilesystemSpec]): @memoized_property def includes(self) -> Tuple[Union[FilesystemLiteralSpec, FilesystemGlobSpec], ...]: return tuple( - spec - for spec in self.dependencies - if isinstance(spec, (FilesystemGlobSpec, FilesystemLiteralSpec)) + spec for spec in self if isinstance(spec, (FilesystemGlobSpec, FilesystemLiteralSpec)) ) @memoized_property def ignores(self) -> Tuple[FilesystemIgnoreSpec, ...]: - return tuple(spec for spec in self.dependencies if isinstance(spec, FilesystemIgnoreSpec)) + return tuple(spec for spec in self if isinstance(spec, FilesystemIgnoreSpec)) @staticmethod def _generate_path_globs( diff --git a/src/python/pants/engine/collection.py b/src/python/pants/engine/collection.py index 1876d141ac4..f20ae3915c0 100644 --- a/src/python/pants/engine/collection.py +++ b/src/python/pants/engine/collection.py @@ -1,14 +1,14 @@ # Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -from typing import Any, ClassVar, Iterable, Sequence, TypeVar, Union, overload +from typing import Any, ClassVar, Iterable, Tuple, TypeVar, Union, cast, overload from pants.util.ordered_set import FrozenOrderedSet T = TypeVar("T") -class Collection(Sequence[T]): +class Collection(Tuple[T, ...]): """A light newtype around immutable sequences for use with the V2 engine. This should be subclassed when you want to create a distinct collection type, such as: @@ -21,11 +21,6 @@ class Examples(Collection[Example]): pass """ - def __init__(self, dependencies: Iterable[T] = ()) -> None: - # TODO: rename to `items`, `elements`, or even make this private. Python consumers should - # not directly access this. - self.dependencies = tuple(dependencies) - @overload # noqa: F811 def __getitem__(self, index: int) -> T: ... @@ -35,23 +30,28 @@ def __getitem__(self, index: slice) -> "Collection[T]": ... def __getitem__(self, index: Union[int, slice]) -> Union[T, "Collection[T]"]: # noqa: F811 + result = super().__getitem__(index) if isinstance(index, int): - return self.dependencies[index] - return self.__class__(self.dependencies[index]) - - def __len__(self) -> int: - return len(self.dependencies) + return cast(T, result) + return self.__class__(cast(Tuple[T, ...], result)) - def __eq__(self, other: Union[Any, "Collection"]) -> bool: + def __eq__(self, other: Any) -> bool: if not isinstance(other, self.__class__): return NotImplemented - return self.dependencies == other.dependencies + return super().__eq__(other) + + def __ne__(self, other: Any) -> bool: + # We must explicitly override to provide the inverse of _our_ __eq__ and not get the + # inverse of tuple.__eq__. + return not self == other - def __hash__(self) -> int: - return hash(self.dependencies) + # Unlike in Python 2 we must explicitly implement __hash__ since we explicitly implement __eq__ + # per the Python 3 data model. + # See: https://docs.python.org/3/reference/datamodel.html#object.__hash__ + __hash__ = Tuple.__hash__ def __repr__(self) -> str: - return f"{self.__class__.__name__}({list(self.dependencies)})" + return f"{self.__class__.__name__}({list(self)})" # NB: This name is clunky. It would be more appropriate to call it `Set`, but that is claimed by diff --git a/src/rust/engine/src/externs/interface.rs b/src/rust/engine/src/externs/interface.rs index 16fc33d65a6..b1c1fea1d66 100644 --- a/src/rust/engine/src/externs/interface.rs +++ b/src/rust/engine/src/externs/interface.rs @@ -1339,7 +1339,7 @@ fn capture_snapshots( session_ptr: PySession, path_globs_and_root_tuple_wrapper: PyObject, ) -> CPyResult { - let values = externs::project_multi(&path_globs_and_root_tuple_wrapper.into(), "dependencies"); + let values = externs::project_iterable(&path_globs_and_root_tuple_wrapper.into()); let path_globs_and_roots = values .iter() .map(|value| { @@ -1398,7 +1398,7 @@ fn merge_directories( session_ptr: PySession, directories_value: PyObject, ) -> CPyResult { - let digests = externs::project_multi(&directories_value.into(), "dependencies") + let digests = externs::project_iterable(&directories_value.into()) .iter() .map(|v| nodes::lift_digest(v)) .collect::, _>>() diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index 359e4f41a38..8a80a27c54a 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -164,7 +164,7 @@ pub fn store_bool(val: bool) -> Value { /// /// Gets an attribute of the given value as the given type. /// -pub fn getattr(value: &Value, field: &str) -> Result +fn getattr(value: &Value, field: &str) -> Result where for<'a> T: FromPyObject<'a>, { @@ -184,6 +184,34 @@ where }) } +/// +/// Collect the Values contained within an outer Python Iterable Value. +/// +fn collect_iterable(value: &Value) -> Result, String> { + let gil = Python::acquire_gil(); + let py = gil.python(); + match value.iter(py) { + Ok(py_iter) => py_iter + .enumerate() + .map(|(i, py_res)| { + py_res.map(Value::from).map_err(|py_err| { + format!( + "Could not iterate {}, failed to extract {}th item: {:?}", + val_to_str(value), + i, + py_err + ) + }) + }) + .collect(), + Err(py_err) => Err(format!( + "Could not iterate {}: {:?}", + val_to_str(value), + py_err + )), + } +} + /// /// Pulls out the value specified by the field name from a given Value /// @@ -191,6 +219,10 @@ pub fn project_ignoring_type(value: &Value, field: &str) -> Value { getattr(value, field).unwrap() } +pub fn project_iterable(value: &Value) -> Vec { + collect_iterable(value).unwrap() +} + pub fn project_multi(value: &Value, field: &str) -> Vec { getattr(value, field).unwrap() } diff --git a/src/rust/engine/src/intrinsics.rs b/src/rust/engine/src/intrinsics.rs index e52248eab04..d5e4214eba5 100644 --- a/src/rust/engine/src/intrinsics.rs +++ b/src/rust/engine/src/intrinsics.rs @@ -314,7 +314,7 @@ fn create_digest_to_digest( context: Context, args: Vec, ) -> BoxFuture<'static, NodeResult> { - let file_values = externs::project_multi(&args[0], "dependencies"); + let file_values = externs::project_iterable(&args[0]); let digests: Vec<_> = file_values .iter() .map(|file| { From 2e04a7bf965f1ef8d15d93643d729f1f05be9805 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sun, 26 Jul 2020 11:57:09 -0600 Subject: [PATCH 2/2] Fix equality under Python3.6. Test failures revealed isinstance for types derived from tuple behaves unexpectedly under Python3.6. Although this can be worked around by saving off the mro in __init_subclass__ and using that to perform a custom isinstance check, more investigation indicated two aspects of the status quo were undesirable: 1. By returning NotImplemented we left equality up to foreign types to decide. 2. The intended use of Collection subtypes is for the engine, which treats all types as distinct; yet we would compare subtypes as equal. As such implement exact type match based equality and document this. --- src/python/pants/engine/collection.py | 6 +++--- src/python/pants/engine/collection_test.py | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/python/pants/engine/collection.py b/src/python/pants/engine/collection.py index f20ae3915c0..b9bd9b35c2e 100644 --- a/src/python/pants/engine/collection.py +++ b/src/python/pants/engine/collection.py @@ -19,6 +19,8 @@ class Example: class Examples(Collection[Example]): pass + + N.B: Collection instances are only considered equal if both their types and contents are equal. """ @overload # noqa: F811 @@ -36,9 +38,7 @@ def __getitem__(self, index: Union[int, slice]) -> Union[T, "Collection[T]"]: # return self.__class__(cast(Tuple[T, ...], result)) def __eq__(self, other: Any) -> bool: - if not isinstance(other, self.__class__): - return NotImplemented - return super().__eq__(other) + return type(self) == type(other) and super().__eq__(other) def __ne__(self, other: Any) -> bool: # We must explicitly override to provide the inverse of _our_ __eq__ and not get the diff --git a/src/python/pants/engine/collection_test.py b/src/python/pants/engine/collection_test.py index 9f6d5a0f12f..40d4ad3661a 100644 --- a/src/python/pants/engine/collection_test.py +++ b/src/python/pants/engine/collection_test.py @@ -46,6 +46,9 @@ def test_collection_reversed() -> None: def test_collection_equality() -> None: + assert () != Collection() + assert Collection() != () + assert Collection([]) == Collection([]) c1 = Collection([1, 2, 3]) assert c1 == Collection([1, 2, 3])