Disallow Any on public interface types #6815
Conversation
src/pytest/__init__.py
Outdated
| cmdline = cmdline | ||
| ExitCode = ExitCode | ||
| #hookimpl = hookimpl | ||
| #hookspec = hookspec |
There was a problem hiding this comment.
hookimpl and hookspec are the two which aren't very easy to type.
I believe we want to do something similar to #3342 for pluggy in order to make this possible.
There was a problem hiding this comment.
With regard to hookimpl/hookspec it would also be great to have some plugin or similar that would make mypy aware of hookimpls beloning to hookspecs of the same name.
(Maybe also / additionally separate types could be used, but that might be tedious)
(Just throwing in some food for thought)
Related: #6725
There was a problem hiding this comment.
The hookimpl and hookspec decorator can be typed relatively easily - but in order to do so, we need to have type stubs or exported types from pluggy.
There was a problem hiding this comment.
I think it is more complex: how does mypy know that @hookimpl(pytest_foo) refers to @hookspec(pytest_foo)?
What I've meant here is that the former should be validated against the latter somehow.
There was a problem hiding this comment.
https://github.com/pytest-dev/pluggy/blob/master/src/pluggy/hooks.py#L22
HookSpec looks like it is a decorator which takes a handful of bools as arguments
I suppose the project_name is something which we could encode into the type system (HookSpecMarker[_T] where _T is the project name).
I'm not sure I follow how hookimpl and hookspec are tied together here.
| # For mypy Any type checking purposes. | ||
| # This file sets disallow_any_expr to ensure that the public API | ||
| # does not have dynamic typing via Any. Manually using each public API | ||
| # type as an expression to enforce this. |
There was a problem hiding this comment.
Can this be moved to __init__.pyi?
There was a problem hiding this comment.
We can explicitly put types into an init.pyi, but that seems less desirable than having the code annotated inline. With the current setup, the code is annotated inline, and the
disallow_any_expr ensures that Any is not used as an annotation in this file.
disallow_any_expr was implemented into mypy here python/mypy#3519
Notably, it requires that each item exists in the file as an expression. The import itself does not count as an expr here - so we need to have a series of a = a style expression assigments
There was a problem hiding this comment.
In that case I'd prefer to simply not use disallow_any_expr for now; while I dislike this part of the patch the rest is fantastic and I'd like to merge it soon. We can revisit the more controversial part later 🙂
There was a problem hiding this comment.
@nipunn1313 I've meant to move the "Manually using each public API type as an expression to enforce this." part of it. It is only boilerplate, isn't it?
There was a problem hiding this comment.
Hi - sorry for the delay - I was on vacation.
Unfortunately - I do not believe we will get any protection unless we use the variables as expressions within the file. The imports themselves are not type checked or protected - only when used as expressions.
If we use a .pyi file - then the annotations in the .pyi file override the annotations in the actual .py file - and we will lose annotation coverage from the actual code - which is the purpose of #3342 py.typed strategy
There was a problem hiding this comment.
disallow_any_expr is actually the most important piece - as it's the one that is guaranteeing that the public interface is typed.
The public interface is only the __init__.py file - so without this boilerplate - we're not actually adding much value.
| UsageError = UsageError | ||
| __pytestPDB = __pytestPDB | ||
| _fillfuncargs = _fillfuncargs | ||
| fixture = fixture |
There was a problem hiding this comment.
fixture and yield_fixture are currently untyped. Shouldn't it fail for them (for example)?
|
Hi @nipunn1313, I think this is probably not the way we want to go right now, and I'm not sure it actually works as intended (see previous comment). It might make more sense once the typing is more thorough. I'll try to keep the issue updated as this progresses. |
This ensures that public interface methods must be typed
This should help with #3342
Per discussion there, ideally we shouldn't require full typing before we move forward, but this should give us a sense of the typing coverage which we have currently.