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

[carljm]

Minor follow-up to #16161

This not_callable flag wasn't functional, because it could never be false. It was initialized to true and then only ever updated with |=, which can never make it false.

Add a test that exercises the case where it should be false (all of the union elements are callable) but bindings is also empty (all union elements have binding errors). Before this PR, the added test wrongly emits a diagnostic that the union Literal[f1] | Literal[f2] is not callable.

And add a test where a union call results in one binding error and one not-callable error, where we currently give the wrong result (we show only the binding error), with a TODO.

Also add TODO comments in a couple other tests where ideally we'd report more than just one error out of a union call.

Also update the flag name to all_errors_not_callable to more clearly indicate the semantics of the flag.

[dhruvmanila]

Nice catch!

[MichaReiser]

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()

[carljm]

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.

[MichaReiser]

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,
	}),
	}
	}

[carljm]

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.

[MichaReiser]

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)

[AlexWaygood]

I'm not sure whether it counts as an alternative solution or a workaround, but did you see what I did with this in my Type::try_iterate() PR the other day? I added this method:

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

Lines 175 to 195 in dd6f623

	impl UnionCallError<'_> {
	/// Return `true` if this `UnionCallError` indicates that the union might not be callable at all.
	/// Otherwise, return `false`.
	///
	/// For example, the union type `Callable[[int], int] | None` may not be callable at all,
	/// because the `None` element in this union has no `__call__` method. Calling an object that
	/// inhabited this union type would lead to a `UnionCallError` that would indicate that the
	/// union might not be callable at all.
	///
	/// On the other hand, the union type `Callable[[int], int] | Callable[[str], str]` is always
	/// *callable*, but it would still lead to a `UnionCallError` if an inhabitant of this type was
	/// called with a single `int` argument passed in. That's because the second element in the
	/// union doesn't accept an `int` when it's called: it only accepts a `str`.
	pub(crate) fn indicates_type_possibly_not_callable(&self) -> bool {
	self.errors.iter().any(|error| match error {
	CallError::BindingError { .. } => false,
	CallError::NotCallable { .. } | CallError::PossiblyUnboundDunderCall { .. } => true,
	CallError::Union(union_error) => union_error.indicates_type_possibly_not_callable(),
	})
	}
	}

And then emitted different diagnostics for the CallError::Union() branch depending on whether it returned true or false:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 3032 to 3071 in dd6f623

	Self::IterCallError(dunder_iter_call_error) => match dunder_iter_call_error {
	CallError::NotCallable { not_callable_type } => report_not_iterable(format_args!(
	"Object of type `{iterable_type}` is not iterable \
	because its `__iter__` attribute has type `{dunder_iter_type}`, \
	which is not callable",
	iterable_type = iterable_type.display(db),
	dunder_iter_type = not_callable_type.display(db),
	)),
	CallError::PossiblyUnboundDunderCall { called_type, .. } => {
	report_not_iterable(format_args!(
	"Object of type `{iterable_type}` may not be iterable \
	because its `__iter__` attribute (with type `{dunder_iter_type}`) \
	may not be callable",
	iterable_type = iterable_type.display(db),
	dunder_iter_type = called_type.display(db),
	));
	}
	CallError::Union(union_call_error) if union_call_error.indicates_type_possibly_not_callable() => {
	report_not_iterable(format_args!(
	"Object of type `{iterable_type}` may not be iterable \
	because its `__iter__` attribute (with type `{dunder_iter_type}`) \
	may not be callable",
	iterable_type = iterable_type.display(db),
	dunder_iter_type = union_call_error.called_type.display(db),
	));
	}
	CallError::BindingError { .. } => report_not_iterable(format_args!(
	"Object of type `{iterable_type}` is not iterable \
	because its `__iter__` method has an invalid signature \
	(expected `def __iter__(self): ...`)",
	iterable_type = iterable_type.display(db),
	)),
	CallError::Union(UnionCallError { called_type, .. }) => report_not_iterable(format_args!(
	"Object of type `{iterable_type}` may not be iterable \
	because its `__iter__` method (with type `{dunder_iter_type}`) \
	may have an invalid signature (expected `def __iter__(self): ...`)",
	iterable_type = iterable_type.display(db),
	dunder_iter_type = called_type.display(db),
	)),
	}

It's finicky, but it seems to work pretty well

[carljm]

@AlexWaygood Yes, we could do that here as well. But I think it's also sub-optimal that given multiple binding errors from a union, we only report one of them. So I think I'd rather just wait for a more full-fledged diagnostic system that allows sub-diagnostics, and then update the CallError::Union handling more comprehensively.

[MichaReiser]

Thank you