use ParamSpec for pytest.skip instead of __call__ Protocol attribute

[karlicoss]

This is a more canonical way of typing generic callbacks/decorators (see https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols)

This helps with potential issues/ambiguity with __call__ semanics in type checkers -- according to python spec, __call__ needs to be present on the class, rather than as an instance attribute to be considered callable.

This worked in mypy because it didn't handle the spec 100% correctly (in this case this has no negative consequences)

However, ty type checker is stricter/more correct about it and every pytest.skip usage results in:

error[call-non-callable]: Object of type _WithException[Unknown, <class 'Skipped'>] is not callable

For more context, see:

• https://github.com/astral-sh/ruff/pull/17832#issuecomment-2855425767
• https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938

Testing:

Tested with running mypy against the following snippet:

import pytest
reveal_type(pytest.skip)
reveal_type(pytest.skip.Exception)
reveal_type(pytest.skip(reason="whatever"))

Before the change:

test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[def (reason: builtins.str =, *, allow_module_level: builtins.bool =) -> Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]"
test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped"
test_pytest_skip.py:4: note: Revealed type is "Never"

After the change:

test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._WithException[[reason: builtins.str =, *, allow_module_level: builtins.bool =], Never, def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped]"
test_pytest_skip.py:3: note: Revealed type is "def (msg: Union[builtins.str, None] =, pytrace: builtins.bool =, allow_module_level: builtins.bool =, *, _use_item_location: builtins.bool =) -> _pytest.outcomes.Skipped"
test_pytest_skip.py:4: note: Revealed type is "Never"

All types are matching and propagated correctly.

[bluetech]

Thanks! This looks good to me. But looking at this again, I wonder if it wouldn't be simpler (in terms of complexity, not in terms of lines of code) to turn these functions into callable classes? Then the Exception is a regular (class) attribute and there should be no typing shenanigans necessary, unless I'm missing something. WDYT?

[bluetech]

Forgot to mention, the CI failures are unrelated and fixed in main -- rebase should take care of it.

[karlicoss]

Hi @bluetech -- sorry, just got around to try this, did on a separate branch (just on skip to start with) https://github.com/pytest-dev/pytest/commit/56b3937ea3e1317022d47b41a00b55c246448af2

Something like this (If I understood your suggestion correctly)

+class _Skip:
+    Exception = Skipped
+
+    def __call__(self, reason: str = "", allow_module_level: bool = False) -> NoReturn:
+        __tracebackhide__ = True
+        raise Skipped(msg=reason, allow_module_level=allow_module_level)
+
+skip = _Skip()

This generally works, and seems to result in correct runtime/typecheck time types. However it fails this test: https://github.com/pytest-dev/pytest/blob/9e9633de9da7a9fab03b4bba3a326bf85b412050/testing/python/collect.py#L1075-L1081

$ tox -e py39-xdist
    def test_skip_simple(self):
        with pytest.raises(pytest.skip.Exception) as excinfo:
            pytest.skip("xxx")
>       assert excinfo.traceback[-1].frame.code.name == "skip"
E       AssertionError: assert '__call__' == 'skip'
E
E         - skip
E         + __call__

testing/python/collect.py:1078: AssertionError

Which kinda makes sense -- python would normally print just in __call__ in the stacktrace, not even in <Classname>.__call__.

Apart from that, the remaining asserts pass if I change the assert to assert excinfo.traceback[-1].frame.code.name == "__call__". Not sure what's the intent of assert -- if it's just meant to check that skip was called, or we actually want the user to see skip in the trace.

So up to you if you'd prefer me to convert the rest (exit/fail/xfail), move over docstrings and fix the tests; or you'd rather not touch it!

[karlicoss]

Hi @bluetech -- any thoughts on this? :)

[bluetech]

I rebased your current code and added a changelog to make things green in case we want to go with that.

Something like this (If I understood your suggestion correctly)

Yes.

This generally works, and seems to result in correct runtime/typecheck time types. However it fails this test:

The purpose of the test is to test that the frame is hidden, so it's OK that the name is changed. The test can be fixed like this:

-        assert excinfo.traceback[-1].frame.code.name == "skip"
+        if sys.version_info >= (3, 11):
+            assert excinfo.traceback[-1].frame.code.raw.co_qualname == "_Skip.__call__"

(there's probably a way to get the qualified name before py311 but wouldn't bother)

One thing I was concerned about is sphinx autodoc not understanding the __call__ business but it seems to work as expected if the docstring is put on the class.

The __call__ indirection is also a slight degradation for "go to definition" functionality but I think it's fine.

So I think the __call__ should work and seems cleaner to me, but the current code in the PR is also a strict improvement and fixes the problem, so no problem going with that if you don't want to do the bigger change.

[karlicoss]

Thanks for suggestions @bluetech! Converted all of skip/fail/etc to simple protocols with __call__ and it looks quite clean now, and types are simpler, so hopefully less chances of type checkers shenanigans (e.g. ty correctly infers Never as a result of pytest.skip(...)). I refined MR description and commit message to reflect the change, so feel free to use the last one when you squash!

[karlicoss]

Thank you! Appreciate your time