use callable protocols for pytest.skip/exit/fail/xfail instead of _WithException wrapper with __call__ 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:

• [ty] Understand classes that inherit from subscripted Protocol[] as generic astral-sh/ruff#17832 (comment)
• 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:

$ mypy test_pytest_skip.py
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:

$ mypy test_pytest_skip.py
test_pytest_skip.py:2: note: Revealed type is "_pytest.outcomes._Skip"
test_pytest_skip.py:3: note: Revealed type is "type[_pytest.outcomes.Skipped]"
test_pytest_skip.py:4: note: Revealed type is "Never"

ty type checker is also inferring the same types correctly now.

All types are matching and propagated correctly (most importantly, Never as a result of pytest.skip(...) call

[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) 56b3937

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:

pytest/testing/python/collect.py

Lines 1075 to 1081 in 9e9633d

	def test_skip_simple(self):
	with pytest.raises(pytest.skip.Exception) as excinfo:
	pytest.skip("xxx")
	assert excinfo.traceback[-1].frame.code.name == "skip"
	assert excinfo.traceback[-1].ishidden(excinfo)
	assert excinfo.traceback[-2].frame.code.name == "test_skip_simple"
	assert not excinfo.traceback[-2].ishidden(excinfo)

$ 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!

[bluetech]

Thanks for the update @karlicoss. Please see my comments, other than them it looks good!

[bluetech]

Let's put the docstring on the class, so that sphinx picks it up correctly. You can verify that it works by checking that it shows up in the PR docs preview (currently empty): https://pytest--13445.org.readthedocs.build/en/13445/reference/reference.html#pytest-skip

[karlicoss]

Oh this docs pipeline is neat, didn't notice that! Moved and seems like it's picked up correctly now

[bluetech]

Usually we omit an explicit annotation when it is inferred.

Also, here I think a ClassVar annotation would be appropriate. So Exception: ClassVar = Exit.

[karlicoss]

Good idea, added ClassVar -- see below comment re: type annotation

[bluetech]

My slight preference it to have the exit = _Exit etc. below the class _Exit instead of group at the bottom, it makes it easier to understand what it's about IMO.

[karlicoss]

done!

[bluetech]

(Here too I think we can omit the explicit type annotations)

[karlicoss]

So I added them because otherwise ty was inferring pytest.skip(reason=...) as Unknown instead of Never -- which is a consequence (and possibly there is some bug involved too) of how ty handles unannotated module/class attributes at the moment.
I reported it here astral-sh/ty#433 (comment)

So if we remove explicit annotation, ty would infer pytest.skip(...) as Unknown at the moment -- this would defeat the purpose of typing annotations to an extent (although would still be a net improvement as at least it wouldn't report pytest.skip as false positives).
IMO would be nice to keep them even if it's a minor 'style' inconsistency as it doesn't make anything else worse. We could leave a comment in code explaining why it's annotated (so someone doesn't remove by accident), but now that the outcome objects are next to the corresponding class it's a bit harder, since would require duplicating the comment 4 times :) Not sure what to do, but looks like this file really isn't modified frequently, and the risk of losing annotations is low -- and in the meantime ty might sort out the Unknown shenanigans.

I can remove the Exception annotations apart from ClassVar if you prefer -- the only downside is that ty would infer pytest.skip.Exception as Unknown, but this doesn't feel like a big deal and everything else would still work as expected.

Let me know what you think!

[bluetech]

Can't say I understand the rationale of ty to behave this way, but it's intentional and it's just a minor style thing. So let's keep the annotations.

[bluetech]

Thanks @karlicoss!

[karlicoss]

Thank you! Appreciate your time