[ty] Match variadic argument to variadic parameter by dhruvmanila · Pull Request #20511 · astral-sh/ruff (original) (raw)

@dhruvmanila

Summary

Closes: astral-sh/ty#1236

This PR fixes a bug where the variadic argument wouldn't match against the variadic parameter in certain scenarios.

This was happening because I didn't realize that the all_elements iterator wouldn't keep on returning the variable element (which is correct, I just didn't realize it back then).

I don't think we can use the resize method here because we don't know how many parameters this variadic argument is matching against as this is where the actual parameter matching occurs.

Test Plan

Expand test cases to consider a few more combinations of arguments and parameters which are variadic.

@dhruvmanila dhruvmanila added bug

Something isn't working

ty

Multi-file analysis & type inference

labels

Sep 22, 2025

@github-actions

@github-actions

mypy_primer results

Changes were detected when running on open source projects

ignite (https://github.com/pytorch/ignite)

sphinx (https://github.com/sphinx-doc/sphinx)

meson (https://github.com/mesonbuild/meson)

vision (https://github.com/pytorch/vision)

schemathesis (https://github.com/schemathesis/schemathesis)

scikit-build-core (https://github.com/scikit-build/scikit-build-core)

pandas (https://github.com/pandas-dev/pandas)

bokeh (https://github.com/bokeh/bokeh)

ibis (https://github.com/ibis-project/ibis)

core (https://github.com/home-assistant/core)

No memory usage changes detected ✅

AlexWaygood

Choose a reason for hiding this comment

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

Nice! I think this approach still has some false negatives on "mixed tuples" with fixed-length suffixes. E.g. here's the current behaviour on this branch:

def func(*args: str): ...

def _( a: tuple[str, ...], b: tuple[str, *tuple[str, ...]], c: tuple[str, *tuple[str, ...], str], d: tuple[int, *tuple[str, ...]], e: tuple[int, *tuple[str, ...], str], f: tuple[*tuple[str, ...], str], g: tuple[*tuple[str, ...], int], h: tuple[int, *tuple[str, ...], int], ): func(*a) # no error (good) func(*b) # no error (good) func(*c) # no error (good) func(*d) # error (good) func(*d) # error (good) func(*e) # error (good) func(*f) # no error (good) func(*g) # <------------- no error (bad!) func(*h) # error (good)

@dhruvmanila

I think the core issue here is that we've a way to structure a one-to-many mapping like *args argument to multiple parameters via the MatchedArgument but we don't have a way to structure a many-to-one mapping which will occur in this scenario where a variadic argument is being unpacked into a variadic parameter. I'm thinking if it's actually required or not, and if so how to structure it.

@dhruvmanila

dhruvmanila

Comment on lines 2495 to 2505

argument: Argument<'a>,
argument_type: Type<'db>,
) {
// If the argument is splatted, convert its type into a tuple describing the splatted
// elements. For tuples, we don't have to do anything! For other types, we treat it as
// an iterator, and create a homogeneous tuple of its output type, since we don't know
// how many elements the iterator will produce.
let argument_types = argument_type.iterate(self.db);
// Resize the tuple of argument types to line up with the number of parameters this
// argument was matched against. If parameter matching succeeded, then we can (TODO:
// should be able to, see above) guarantee that all of the required elements of the
// splatted tuple will have been matched with a parameter. But if parameter matching
// failed, there might be more required elements. That means we can't use
// TupleLength::Fixed below, because we would otherwise get a "too many values" error
// when parameter matching failed.
let desired_size =
TupleLength::Variable(self.argument_matches[argument_index].parameters.len(), 0);
let argument_types = argument_types
.resize(self.db, desired_size)
.expect("argument type should be consistent with its arity");
// Check the types by zipping through the splatted argument types and their matched
// parameters.
for (argument_type, parameter_index) in
(argument_types.all_elements()).zip(&self.argument_matches[argument_index].parameters)
for (parameter_index, variadic_argument_type) in
self.argument_matches[argument_index].iter()
{
self.check_argument_type(
adjusted_argument_index,
argument,
*argument_type,
*parameter_index,
variadic_argument_type.unwrap_or_else(Type::unknown),
parameter_index,
);

Choose a reason for hiding this comment

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

The reason this isn't required now is because the unpacking of a variadic argument is being done during parameter matching itself. This was added in #20130 to make sure that specialization builder has access to the correct type for each parameter.

The MatchedArgument already stores the unpacked types for a variadic argument so we can directly use that here instead.

Comment on lines 2139 to 2145

fn match_variadic(
&mut self,
db: &'db dyn Db,
argument_index: usize,
argument: Argument<'a>,
argument_type: Option<Type<'db>>,
length: TupleLength,
) -> Result<(), ()> {

Choose a reason for hiding this comment

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

The length isn't required here because we already have access to Tuple<Type> in this function so we can use that directly to get the TupleLength. This also simplifies the overload evaluation where we needed to change this length when argument type expansion lead to retrying of parameter matching.

Choose a reason for hiding this comment

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

That's a great catch!

Comment on lines +2186 to 2197

// Finally, if there is a variadic parameter we can match any of the remaining unpacked
// argument types to it, but only if there is at least one remaining argument type. This is
// because a variadic parameter is optional, so if this was done unconditionally, ty could
// raise a false positive as "too many arguments".
if self.parameters.variadic().is_some() {
if let Some(argument_type) = argument_types.next().or(variable_element) {
self.match_positional(argument_index, argument, Some(argument_type))?;
for argument_type in argument_types {
self.match_positional(argument_index, argument, Some(argument_type))?;
}
}
}

Choose a reason for hiding this comment

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

This is the core logic that would actually match the variadic argument with the variadic parameter in cases where it's missed today. Refer to mdtest examples.

@dhruvmanila

@dhruvmanila

Ecosystem analysis

All of the new diagnostics in ignite are, I think, correct. The annotation of the variadic parameter for register_events function is *event_names: Union[List[str], List[EventEnum]] which suggests that each element of an unpacking should be a list but the provided parameter is unpacking the enum class directly.

from enum import Enum

class Foo(Enum): foo = "foo" bar = "bar"

def f(*args: list[Foo]): ...

This should raise a diagnostic because it unpacks into Foo variants but the

annotated parameter type expects each element to be a list[Foo]

f(*Foo)

The diagnostic in vision is correct as well:

import itertools import string

def _(*digits: str): if not digits: digits = string.ascii_lowercase else: # ty: Object of type str is not assignable to tuple[str, ...] [invalid-assignment] digits = "".join(itertools.chain(*digits))

The diagnostic in bokeh seems to be related to generics which, if I remember correctly, was brought up during an early discussion regarding how to consider the entire union to map the concrete type so that it doesn't map it to T and instead considers T | list[T] (the details are a bit hazy in my mind right now).

def f[T](*args: T | list[T]) -> list[T]: return []

class Foo: ...

def _(*args: Foo | list[Foo]): # This PR reveals this to be list[Foo | list[Foo]] and in bokeh it then creates a # diagnostic when it's being used where list[Foo] is expected. reveal_type(f(*args))

@dhruvmanila

(This is something that I'd love to get @dcreager review on specifically as the original author for single-starred argument.)

AlexWaygood

Choose a reason for hiding this comment

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

This looks reasonable to me, thanks! I agree it would be great to get @dcreager's eyes on it, though

dcreager

Choose a reason for hiding this comment

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

This approach looks good! We still need to know the type of the splatted argument before we can perform parameter matching; otherwise we don't know how many elements are in the argument and therefore how many parameters it will be matched to. I think that's unavoidable, though. (And @ibraheemdev, I think this the main thing that might wreak havoc on your work to use bidirectional type checking with function arguments.)

Some(tuple) => (
Either::Left(tuple.all_elements().copied()),
tuple.len(),
tuple.variable_element().copied(),

Choose a reason for hiding this comment

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

It looks like you only ever use the variable element to extend the all_elements iterator with a never-ending stream of variable_element items. You could do the same thing by chaining a std::iter::repeat to the end of the all_elements iterator. Then you wouldn't need the .or(variable_element) calls below. (You might also consider adding that as an additional method on Tuple or TupleSpec)

Choose a reason for hiding this comment

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

That's a good idea although there's one place where I don't use the .or(...) call at the end which just loops over the remaining argument types if any. This is usually the case for suffix elements.

Comment on lines 2139 to 2145

fn match_variadic(
&mut self,
db: &'db dyn Db,
argument_index: usize,
argument: Argument<'a>,
argument_type: Option<Type<'db>>,
length: TupleLength,
) -> Result<(), ()> {

Choose a reason for hiding this comment

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

That's a great catch!

@dhruvmanila

This approach looks good! We still need to know the type of the splatted argument before we can perform parameter matching; otherwise we don't know how many elements are in the argument and therefore how many parameters it will be matched to. I think that's unavoidable, though.

Yeah, I agree that this is unavoidable.

@dhruvmanila

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})