Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make union error messages better #7999

Merged
merged 7 commits into from Jul 9, 2019

Conversation

@gshuflin
Copy link
Contributor

commented Jul 2, 2019

cf. #7819 , we should be printing out a more helpful error message in
the case that a Get request is unsatisfiable because of a type
mismatch involving union membership.

This commit defines two new FFI functions: one for getting a handle to
the underlying Python class representing a type, given a TypeId; and
another for checking whether or not a type is a union (by inspecting
the '_is_union' attribute). It also modifies the Get struct to track
the optional declared subject of a Get request.

Using these, if a Get request fails in nodes.rs, we look at that
declared subject (if it exists), and inspect the Optional type contained
within in order to see if it is a union. If so, we print a clearer error
message mentioning the name and description of the union. If not, we
fall back to the existing error message.

In order to correctly print the description of the union, we also needed
to alter the @union decorator to explicitly create an attr on
@union-decorated types called 'union_description', and search for this
attr with project_str in the new union error message handling code.

Make union error messages better
cf. #7819 , we should be printing out a more helpful error message in
the case that a `Get` request is unsatisfiable because of a type
mismatch involving union membership.

This commit defines two new FFI functions: one for getting a handle to
the underlying Python class representing a type, given a TypeId; and
another for checking whether or not a type is a union (by inspecting
the '_is_union' attribute). It also modifies the `Get` struct to track
the optional declared subject of a `Get` request.

Using these, if a Get request fails in `nodes.rs`, we look at that
declared subject (if it exists), and inspect the Optional type contained
within in order to see if it is a union. If so, we print a clearer error
message mentioning the name and description of the union. If not, we
fall back to the existing error message.

In order to correctly print the description of the union, we also needed
to alter the @union decorator to explicitly create an attr on
@union-decorated types called 'union_description', and search for this
attr with `project_str` in the new union error message handling code.
@jsirois
Copy link
Member

left a comment

The test failure in target ./pants test tests/python/pants_test/engine:scheduler -- -k test_union_rules is legit. It's a fairly brittle existing test that greps stack traces for a message that is now different with your work here.

))
let is_union = get.declared_subject.map(externs::is_union).unwrap_or(false);
if is_union {
let union_ty = get.declared_subject.unwrap();

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 3, 2019

Member

This is logically safe, but reading an unwrap() here is jarring, requiring a second look to make sure this can never error. What do you think about using an early return instead?:

diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs
index 145ed93af..424b23b11 100644
--- a/src/rust/engine/src/nodes.rs
+++ b/src/rust/engine/src/nodes.rs
@@ -864,21 +864,20 @@ impl Task {
           .ok_or_else(|| throw(&format!("no edges for task {:?} exist!", entry)))
           .and_then(|edges| {
             edges.entry_for(&dependency_key).cloned().ok_or_else(|| {
-              let is_union = get.declared_subject.map(externs::is_union).unwrap_or(false);
-              if is_union {
-                let union_ty = get.declared_subject.unwrap();
-                let value = externs::get_value_from_type_id(union_ty);
-                let description = externs::project_str(&value, "union_description");
-                throw(&format!(
-                  "Type {} is not a member of the {} @union (\"{}\")",
-                  get.subject, union_ty, description
-                ))
-              } else {
-                throw(&format!(
-                  "{:?} did not declare a dependency on {:?}",
-                  entry, dependency_key
-                ))
+              if let Some(subject_type) = get.declared_subject {
+                if externs::is_union(subject_type) {
+                  let value = externs::get_value_from_type_id(subject_type);
+                  let description = externs::project_str(&value, "union_description");
+                  return throw(&format!(
+                    "Type {} is not a member of the {} @union (\"{}\")",
+                    get.subject, subject_type, description
+                  ))
+                }
               }
+              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

This would be really clean with the let_chains feature and a clear win with no need to nest ifs and early return, but maybe preferrable anyhow.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jul 3, 2019

Author Contributor

I ended up refactoring this into a guarded match expression, which avoids both the unwrap() call and the early return

@@ -302,6 +302,7 @@ def get_some_union_type(x):
assert isinstance(cls, type)
return type(cls.__name__, (cls,), {
'_is_union': True,
'union_description': cls.__doc__,

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 3, 2019

Member

How about a fallback to the cls.__name__ if cls.__doc__ is empty? The current erroring test shows off a fairly unhelpful case of an empty doc string.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jul 3, 2019

Author Contributor

Good idea, added

gshuflin added some commits Jul 3, 2019

Print TypeId only in union error message
Instead of printing the Get subject (a Key, which has extraneous
debug information), just print the name of the inner TypeId. This
makes the error message and tests for the error message cleaner.
Union description fallback without docstring
If there is no docstring for a union class, set the
union_description filed to its __name__ instead
Avoid unwrap() call in union error message flow
Use a guarded match expression to avoid having to unwrap a
value we know but can't statically prove is Some

@gshuflin gshuflin force-pushed the gshuflin:union-description branch from a54c8f2 to c71412d Jul 3, 2019

@jsirois

jsirois approved these changes Jul 4, 2019

Copy link
Member

left a comment

This looks good to me. It would be good for at least one of @stuhood, @Eric-Arellano or @cosmicexplorer to chime in since they had so much input here.

return type(cls.__name__, (cls,), {
'_is_union': True,
'union_description': cls.__doc__,
'union_description': union_description,

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 4, 2019

Member

Either cls.__doc__ or cls.__name__ here or union_description = cls.__doc__ or cls.__name__ above is probably more idiomatic, but this, of course, is just fine.

This comment has been minimized.

Copy link
@Eric-Arellano
.entry_for(&dependency_key)
.cloned()
.ok_or_else(|| match get.declared_subject {
Some(ty) if externs::is_union(ty) => {

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 4, 2019

Member

Much nicer!

@@ -206,7 +208,7 @@ def test_union_rules(self):
self.assertTrue(isinstance(a, A))
# Fails due to no union relationship from A -> UnionBase.
expected_msg = """\
Exception: WithDeps(Inner(InnerEntry { params: {UnionWrapper}, rule: Task(Task { product: A, clause: [Select { product: UnionWrapper }], gets: [Get { product: A, subject: UnionA }, Get { product: A, subject: UnionB }], func: a_union_test(), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: A, subject: A })
Type A is not a member of the UnionBase @union ("Docstring for UnionBase")

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 4, 2019

Member

Its probably a good idea to test the fallback for no docstring.

@stuhood

stuhood approved these changes Jul 4, 2019

Copy link
Member

left a comment

Thanks!


@_extern_decl('bool', ['ExternContext*', 'TypeId'])
def extern_is_union(self, context_handle, type_id):
"Return whether or not a type is a member of a union"

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 4, 2019

Member

Convention is to use a triple quoted string for comments.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 4, 2019

Contributor

Yes, in fact that is what populates the __doc__ dunder method. If you use triple quotes immediately after the def line, then it becomes docstring.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jul 8, 2019

Author Contributor

Looks like Python 3 doesn't care syntactically whether a docstring is triple-quoted or not, but the docs say that one should use triple-quotes for consistency. I'll change it to triple quotes, and maybe this requirement should be part of the linter?

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 8, 2019

Member

It is, but :(:

pants/pants.ini

Lines 284 to 303 in 96c152a

[pycheck-class-factoring]
skip: True
[pycheck-pycodestyle]
skip: True
[pycheck-import-order]
skip: True
[pycheck-variable-names]
skip: True
[pycheck-trailing-whitespace]
skip: True
[pycheck-context-manager]
skip: True
[pycheck-newstyle-classes]
skip: True

@Eric-Arellano
Copy link
Contributor

left a comment

Cool! That's such a better error message. Thanks for diving into this.


@_extern_decl('bool', ['ExternContext*', 'TypeId'])
def extern_is_union(self, context_handle, type_id):
"Return whether or not a type is a member of a union"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 4, 2019

Contributor

Yes, in fact that is what populates the __doc__ dunder method. If you use triple quotes immediately after the def line, then it becomes docstring.

@_extern_decl('bool', ['ExternContext*', 'TypeId'])
def extern_is_union(self, context_handle, type_id):
"Return whether or not a type is a member of a union"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 4, 2019

Contributor

Nit: usually don't want this whitespace after the docstring, unless it's a really big function where you're using whitespace to differentiate sections.

return type(cls.__name__, (cls,), {
'_is_union': True,
'union_description': cls.__doc__,
'union_description': union_description,

This comment has been minimized.

Copy link
@Eric-Arellano
@jsirois

jsirois approved these changes Jul 8, 2019

@@ -151,11 +166,14 @@ def rules(cls):
# B is both a RootRule and an intermediate product here.
RootRule(B),
RootRule(C),
RootRule(UnionX),

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 8, 2019

Member

I'm pretty sure this registration is not needed since the test does not construct a (root) Param of UnionX.

@@ -291,8 +291,7 @@ def extern_get_handle_from_type_id(self, context_handle, ty):

@_extern_decl('bool', ['ExternContext*', 'TypeId'])
def extern_is_union(self, context_handle, type_id):
"Return whether or not a type is a member of a union"

"""Return whether or not a type is a member of a union"""

This comment has been minimized.

Copy link
@jsirois

jsirois Jul 8, 2019

Member

Too small to burn extra CI for but we tend to end all doc sentences with period.

@stuhood

stuhood approved these changes Jul 9, 2019

Copy link
Member

left a comment

Thanks again!

@stuhood stuhood merged commit a218a78 into pantsbuild:master Jul 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

illicitonion added a commit that referenced this pull request Jul 10, 2019

Make union error messages better (#7999)
Fixes #7819 , we should be printing out a more helpful error message in
the case that a `Get` request is unsatisfiable because of a type
mismatch involving union membership.

This commit defines two new FFI functions: one for getting a handle to
the underlying Python class representing a type, given a TypeId; and
another for checking whether or not a type is a union (by inspecting
the '_is_union' attribute). It also modifies the `Get` struct to track
the optional declared subject of a `Get` request.

Using these, if a Get request fails in `nodes.rs`, we look at that
declared subject (if it exists), and inspect the Optional type contained
within in order to see if it is a union. If so, we print a clearer error
message mentioning the name and description of the union. If not, we
fall back to the existing error message.

In order to correctly print the description of the union, we also needed
to alter the @union decorator to explicitly create an attr on
@union-decorated types called 'union_description', and search for this
attr with `project_str` in the new union error message handling code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.