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.

Sign up for GitHub

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

[red-knot] fix non-callable reporting for unions #16387

Merged
merged 2 commits into from
Feb 26, 2025
Merged

[red-knot] fix non-callable reporting for unions #16387

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa1b0ac
[red-knot] fix non-callable reporting for unions
carljm Feb 26, 2025
9178297
prefer CallError::Union for mixed-errors case
carljm Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/call/union.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ def _(flag: bool, flag2: bool):
else:
def f() -> int:
return 1
# TODO we should mention all non-callable elements of the union
# error: [call-non-callable] "Object of type `Literal[1]` is not callable"
# revealed: int | Unknown
reveal_type(f())
Expand Down Expand Up @@ -108,3 +109,20 @@ def _(flag: bool):
x = f(3)
reveal_type(x) # revealed: Unknown
```

## Union of binding errors

```py
def f1(): ...
def f2(): ...
def _(flag: bool):
if flag:
f = f1
else:
f = f2

# TODO: we should show all errors from the union, not arbitrarily pick one union element
# error: [too-many-positional-arguments] "Too many positional arguments to function `f1`: expected 0, got 1"
x = f(3)
reveal_type(x) # revealed: Unknown
```
2 changes: 1 addition & 1 deletion crates/red_knot_python_semantic/src/types/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ impl<'db> CallOutcome<'db> {
let elements = union.elements(db);
let mut bindings = Vec::with_capacity(elements.len());
let mut errors = Vec::new();
let mut not_callable = true;
let mut not_callable = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now wrong but in different ways then before?

If I understand this correctly, it will now return not_callable if one variant is not callable and the other has binding errors. What I think we want is to only return not_callable if all errors are not_callable.

So the fix might be to keep initializing the variable to true but change the error case to not_callable &= error.is_not_callable()

Copy link
Contributor Author

@carljm carljm Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that behavior would be correct. A union with at least one not-callable element is not callable.

In general for any type it is always true that some inhabitants of the type may permit some behavior that the overall type does not permit. The definition of a type supporting an operation is that all inhabitants support it.

I would like us to offer more detailed diagnostics for not-callable unions, where we would clarify which member(s) are not callable. In that world, I do think we might want to return CallError::Union in more cases. But with the behavior today, where CallError::Union only reports one of its errors, I think it would be wrong if we ignored a not-callable error from one element and just reported a binding error from some other type in the union. So I think we need to return NotCallable if any element is not callable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this will lead to worse diagnostics overall because we then end up saying that the type isn't callable instead of saying which variant isn't callable.

I can't argue on what this means on the type inference level but your original implementation only returned NotCallable if all variants weren't callable, unless I'm misreading the implementation:

ruff/crates/red_knot_python_semantic/src/types/call.rs

Lines 240 to 282 in 792f9e3

let mut not_callable = vec![];
let mut union_builder = UnionBuilder::new(context.db());
let mut revealed = false;
for outcome in outcomes {
let return_ty = match outcome {
Self::NotCallable { not_callable_ty } => {
not_callable.push(*not_callable_ty);
Type::unknown()
}
Self::RevealType {
binding,
revealed_ty: _,
} => {
if revealed {
binding.return_type()
} else {
revealed = true;
outcome.unwrap_with_diagnostic(context, node)
}
}
_ => outcome.unwrap_with_diagnostic(context, node),
};
union_builder = union_builder.add(return_ty);
}
let return_ty = union_builder.build();
match not_callable[..] {
[] => Ok(return_ty),
[elem] => Err(NotCallableError::UnionElement {
not_callable_ty: elem,
called_ty: *called_ty,
return_ty,
}),
_ if not_callable.len() == outcomes.len() => Err(NotCallableError::Type {
not_callable_ty: *called_ty,
return_ty,
}),
_ => Err(NotCallableError::UnionElements {
not_callable_tys: not_callable.into_boxed_slice(),
called_ty: *called_ty,
return_ty,
}),
}
}

Copy link
Contributor Author

@carljm carljm Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your original implementation only returned NotCallable if all variants weren't callable, unless I'm misreading the implementation

Both branches in the linked original implementation return NotCallableError, whether some or all elements are not callable. The only difference is whether we return a "simple" NotCallableError (because if all elements are not callable, we don't really need to give more details) or a "union" NotCallableError (because we want to clarify which elements are not callable.)

we then end up saying that the type isn't callable instead of saying which variant isn't callable.

I agree. The core problem here is the current handling of CallError::Union only reporting one of its errors. This means that in a case where we have one binding error and one not-callable in a union, might decide to just report the binding error and ignore the not-callable error. That's the wrong result I aimed to avoid with this change. In that case I would much rather see a not-callable error on the entire union, than a wrong-arguments error for the one callable element.

We can go ahead and return CallError::Union for that case. It will just increase the priority of better diagnostics for CallError::Union.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer keeping returning Union because it already forces the right handling in all upstream code.

Fixing the diagnostic is an issue on its own where we may want to be more clever about how we handle different errors (I don't think rendering all errors is a good idea but we could sort the errors and e.g. report not callable first)


for element in elements {
match call(*element) {
Expand Down
Loading