Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

__match_args__ annotations #5402

Closed
freundTech opened this issue May 10, 2021 · 9 comments
Closed

__match_args__ annotations #5402

freundTech opened this issue May 10, 2021 · 9 comments

Comments

@freundTech
Copy link
Contributor

freundTech commented May 10, 2021

Python 3.10 adds the match statement and with it adds the __match_args__ attribute to some classes.
To statically check class patterns in match statements the value of that attribute must be statically known.

I'm currently implementing match statement support for mypy and would like to hear some opinions on how __match_args__ should be annotated.

Let's take ast.BinOp as an example. It's __match_args__ is ('left', 'op', 'right').

One option to annotate it would be

__match_args__: Final[Tuple[Literal["left"], Literal["op"], Literal["right"]]]

That is however very verbose and not easy to read. Mypy can also deal with annotations in the form of

__match_args__: Final = ("left", "op", "right")

, inferring that the strings must be literal. This approach is much easier to read, but might be harder for type checkers to support and breaks with established typeshed conventions.

I would like to hear some opinions on those two approaches and other suggestions, especially from developers of the other type checkers.

A second point of discussion would be actually adding the annotations. In theory they could be added by hand, however that would be a lot of work. Ideally a program could be written to look up the match args at runtime and automatically add them to the stubs.

People who might be interested in this:
@erictraut

@JelleZijlstra
Copy link
Member

I feel like the closest precedent is __all__, which is also a usually static collection of strings. We already have __all__ = ["list", "of", "strings"] in a few places, so I think that makes sense for __match_args__ too. I feel like we shouldn't need the Final qualifier to be made explicit.

@erictraut
Copy link
Contributor

Pyright currently supports all of the following forms within type stubs:

__match_args__: Final[Tuple[Literal["left"], Literal["op"], Literal["right"]]]
__match_args__: Tuple[Literal["left"], Literal["op"], Literal["right"]]
__match_args__: Final = ("left", "op", "right")
__match_args__ = ("left", "op", "right")

It does not support lists because type inference of list expressions doesn't preserve types of individual elements within the list. (I tried to convince Guido that __match_args__ should support only tuples and not lists, but PEP 634 was already too far along for any change to be made at that point. I still think it's very unfortunate that it supports lists.) Jelle is correct that lists are supported for the __all__ statement, but that requires a bunch of special-case logic in the type checker, it's in the binder rather than the semantic analyzer, and it therefore doesn't support arbitrary expressions but is restricted to very specific syntactic patterns. I don't want to duplicate all of that logic for __match_args__ to support lists.

I realize that your question was specifically focused on type stub files. We also need to consider inlined types in a "py.typed" library. Pyright implements some very specific rules about where it will and won't infer types in a "py.typed" source (".py") file. Inference is used very sparingly for several reasons documented here. These rules, as currently implemented, would require an explicit type annotation (such as __match_args__: Final[Tuple[Literal["left"], Literal["op"], Literal["right"]]]) for proper behavior of __match_args__ within a "py.typed" source file. It arguably makes sense to expand those rules to accommodate type inference in the case of tuple expressions with string literals used specifically for __match_args__ definitions. This obviously doesn't impact typeshed, which is limited to ".pyi" files.

@JelleZijlstra
Copy link
Member

PEP 634 actually specifies that __match_args__ must be a tuple (in https://www.python.org/dev/peps/pep-0634/#class-patterns, If the returned value is not a tuple, the conversion fails and TypeError is raised.).

@freundTech
Copy link
Contributor Author

freundTech commented May 10, 2021

@JelleZijlstra
At runtime __match_args__ isn't final and can be overwritten, which can change the match behavior of classes after they have been defined.
For this reason my implementation in mypy currently emits a warning every time it encounters a non-final __match_args__, telling the user that class patterns can't be statically checked with non-final match args.

Non-final match args are useless for type checkers, because we can not know if they will be changed at runtime. You could just assume them to be unchanged, but that is obviously unsave.

Match args could be made implicitly final, but I feel like that can lead to a lot of confusion and would be inconsistent even with __all__ (mypy allows assigning to __all__ of an imported module).

@freundTech
Copy link
Contributor Author

PEP 634 actually specifies that __match_args__ must be a tuple (in https://www.python.org/dev/peps/pep-0634/#class-patterns, If the returned value is not a tuple, the conversion fails and TypeError is raised.).

This change was made last month: python/peps#1909

@erictraut
Copy link
Contributor

Ah, that's great to hear. I guess my feedback didn't come in too late to influence the spec. You just made my day. :)

@JelleZijlstra
Copy link
Member

At runtime match_args isn't final and can be overwritten, which can change the match behavior of classes after they have been defined.

That just feels overly pedantic to me. There are several other magical attributes that would influence the type checker (e.g., __hash__ = None, __all__), and they're not required to be Final. For __all__, for example, I doubt mypy will adjust the behavior of from ... import * if it encounters a change to __all__. I think it's sufficient to document that if you mutate __match_args__, mypy won't support it.

@freundTech
Copy link
Contributor Author

I could later lift that restriction if consensus is that it's not needed.
For mypy this would require special-casing __match_args__ as mypy currently only infers literals for final types:

from typing import Final

test = ("test", "abc")
test2: Final = ("test", "abc")

reveal_type(test)  # Tuple[builtins.str, builtins.str]
reveal_type(test2)  # Tuple[Literal['test']?, Literal['abc']?]

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 12, 2022

Having mypy infer __match_args__ to be Final proved to be a suboptimal approach: it would have made it impossible to add __match_args__ to classes in _ast.pyi without littering the stub with # type: ignores (see python/mypy#12411 (comment)). @JukkaL fixed this in python/mypy#12415, and typeshed now has ~100% __match_args__ coverage in the stdlib (according to stubtest).

For non-@final classes, our current approach is to do __match_args__ like this:

typeshed/stdlib/_ast.pyi

Lines 31 to 34 in 55eb19d

class TypeIgnore(type_ignore):
if sys.version_info >= (3, 10):
__match_args__ = ("lineno", "tag")
tag: str

While for @final classes, our current approach is to do __match_args__ like this:

@final
class uname_result(structseq[str], tuple[str, str, str, str, str]):
if sys.version_info >= (3, 10):
__match_args__: Final = ("sysname", "nodename", "release", "version", "machine")
@property
def sysname(self) -> str: ...
@property
def nodename(self) -> str: ...
@property
def release(self) -> str: ...
@property
def version(self) -> str: ...
@property
def machine(self) -> str: ...

And here's how mypy infers the types on 0.961:

from ast import TypeIgnore
from os import uname_result

reveal_type(TypeIgnore.__match_args__)  # Revealed type is "Tuple[Literal['lineno']?, Literal['tag']?]"
reveal_type(uname_result.__match_args__)  # Revealed type is "Tuple[Literal['sysname']?, Literal['nodename']?, Literal['release']?, Literal['version']?, Literal['machine']?]"
TypeIgnore.__match_args__ = ('foo', 'bar')  # error: Cannot assign to "__match_args__"

https://mypy-play.net/?mypy=latest&python=3.10&gist=8f8033d871b64b21e5ba9a8cb570cd04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants