From cc373c8e09498c7acfb3343c62307572af2908a4 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 29 Oct 2020 13:23:58 -0700 Subject: [PATCH 1/5] Remove unnecessary `Option` Not sure why we had this in the first place # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/rust/engine/src/externs/mod.rs | 4 +-- src/rust/engine/src/nodes.rs | 44 ++++++++++++++---------------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs index 1f7e3937436..4e46a46b257 100644 --- a/src/rust/engine/src/externs/mod.rs +++ b/src/rust/engine/src/externs/mod.rs @@ -452,7 +452,7 @@ py_class!(pub class PyGeneratorResponseGetMulti |py| { pub struct Get { pub output: TypeId, pub input: Key, - pub input_type: Option, + pub input_type: TypeId, } impl Get { @@ -462,7 +462,7 @@ impl Get { input: interns .key_insert(py, get.subject(py).clone_ref(py).into()) .map_err(|e| Failure::from_py_err_with_gil(py, e))?, - input_type: Some(interns.type_insert(py, get.declared_subject(py).clone_ref(py))), + input_type: interns.type_insert(py, get.declared_subject(py).clone_ref(py)), }) } } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 7c02e96b115..48eb87e0da5 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -904,32 +904,30 @@ impl Task { .edges_for_inner(&entry) .ok_or_else(|| throw(&format!("no edges for task {:?} exist!", entry))) .and_then(|edges| { - edges - .entry_for(&dependency_key) - .cloned() - .ok_or_else(|| match get.input_type { - Some(ty) if externs::is_union(ty) => { - let value = externs::type_for_type_id(ty).into_object(); - match externs::call_method( - &value, - "non_member_error_message", - &[externs::val_for(&get.input)], - ) { - Ok(err_msg) => throw(&externs::val_to_str(&err_msg)), - // If the non_member_error_message() call failed for any reason, - // fall back to a generic message. - Err(_e) => throw(&format!( - "Type {} is not a member of the {} @union", - get.input.type_id(), - ty - )), - } + edges.entry_for(&dependency_key).cloned().ok_or_else(|| { + if externs::is_union(get.input_type) { + let value = externs::type_for_type_id(get.input_type).into_object(); + match externs::call_method( + &value, + "non_member_error_message", + &[externs::val_for(&get.input)], + ) { + Ok(err_msg) => throw(&externs::val_to_str(&err_msg)), + // If the non_member_error_message() call failed for any reason, + // fall back to a generic message. + Err(_e) => throw(&format!( + "Type {} is not a member of the {} @union", + get.input.type_id(), + get.input_type + )), } - _ => throw(&format!( + } else { + throw(&format!( "{:?} did not declare a dependency on {:?}", entry, dependency_key - )), - }) + )) + } + }) }); // The subject of the get is a new parameter that replaces an existing param of the same // type. From 4aece800ae9c1fc2bcbb26c716ab1f6fc100a090 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 29 Oct 2020 15:08:21 -0700 Subject: [PATCH 2/5] Improve error message for invalid input in `Get()`s # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/engine/internals/scheduler_test.py | 10 +++--- src/rust/engine/src/core.rs | 32 +++++++++++++------ src/rust/engine/src/nodes.rs | 23 +++++++++++-- src/rust/engine/src/tasks.rs | 2 +- 4 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/python/pants/engine/internals/scheduler_test.py b/src/python/pants/engine/internals/scheduler_test.py index 6a0e462a254..3c882a1ee54 100644 --- a/src/python/pants/engine/internals/scheduler_test.py +++ b/src/python/pants/engine/internals/scheduler_test.py @@ -261,15 +261,13 @@ def rules(cls): ) def test_get_type_match_failure(self): - """Test that Get(...)s are now type-checked during rule execution, to allow for union - types.""" - with self.assertRaises(ExecutionError) as cm: # `a_typecheck_fail_test` above expects `wrapper.inner` to be a `B`. self.request(A, [TypeCheckFailWrapper(A())]) - - expected_regex = "WithDeps.*did not declare a dependency on JustGet" - self.assertRegex(str(cm.exception), expected_regex) + assert ( + "Invalid input to `Get(A, B)`. Expected an object with type `B`, but got `A()` with " + "type `A` instead." + ) in str(cm.exception) def test_unhashable_root_params_failure(self): """Test that unhashable root params result in a structured error.""" diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index 6d46f3c1fd6..87911565611 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -148,33 +148,47 @@ impl fmt::Display for TypeId { } } -// An identifier for a python function. +/// An identifier for a Python function. #[repr(C)] #[derive(Clone, Copy, Eq, Hash, PartialEq)] pub struct Function(pub Key); impl Function { + /// A Python function's module, e.g. `project.app`. + pub fn module(&self) -> String { + let val = externs::val_for(&self.0); + externs::getattr_as_string(&val, "__module__") + } + + /// A Python function's name, without its module. pub fn name(&self) -> String { - let Function(key) = self; - let val = externs::val_for(&key); - let module = externs::getattr_as_string(&val, "__module__"); - let name = externs::getattr_as_string(&val, "__name__"); + let val = externs::val_for(&self.0); + externs::getattr_as_string(&val, "__name__") + } + + /// The line number of a Python function's first line. + pub fn line_number(&self) -> u64 { + let val = externs::val_for(&self.0); // NB: this is a custom dunder method that Python code should populate before sending the // function (e.g. an `@rule`) through FFI. - let line_number: u64 = externs::getattr(&val, "__line_number__").unwrap(); - format!("{}:{}:{}", module, line_number, name) + externs::getattr(&val, "__line_number__").unwrap() + } + + /// The function represented as `path.to.module:lineno:func_name`. + pub fn full_name(&self) -> String { + format!("{}:{}:{}", self.module(), self.line_number(), self.name()) } } impl fmt::Display for Function { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}()", self.name()) + write!(f, "{}()", self.full_name()) } } impl fmt::Debug for Function { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}()", self.name()) + write!(f, "{}()", self.full_name()) } } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 48eb87e0da5..f9f219e4322 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -902,7 +902,7 @@ impl Task { .core .rule_graph .edges_for_inner(&entry) - .ok_or_else(|| throw(&format!("no edges for task {:?} exist!", entry))) + .ok_or_else(|| throw(&format!("No edges for task {:?} exist!", entry))) .and_then(|edges| { edges.entry_for(&dependency_key).cloned().ok_or_else(|| { if externs::is_union(get.input_type) { @@ -922,9 +922,26 @@ impl Task { )), } } else { + let parent_rule_func = match &*entry { + rule_graph::Entry::WithDeps(entry_with_deps) => match entry_with_deps.rule() { + Some(ref rule) => match rule { + tasks::Rule::Task(ref task) => Some(task.func), + _ => None, + }, + None => None, + }, + _ => None + }; + let rule_context = match parent_rule_func { + Some(func) => format!( + " See the rule {}() in {} (line {}).", + func.name(), func.module(), func.line_number() + ), + _ => "".to_string() + }; throw(&format!( - "{:?} did not declare a dependency on {:?}", - entry, dependency_key + "Invalid input to `Get({}, {})`. Expected an object with type `{}`, but got `{:?}` with type `{}` instead.{}", + get.output, get.input_type, get.input_type, get.input, get.input.type_id(), rule_context )) } }) diff --git a/src/rust/engine/src/tasks.rs b/src/rust/engine/src/tasks.rs index 48e78440588..1f34c09a5bc 100644 --- a/src/rust/engine/src/tasks.rs +++ b/src/rust/engine/src/tasks.rs @@ -23,7 +23,7 @@ impl DisplayForGraph for Rule { fn fmt_for_graph(&self, display_args: DisplayForGraphArgs) -> String { match self { Rule::Task(ref task) => { - let task_name = task.func.name(); + let task_name = task.func.full_name(); let product = format!("{}", task.product); let clause_portion = Self::formatted_select_clause(&task.clause, display_args); From 71deaa873b623c962a0bc903cc95597d90a72b69 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 29 Oct 2020 21:10:15 -0700 Subject: [PATCH 3/5] Move the error message to Python # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/engine/internals/scheduler_test.py | 27 ------ .../pants/engine/internals/selectors.py | 57 ++++++++---- .../pants/engine/internals/selectors_test.py | 91 +++++++++++++------ src/rust/engine/src/nodes.rs | 23 +---- 4 files changed, 106 insertions(+), 92 deletions(-) diff --git a/src/python/pants/engine/internals/scheduler_test.py b/src/python/pants/engine/internals/scheduler_test.py index 3c882a1ee54..220de96b089 100644 --- a/src/python/pants/engine/internals/scheduler_test.py +++ b/src/python/pants/engine/internals/scheduler_test.py @@ -122,22 +122,6 @@ async def error_msg_test_rule(union_wrapper: UnionWrapper) -> UnionX: raise AssertionError("The statement above this one should have failed!") -class TypeCheckFailWrapper: - """This object wraps another object which will be used to demonstrate a type check failure when - the engine processes an `await Get(...)` statement.""" - - def __init__(self, inner): - self.inner = inner - - -@rule -async def a_typecheck_fail_test(wrapper: TypeCheckFailWrapper) -> A: - # This `await` would use the `nested_raise` rule, but it won't get to the point of raising since - # the type check will fail at the Get. - _ = await Get(A, B, wrapper.inner) # noqa: F841 - return A() - - @dataclass(frozen=True) class CollectionType: # NB: We pass an unhashable type when we want this to fail at the root, and a hashable type @@ -252,23 +236,12 @@ class SchedulerWithNestedRaiseTest(TestBase): def rules(cls): return ( *super().rules(), - a_typecheck_fail_test, c_unhashable, nested_raise, - QueryRule(A, (TypeCheckFailWrapper,)), QueryRule(A, (B,)), QueryRule(C, (CollectionType,)), ) - def test_get_type_match_failure(self): - with self.assertRaises(ExecutionError) as cm: - # `a_typecheck_fail_test` above expects `wrapper.inner` to be a `B`. - self.request(A, [TypeCheckFailWrapper(A())]) - assert ( - "Invalid input to `Get(A, B)`. Expected an object with type `B`, but got `A()` with " - "type `A` instead." - ) in str(cm.exception) - def test_unhashable_root_params_failure(self): """Test that unhashable root params result in a structured error.""" # This will fail at the rust boundary, before even entering the engine. diff --git a/src/python/pants/engine/internals/selectors.py b/src/python/pants/engine/internals/selectors.py index 185f98e3228..4231f97029a 100644 --- a/src/python/pants/engine/internals/selectors.py +++ b/src/python/pants/engine/internals/selectors.py @@ -21,6 +21,7 @@ overload, ) +from pants.engine.unions import union from pants.util.meta import frozen_after_init _Output = TypeVar("_Output") @@ -149,33 +150,57 @@ def __init__( input_arg0: Union[Type[_Input], _Input], input_arg1: Optional[_Input] = None, ) -> None: - self.output_type = output_type - self.input_type = self._validate_input_type( - input_arg0 if input_arg1 is not None else type(input_arg0) - ) - self.input = self._validate_input(input_arg1 if input_arg1 is not None else input_arg0) - - self._validate_output_type() + self.output_type = self._validate_output_type(output_type) + if input_arg1 is None: + self.input_type = type(input_arg0) + self.input = self._validate_input(input_arg0, shorthand_form=True) + else: + self.input_type = self._validate_explicit_input_type(input_arg0) + self.input = self._validate_input(input_arg1, shorthand_form=False) - def _validate_output_type(self) -> None: - if not isinstance(self.output_type, type): + @staticmethod + def _validate_output_type(output_type: Any) -> Type[_Output]: + if not isinstance(output_type, type): raise TypeError( - f"The output type must be a type, but given {self.output_type} of type " - f"{type(self.output_type)}." + "Invalid Get. The first argument (the output type) must be a type, but given " + f"`{output_type}` with type {type(output_type)}." ) + return cast(Type[_Output], output_type) @staticmethod - def _validate_input_type(input_type: Any) -> Type[_Input]: + def _validate_explicit_input_type(input_type: Any) -> Type[_Input]: if not isinstance(input_type, type): raise TypeError( - f"The input type must be a type, but given {input_type} of type {type(input_type)}." + "Invalid Get. Because you are using the longhand form Get(OutputType, InputType, " + f"input), the second argument must be a type, but given `{input_type}` of type " + f"{type(input_type)}." ) return cast(Type[_Input], input_type) - @staticmethod - def _validate_input(input_: Any) -> _Input: + def _validate_input(self, input_: Any, *, shorthand_form: bool) -> _Input: if isinstance(input_, type): - raise TypeError(f"The input argument cannot be a type, but given {input_}.") + if shorthand_form: + raise TypeError( + "Invalid Get. Because you are using the shorthand form " + "Get(OutputType, InputType(constructor args)), the second argument should be " + f"a constructor call, rather than a type, but given {input_}." + ) + else: + raise TypeError( + "Invalid Get. Because you are using the longhand form " + "Get(OutputType, InputType, input), the third argument should be " + f"an object, rather than a type, but given {input_}." + ) + # If the input_type is not annotated with `@union`, then we validate that the input is + # exactly the same type as the input_type. (Why not check unions? We don't have access to + # `UnionMembership` to know if it's a valid union member. The engine will check that.) + if not union.is_instance(self.input_type) and type(input_) != self.input_type: + # We can assume we're using the longhand form because the shorthand form guarantees + # that the `input_type` is the same as `input`. + raise TypeError( + f"Invalid Get. The third argument `{input_}` must have the exact same type as the " + f"second argument, {self.input_type}, but had the type {type(input_)}." + ) return cast(_Input, input_) def __await__( diff --git a/src/python/pants/engine/internals/selectors_test.py b/src/python/pants/engine/internals/selectors_test.py index 3a283b0a7da..2a1d5e3ecae 100644 --- a/src/python/pants/engine/internals/selectors_test.py +++ b/src/python/pants/engine/internals/selectors_test.py @@ -3,11 +3,12 @@ import ast import re -from typing import Any, Tuple +from typing import Any, Callable, Tuple import pytest from pants.engine.internals.selectors import Get, GetConstraints, GetParseError, MultiGet +from pants.engine.unions import union def parse_get_types(get: str) -> Tuple[str, str]: @@ -83,47 +84,77 @@ def __eq__(self, other: Any): def test_create_get() -> None: - get = Get(AClass, BClass, 42) + get = Get(AClass, int, 42) assert get.output_type is AClass - assert get.input_type is BClass + assert get.input_type is int assert get.input == 42 - -def test_create_get_abbreviated() -> None: - # Test the equivalence of the 1-arg and 2-arg versions. + # Also test the equivalence of the 1-arg and 2-arg versions. assert Get(AClass, BClass()) == Get(AClass, BClass, BClass()) -def test_invalid_get_abbreviated() -> None: - with pytest.raises( - expected_exception=TypeError, - match=re.escape(f"The input argument cannot be a type, but given {BClass}."), - ): - Get(AClass, BClass) +def assert_invalid_get(create_get: Callable[[], Get], *, expected: str) -> None: + with pytest.raises(TypeError) as exc: + create_get() + assert str(exc.value) == expected -def test_invalid_get_input() -> None: - with pytest.raises( - expected_exception=TypeError, - match=re.escape(f"The input argument cannot be a type, but given {BClass}."), - ): - Get(AClass, BClass, BClass) +def test_invalid_get() -> None: + # Bad output type. + assert_invalid_get( + lambda: Get(1, str, "bob"), # type: ignore[call-overload, no-any-return] + expected=( + "Invalid Get. The first argument (the output type) must be a type, but given " + f"`1` with type {int}." + ), + ) + # Bad second argument. + assert_invalid_get( + lambda: Get(AClass, BClass), + expected=( + "Invalid Get. Because you are using the shorthand form " + "Get(OutputType, InputType(constructor args)), the second argument should be " + f"a constructor call, rather than a type, but given {BClass}." + ), + ) + assert_invalid_get( + lambda: Get(AClass, 1, BClass), # type: ignore[call-overload, no-any-return] + expected=( + "Invalid Get. Because you are using the longhand form Get(OutputType, InputType, " + "input), the second argument must be a type, but given `1` of type " + f"{int}." + ), + ) -def test_invalid_get_input_type() -> None: - with pytest.raises( - expected_exception=TypeError, - match=re.escape(f"The input type must be a type, but given {1} of type {type(1)}."), - ): - Get(AClass, 1, BClass) # type: ignore[call-overload] + # Bad third argument. + assert_invalid_get( + lambda: Get(AClass, BClass, BClass), + expected=( + "Invalid Get. Because you are using the longhand form Get(OutputType, InputType, " + "input), the third argument should be an object, rather than a type, but given " + f"{BClass}." + ), + ) -def test_invalid_get_output_type() -> None: - with pytest.raises( - expected_exception=TypeError, - match=re.escape(f"The output type must be a type, but given {1} of type {type(1)}."), - ): - Get(1, "bob") # type: ignore[call-overload] +def test_invalid_get_input_does_not_match_type() -> None: + assert_invalid_get( + lambda: Get(AClass, str, 1), + expected=( + f"Invalid Get. The third argument `1` must have the exact same type as the " + f"second argument, {str}, but had the type {int}." + ), + ) + + # However, if the `input_type` is a `@union`, then we do not eagerly validate. + @union + class UnionBase: + pass + + union_get = Get(AClass, UnionBase, 1) + assert union_get.input_type == UnionBase + assert union_get.input == 1 def test_multiget_invalid_types() -> None: diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index f9f219e4322..cf7878003c5 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -922,26 +922,11 @@ impl Task { )), } } else { - let parent_rule_func = match &*entry { - rule_graph::Entry::WithDeps(entry_with_deps) => match entry_with_deps.rule() { - Some(ref rule) => match rule { - tasks::Rule::Task(ref task) => Some(task.func), - _ => None, - }, - None => None, - }, - _ => None - }; - let rule_context = match parent_rule_func { - Some(func) => format!( - " See the rule {}() in {} (line {}).", - func.name(), func.module(), func.line_number() - ), - _ => "".to_string() - }; + // NB: The Python constructor for `Get()` will have already errored if + // `type(input) != input_type`. throw(&format!( - "Invalid input to `Get({}, {})`. Expected an object with type `{}`, but got `{:?}` with type `{}` instead.{}", - get.output, get.input_type, get.input_type, get.input, get.input.type_id(), rule_context + "Could not find a rule to satisfy Get({}, {}, {}).", + get.output, get.input_type, get.input )) } }) From 23abd07a0dbeb683f6907765d0aeb9386c9fb896 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 30 Oct 2020 10:06:10 -0700 Subject: [PATCH 4/5] Fix scheduler_test.py And rewrite part of it to use RuleRunner and to have better proximity. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/engine/internals/scheduler_test.py | 161 +++++++++--------- 1 file changed, 81 insertions(+), 80 deletions(-) diff --git a/src/python/pants/engine/internals/scheduler_test.py b/src/python/pants/engine/internals/scheduler_test.py index 220de96b089..43ee3b5b478 100644 --- a/src/python/pants/engine/internals/scheduler_test.py +++ b/src/python/pants/engine/internals/scheduler_test.py @@ -7,14 +7,13 @@ from textwrap import dedent from typing import Any -from pants.engine.internals.engine_testutil import ( - assert_equal_with_printing, - remove_locations_from_traceback, -) +import pytest + +from pants.engine.internals.engine_testutil import remove_locations_from_traceback from pants.engine.internals.scheduler import ExecutionError from pants.engine.rules import Get, rule from pants.engine.unions import UnionRule, union -from pants.testutil.rule_runner import QueryRule +from pants.testutil.rule_runner import QueryRule, RuleRunner from pants.testutil.test_base import TestBase @@ -28,16 +27,6 @@ class B: pass -def fn_raises(x): - raise Exception(f"An exception for {type(x).__name__}") - - -@rule(desc="Nested raise") -def nested_raise(b: B) -> A: - fn_raises(b) - return A() - - @rule def consumes_a_and_b(a: A, b: B) -> str: return str(f"{a} and {b}") @@ -122,21 +111,6 @@ async def error_msg_test_rule(union_wrapper: UnionWrapper) -> UnionX: raise AssertionError("The statement above this one should have failed!") -@dataclass(frozen=True) -class CollectionType: - # NB: We pass an unhashable type when we want this to fail at the root, and a hashable type - # when we'd like it to succeed. - items: Any - - -@rule -async def c_unhashable(_: CollectionType) -> C: - # This `await` would use the `nested_raise` rule, but it won't get to the point of raising since - # the hashability check will fail. - _result = await Get(A, B, list()) # noqa: F841 - return C() - - @rule def boolean_and_int(i: int, b: bool) -> A: return A() @@ -231,54 +205,81 @@ def test_union_rules_no_docstring(self): self.request(UnionX, [UnionWrapper(UnionA())]) -class SchedulerWithNestedRaiseTest(TestBase): - @classmethod - def rules(cls): - return ( - *super().rules(), - c_unhashable, - nested_raise, - QueryRule(A, (B,)), - QueryRule(C, (CollectionType,)), - ) +# ----------------------------------------------------------------------------------------------- +# Test tracebacks. +# ----------------------------------------------------------------------------------------------- - def test_unhashable_root_params_failure(self): - """Test that unhashable root params result in a structured error.""" - # This will fail at the rust boundary, before even entering the engine. - with self.assertRaisesRegex(TypeError, "unhashable type: 'list'"): - self.request(C, [CollectionType([1, 2, 3])]) - - def test_unhashable_get_params_failure(self): - """Test that unhashable Get(...) params result in a structured error.""" - # This will fail inside of `c_unhashable_dataclass`. - with self.assertRaisesRegex(ExecutionError, "unhashable type: 'list'"): - self.request(C, [CollectionType(tuple())]) - - def test_trace_includes_rule_exception_traceback(self): - # Execute a request that will trigger the nested raise, and then directly inspect its trace. - request = self.scheduler.execution_request([A], [B()]) - _, throws = self.scheduler.execute(request) - - with self.assertRaises(ExecutionError) as cm: - self.scheduler._raise_on_error([t for _, t in throws]) - - trace = remove_locations_from_traceback(str(cm.exception)) - assert_equal_with_printing( - self, - dedent( - f"""\ - 1 Exception encountered: - - Engine traceback: - in select - in {self.__module__}.{nested_raise.__name__} - Traceback (most recent call last): - File LOCATION-INFO, in nested_raise - fn_raises(b) - File LOCATION-INFO, in fn_raises - raise Exception(f"An exception for {{type(x).__name__}}") - Exception: An exception for B - """ - ), - trace, - ) + +def fn_raises(): + raise Exception("An exception!") + + +@rule(desc="Nested raise") +def nested_raise() -> A: + fn_raises() + return A() + + +def test_trace_includes_rule_exception_traceback() -> None: + rule_runner = RuleRunner(rules=[nested_raise, QueryRule(A, [])]) + with pytest.raises(ExecutionError) as exc: + rule_runner.request(A, []) + normalized_traceback = remove_locations_from_traceback(str(exc.value)) + assert normalized_traceback == dedent( + f"""\ + 1 Exception encountered: + + Engine traceback: + in select + in {__name__}.{nested_raise.__name__} + Traceback (most recent call last): + File LOCATION-INFO, in nested_raise + fn_raises() + File LOCATION-INFO, in fn_raises + raise Exception(f"An exception!") + Exception: An exception! + """ + ) + + +# ----------------------------------------------------------------------------------------------- +# Test unhashable types. +# ----------------------------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class MaybeHashableWrapper: + maybe_hashable: Any + + +@rule +async def unhashable(_: MaybeHashableWrapper) -> B: + return B() + + +@rule +async def call_unhashable_with_invalid_input() -> C: + _ = await Get(B, MaybeHashableWrapper([1, 2])) + return C() + + +def test_unhashable_types_failure() -> None: + rule_runner = RuleRunner( + rules=[ + unhashable, + call_unhashable_with_invalid_input, + QueryRule(B, [MaybeHashableWrapper]), + QueryRule(C, []), + ] + ) + + # Succeed if an argument to a rule is hashable. + assert rule_runner.request(B, [MaybeHashableWrapper((1, 2))]) == B() + # But fail if an argument to a rule is unhashable. This is a TypeError because it fails while + # hashing as part of FFI. + with pytest.raises(TypeError, match="unhashable type: 'list'"): + rule_runner.request(B, [MaybeHashableWrapper([1, 2])]) + + # Fail if the `input` in a `Get` is not hashable. + with pytest.raises(ExecutionError, match="unhashable type: 'list'"): + rule_runner.request(C, []) From 7447d81baf618e667606ae3dd8f98c081d794b74 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 30 Oct 2020 13:01:02 -0700 Subject: [PATCH 5/5] Fix test string match failing # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/engine/internals/scheduler_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/python/pants/engine/internals/scheduler_test.py b/src/python/pants/engine/internals/scheduler_test.py index 43ee3b5b478..28b0a2fc3e5 100644 --- a/src/python/pants/engine/internals/scheduler_test.py +++ b/src/python/pants/engine/internals/scheduler_test.py @@ -236,7 +236,7 @@ def test_trace_includes_rule_exception_traceback() -> None: File LOCATION-INFO, in nested_raise fn_raises() File LOCATION-INFO, in fn_raises - raise Exception(f"An exception!") + raise Exception("An exception!") Exception: An exception! """ )