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

Type mismatches in await Gets give unclear error message #8349

Closed
illicitonion opened this issue Sep 27, 2019 · 4 comments · Fixed by #11081
Closed

Type mismatches in await Gets give unclear error message #8349

illicitonion opened this issue Sep 27, 2019 · 4 comments · Fixed by #11081
Assignees

Comments

@illicitonion
Copy link
Contributor

illicitonion commented Sep 27, 2019

If you do something like:
snapshot = yield Get(Snapshot, PathGlobs, "foo")
where there is a type mismatch between the declared parameter type, and the actual parameter type, you get an error:

Exception: WithDeps(Inner(InnerEntry { params: {Console, OptionsBootstrapper, Specs}, rule: Task(Task { product: List, clause: [Select { product: Console }, Select { product: _Options }, Select { product: Specs }], gets: [Get { product: Snapshot, subject: PathGlobs }, Get { product: HydratedTargets, subject: Specs }, Get { product: BuildFileAddresses, subject: Specs }], func: list_targets(), cacheable: false }) })) did not declare a dependency on JustGet(Get { product: Snapshot, subject: str })

Talking about "did not declare a dependency" is really misleading; this error message should read more like:

"Did not have a set of rules installed to transform from a str to a Snapshot, which you tried to do in file X on line Y"

and maybe even a hint along the lines of "You declared that the argument to be of type PathGlobs but it was actually of type str".

@stuhood
Copy link
Member

stuhood commented Sep 27, 2019

Slightly grouchy comment, but: #7502 (comment) ... relates to #7509.

@stuhood
Copy link
Member

stuhood commented Sep 27, 2019

Oh! There is a much, much easier way to fix 90% of cases of this one though: the constructor for Get should verify that the types match for the three arg form, here:

elif len(args) == 3:
product, subject_declared_type, subject = args

@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Feb 27, 2020

Oh! There is a much, much easier way to fix 90% of cases of this one though: the constructor for Get should verify that the types match for the three arg form, here:

I tried doing this a few weeks ago. It fails because of union types :/ I couldn't come up with a clean fix. Inspecting union_membership was clunky iirc (and maybe not even possible? I don't recall).

--

@TansyArron and I came up with this error message:

Exception: await Get[Digest](InputFilesContent) expected InputFilesContent but got str instead. See the @rule construct_coverage_config() from pants.backend.python.rules.pytest_coverage line 100.

Rust already has all the information needed to render this, including line numbers. We only need to implement it now.

@Eric-Arellano
Copy link
Contributor

@herman5 said he's interested in taking this on 🎉

To formalize the problem a little more, the error happens when you do:

await Get[A](B, z)

where z is not an instance of the class B. A is the "product". B is the "subject" type, and z is the "subject" instance.

--

The goal is to say something like this:

Exception: await Get[Digest](InputFilesContent) expected InputFilesContent but got str instead. See the @rule construct_coverage_config() from pants.backend.python.rules.pytest_coverage line 100.

Please feel free to improve this!

--

You'll need to change this:

fn gen_get(
context: &Context,
params: &Params,
entry: &Arc<rule_graph::Entry<Rule>>,
gets: Vec<externs::Get>,
) -> NodeFuture<Vec<Value>> {
let get_futures = gets
.into_iter()
.map(|get| {
let context = context.clone();
let mut params = params.clone();
let entry = entry.clone();
let dependency_key = selectors::DependencyKey::JustGet(selectors::Get {
product: get.product,
subject: *get.subject.type_id(),
});
let entry = context
.core
.rule_graph
.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.declared_subject {
Some(ty) if externs::is_union(ty) => {
let value = externs::get_value_from_type_id(ty);
match externs::call_method(
&value,
"non_member_error_message",
&[externs::val_for(&get.subject)],
) {
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.subject.type_id(),
ty
)),
}
}
_ => throw(&format!(
"{:?} did not declare a dependency on {:?}",
entry, dependency_key
)),
})
});

Line 809 is what you'll want to change. You can get the declared product type (A) and the actual bad subject type (z) from lines 778-779:

product: get.product,
subject: *get.subject.type_id(),

I'm not totally sure how to get the declared subject type (B)? It's definitely possible - if you look at the original error message posted by Daniel, we have access to all of the rule's gets. Possibly, you need to iterate through the rule's gets to filter by the product type, then use that to find the declared subject type?

I think entry should have the rule information you need, specifically the rule name, the line number, and the module name. This is where we generate the representation of the rule function:

impl Function {
pub fn name(&self) -> String {
let Function(key) = self;
let module = externs::project_str(&externs::val_for(&key), "__module__");
let name = externs::project_str(&externs::val_for(&key), "__name__");
// 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 = externs::project_str(&externs::val_for(&key), "__line_number__");
format!("{}:{}:{}", module, line_number, name)
}
}
impl fmt::Display for Function {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}()", self.name())
}
}

This generates a value like pants_test.engine.rules:84:a_from_b_noop(). Feel free to stick with that representation of a rule, as opposed to something like construct_coverage_config() from pants.backend.python.rules.pytest_coverage line 100.. Otherwise, you'd need to generalize that above code so that you can request the distinct parts like module, lineno, and name. Maybe that's a good change and how we should have done it from the start?

You might also find this https://github.com/pantsbuild/pants/blob/master/src/rust/engine/src/tasks.rs helpful to see how we generally represent a rule in graph error messages. See the type FormattedTaskRuleElements and impl fmt::Display for Rule. Generally, this has more information than we want for this particular error message, but this could help see how to work with the type.

--

Finally, you'll want to make sure that ./v2 test --debug tests/python/pants_test/engine/test_scheduler.py --pytest-args='-k test_get_type_match_failure' passes.

@Eric-Arellano Eric-Arellano changed the title Type mismatches in yield Gets give unclear error message Type mismatches in await Gets give unclear error message May 18, 2020
@Eric-Arellano Eric-Arellano self-assigned this Oct 29, 2020
Eric-Arellano added a commit that referenced this issue Oct 30, 2020
Before:

> Exception: WithDeps(Inner(InnerEntry { params: {Specs, Console}, rule: Task(Task { product: List, can_modify_workunit: false, clause: [Addresses, ListSubsystem, Console], gets: [Get { output: UnexpandedTargets, input: Addresses }], func: pants.backend.project_info.list_targets:64:list_targets(), cacheable: false, display_info: DisplayInfo { name: "pants.backend.project_info.list_targets.list_targets", desc: Some("`list` goal"), level: Debug } }) })) did not declare a dependency on JustGet(Get { output: UnexpandedTargets, input: int })

After:

```
Engine traceback:
  in select
  in pants.backend.project_info.list_targets.list_targets
Traceback (most recent call last):
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/native.py", line 65, in generator_send
    res = func.send(arg)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/backend/project_info/list_targets.py", line 79, in list_targets
    targets = await Get(UnexpandedTargets, Addresses, 123)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/util/meta.py", line 182, in new_init
    prev_init(self, *args, **kwargs)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/selectors.py", line 159, in __init__
    self.input = self._validate_input(input_arg1, shorthand_form=False)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/selectors.py", line 201, in _validate_input
    f"Invalid Get. The third argument `{input_}` must have the exact same type as the "
TypeError: Invalid Get. The third argument `123` must have the exact same type as the second argument, pants.engine.addresses.Addresses, but had the type <class 'int'>.
```

Note that the stacktrace includes the offending `Get`, including the line and file where it's set. This is because we now eagerly validate in the Python constructor for `Get`.

We can only apply this new error message for non-union `Get` calls; the engine must handle type errors relating to `unions` because Python does not have knowledge of `UnionMembership` in the `Get` constructor.

Closes #8349.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants