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

Typing the ast.AST subclass constructors #8378

Closed
multimeric opened this issue Jul 24, 2022 · 5 comments · Fixed by #11880
Closed

Typing the ast.AST subclass constructors #8378

multimeric opened this issue Jul 24, 2022 · 5 comments · Fixed by #11880
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: improvement Improve/refactor existing annotations, other stubs issues

Comments

@multimeric
Copy link

We currently have static types for the fields of most (all?) ast classes, but none of these have typed constructors, e.g.:

typeshed/stdlib/_ast.pyi

Lines 79 to 86 in 62cde01

class ClassDef(stmt):
if sys.version_info >= (3, 10):
__match_args__ = ("name", "bases", "keywords", "body", "decorator_list")
name: _Identifier
bases: list[expr]
keywords: list[keyword]
body: list[stmt]
decorator_list: list[expr]

I think technically this might be because none of these subclasses actually have a unique constructor, but that shouldn't stop us from typing each of the constructors, since in practise the constructor arguments must correspond to the class fields.

Before I do this, though, I'm firstly wondering if an easy solution here would be to apply the dataclass_transform decorator (note: not the same as the dataclass decorator). This would simply tell the type checker that all the class fields can and should be provided in the constructor, which is broadly correct. However I don't have a deep understanding of how the ast.AST constructor works, so this might not be the correct behaviour. For ClassDef this might look like:

from typing_extensions import dataclass_transform

@dataclass_transform
class ClassDef(stmt): 
    if sys.version_info >= (3, 10): 
        __match_args__ = ("name", "bases", "keywords", "body", "decorator_list") 
    name: _Identifier 
    bases: list[expr] 
    keywords: list[keyword] 
    body: list[stmt] 
    decorator_list: list[expr] 

If this isn't sufficient, I propose that we simply add an __init__() stub to each subclass. For instance, for the ClassDef above, this might look like:

class ClassDef(stmt): 
    if sys.version_info >= (3, 10): 
        __match_args__ = ("name", "bases", "keywords", "body", "decorator_list") 
    name: _Identifier 
    bases: list[expr] 
    keywords: list[keyword] 
    body: list[stmt] 
    decorator_list: list[expr] 

    def __init__(
        name: _Identifier 
        bases: list[expr] 
        keywords: list[keyword] 
        body: list[stmt] 
        decorator_list: list[expr] 
    ):
        ...
@hauntsaninja
Copy link
Collaborator

I'd be in favour of adding __init__ stubs to each subclass. PR welcome!

@multimeric
Copy link
Author

Thoughts on the dataclass_transform idea?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 24, 2022

I would prefer explicit __init__s, since typically typeshed prefers to mirror closely what's happening at the runtime, rather than getting fancy and lying. Also not sure how well supported PEP 681 currently is by type checkers — last I checked mypy doesn't yet support it.
(Note dataclass_transform would have to be applied to a base class to work, not to the class itself. You'd also want to be careful to get all the params right, e.g. we shouldn't synthesise comparison methods and stuff)

@Akuli
Copy link
Collaborator

Akuli commented Jul 24, 2022

Invoking the constructors seems fragile to me. For example, if you instantiate ast.Module, it's easy to forget passing in a second argument and end up with a Module that doesn't have a type_ignores attribute:

>>> ast.dump(ast.parse('f()'))
"Module(body=[Expr(value=Call(func=Name(id='f', ctx=Load()), args=[], keywords=[]))], type_ignores=[])"
>>> ast.parse('f()').type_ignores
[]
>>> ast.Module(body=[]).type_ignores
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Module' object has no attribute 'type_ignores'

You can even make an empty module that doesn't have any attributes:

>>> ast.dump(ast.Module())
'Module()'
>>> ast.dump(ast.Module([]))
'Module(body=[])'
>>> ast.dump(ast.Module([], []))
'Module(body=[], type_ignores=[])'

So code that instantiates AST nodes will break every time a new attribute is added. Maybe it means that you shouldn't do this and the type checker should error if you do this, or maybe it just means that type checking is unusually important here.

@multimeric
Copy link
Author

@Akuli I interpret it as type checking being particularly important for this module, which is why I raised this issue. imo ideally the actual constructor would validate the arguments too, but it doesn't seem to do that, and I don't want to touch the C code that provides the constructor, so I'm instead aiming at a type annotated constructor.

@AlexWaygood AlexWaygood added the stubs: improvement Improve/refactor existing annotations, other stubs issues label Jul 24, 2022
@AlexWaygood AlexWaygood added the help wanted An actionable problem of low to medium complexity where a PR would be very welcome label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: improvement Improve/refactor existing annotations, other stubs issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants