Fallback to checking kwargs of function call when looking for arguments of a call

[md384]

I have made things!

Related issues

Closes #2043

[sobolevn]

We have several usages of this function: https://github.com/search?q=repo%3Atypeddjango%2Fdjango-stubs%20get_call_argument_by_name&type=code

Can you please also test the QuerySet one?

[sobolevn]

stupid question: what will happen if we do kwargs.pop('flat'); super().values_list(*args, flat=True, **kwargs)?

[md384]

The flat=True doesn't change the return type of the get call in the tests. This seems like a separate bug and is beyond my knowledge of mypy or mypy plugins -there doesn't seem to be anything usable in the context.

[flaeppe]

I'm suspecting there's more cases to cover for here.

[flaeppe]

I don't think usage of ctx.arg_names[1] will work like this here.

For example if I instead declare my field like:

class SecondField(fields.IntegerField[_ST, _GT]):
    def __init__(self, *args: Any, blank: bool = True, **kwargs: Any) -> None:
        super().__init__(*args, blank=blank, **kwargs)

And call it with: SecondField(blank=True, null=True)

ctx.arg_names could look like:

[[], ['blank'], ['null']]

Which results in a mismatch, resulting in the issue we're trying to solve.

With the above in mind. If I instead then do:

class ThirdField(fields.IntegerField[_ST, _GT]):
    def __init__(self) -> None:
        super().__init__()

And call it like: ThirdField() I get an IndexError crash on ctx.arg_names[1]

[md384]

I think we can do something along the lines of

    # check for named arg in function call keyword args
    for idx, arg_group in enumerate(ctx.arg_names):
        if name in arg_group:
            sub_idx = arg_group.index(name)
            return ctx.args[idx][sub_idx]

to handle these cases.

[flaeppe]

Did you see my comment: #2043 (comment)?

[md384]

Did you see my comment: #2043 (comment)?

Yes, but that means the developer has to know about these "magic" kwargs in a number of functions.

IMO, the plugin should be able to handle the *args, **kwargs case since it is a common pattern and the presence/absence of some kwargs unexpectedly (at least for the typical django-stubs user) changes the return type.

[flaeppe]

I disagree with that the plugin should try to resolve *args: Any and **kwargs: Any here. In general the plugin should help express things that can't be statically declared, but this isn't such a case. There's a way to statically declare this and, in my eyes, introducing plugin code here just increases the risk of interference.

I can agree with that it might be tedious to declare any or all overridden arguments, especially since there are quite a few..

But we want the plugin to do as little as possible and fallback on mypy to do the rest.

At least that's my opinion. @intgr or @sobolevn might have different thoughts here.

[intgr]

I'm torn on this.

From a typing perspective, *args: Any, **kwargs: Any is problematic, since it suppresses all typechecking of the call. You can write MyIntegerField(1, 2, 3, null=Ellipsis, no_such_arg=123) and mypy won't bat an eyelid. From this perspective it seems consistent to also reject inspecting null= if the function definition is vague.

On the other hand, this PR does seem like an improvement in making django-stubs less finnicky. The implementation makes sense and is self-contained in the helper function; other callers of this function may benefit from the changes as well.

If we can't think of any downsides of this approach, seems unfair to block it. So a weak vote for merging from me.