fix: type Markable is reduced to Any by mypy

[terencehonles]

The following code:

pytest/src/_pytest/mark/structures.py

Line 269 in 364bbe4

Markable = TypeVar("Markable", bound=Union[Callable[..., object], type])

when using reveal_type with mypy resolves to Any which is not what we want, but I believe the type variable can probably be simplified to the following and still be correct:

Markable = TypeVar("Markable", bound=Callable)

The previous definition defined the return of the callable and did accept any class. I don't believe we care about the return of the callable and classes are also callable so this should be sufficient and still correct.

[terencehonles]

Sorry, I probably should have just opened this as an issue. This might be an issue with the overload for @parameterize that I'm seeing.

def markable(arg: Callable[[Dict[str, str]], Path]) -> None:                                                                 
    pass

Markable = TypeVar("Markable", bound=Union[Callable[..., object], type])

def test(m: Markable) -> Markable:                                            
    return m                                                               

reveal_type(test(markable))  #    def markable(arg: Callable[[Dict[str, str]], Path]):

[terencehonles]

It looks like it is probably the ignored code:

pytest/src/_pytest/mark/structures.py

Lines 347 to 352 in 364bbe4

	# Type ignored because the overloads overlap with an incompatible
	# return type. Not much we can do about that. Thankfully mypy picks
	# the first match so it works out even if we break the rules.
	@overload
	def __call__(self, arg: Markable) -> Markable: # type: ignore[misc]
	pass

I was able to add the following to the end of this PR and check that it resolved the types correctly:

if TYPE_CHECKING:
    from typing import Dict

    @MARK_GEN.parametrize('test', [1])
    def testing(x: Dict[str, int]) -> List[int]:
        return list(x.values())
    reveal_type(testing)

src/_pytest/mark/structures.py:589: note: Revealed type is 'def (x: builtins.dict[builtins.str, builtins.int]) -> builtins.list[builtins.int]'

[terencehonles]

Well I'm still having issues and the original type definition doesn't look like it has issues, and for me it looks like it's related to using a named tuple and reported that to mypy python/mypy#10470, but I believe it's probably because of this python/typing#253 (comment)

This PR, if accepted, is simplifying the TypeVar and removing the need for the # type: ignore comment

[nicoddemus]

Thanks @terencehonles!

Leaving this to our in-house typing expert @bluetech. 😁

[bluetech]

Hi @terencehonles,

Trying to follow the issue here, I'm not quite sure I got it. Can you maybe add a "test case" to the file tests/typing_checks.py? This is just a file which mypy checks. A "test case" in this file is simply a piece of code that would fail to type-check before the change, and type-check correctly after. It is not executed.

IIUC, your change makes Markable just a Callable which allows it to unify with the other overload and thus somehow make mypy work, however I think that ignore[misc] is actually correct and if it doesn't trigger then something is off...

[bluetech]

Markable is an unbound TypeVar here, which usually doesn't make sense. Since the typing in covered by the overloads I chose to omit the return type here.

[terencehonles]

Is it unbound? I figured mypy would use the overload, but I can adjust this so there is less overlap. I can force non keyword arguments but without knowing at least one required keyword argument I don't know how to tell mypy that at least one has to be provided. to force the MarkDecorator path.

[terencehonles]

Just to follow up on the last part of your comment... As you have it mypy (possibly any type checker) will read this as -> Any. I had thought as you are on a different codebase and I noticed that mypy wasn't checking the function at all (I had left off the argument signature too), I then added the argument signature and tested that I could return anything even outside the bounds of the @overloads. This might have been fixed in mypy, but it's best to be explicit about your return type. When I had added the return annotation it required casting the return in the Markable path since it was now checking the return was not Any.

[bluetech]

When a function has overloads defined, the typing of the implementation function is only used for the implementation -- it is ignored in favor of the overloads everywhere else.

[terencehonles]

Yes, that's correct, but shouldn't the function return be type checked so that future editors don't accidentally make a mistake?

[bluetech]

I think best we can do is add a comment.

[terencehonles]

Thanks for the review @bluetech, I'm responding to your overall comment, but replies to the line specific comments are inline.

Trying to follow the issue here, I'm not quite sure I got it. Can you maybe add a "test case" to the file tests/typing_checks.py? This is just a file which mypy checks. A "test case" in this file is simply a piece of code that would fail to type-check before the change, and type-check correctly after. It is not executed.

Thanks for pointing me to this file, it was useful to see the other issues that had come up and as mentioned inline I think we should probably be changing any use of object to Any as mentioned in this comment #8674 (comment)

As far as adding a test case, I understand the PR had a few comments that probably made it harder to follow, but it looks like the bug I was seeing is in mypy as mentioned in this comment #8674 (comment) and not in pytest. Putting the example code at the bottom of the file without my changes:

if TYPE_CHECKING:
    from typing import Dict

    @MARK_GEN.parametrize('test', [1])
    def testing(x: Dict[str, int]) -> List[int]:
        return list(x.values())
    reveal_type(testing)

It properly revealed the correct types. So this change has been reduced to not fixing a bug in pytest, but simplifying the type variables and removing the mypy error.

IIUC, your change makes Markable just a Callable which allows it to unify with the other overload and thus somehow make mypy work, however I think that ignore[misc] is actually correct and if it doesn't trigger then something is off...

Yes, I think this is mostly correct, but I'm not sure I agree about the last part 😅, looking for possible documentation I believe the difference is mentioned here https://mypy.readthedocs.io/en/stable/more_types.html#type-checking-the-variants

So in this example, the int argument in the first variant is a subtype of the object argument in the second, yet the int return type is not a subtype of str. Both conditions are true, so mypy will correctly flag unsafe_func as being unsafe.

[bluetech]

@terencehonles So what's left from me is:

• I'd still like to see the last unresolved comment fixed (removing the return type from the implemention def)
• If you could add a regression test (doesn't type check before, type checks after) to the tests/typing_checks.py file that would also be great.

[bluetech]

I'll close this now since I'm not sure it's an improvement, but feel free to reopen if you come back to it.