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

Add support for type annotations to attr: ast parsing #3391

Merged
merged 6 commits into from Jun 19, 2022

Conversation

karlotness
Copy link
Contributor

@karlotness karlotness commented Jun 19, 2022

Summary of changes

This extends the ast parsing setuptools does to load attributes without evaluating a module to also process simple assignments with type annotations. Those produce a different node ast.AnnAssign rather than ast.Assign and require slightly different handling since the node has a different attribute target vs targets.

Since that seemed like it might make the generators more complicated, I reworked things into for loops and made some other tweaks. I'd be happy to take a different approach though.

The existing ast.Assign nodes should pass through exactly the same processing. The AttributeError that could result should also be the same too, except that it won't be chained with a StopIteration if the outer for loop is exhausted (as it would be with the next() call).

This doesn't close any existing open issue that I can find. There is #2563 which looks related. I think in that case the attribute loading worked because setuptools fell back to evaluating the module when the ast processing likely failed.

That fallback works in some cases, but it causes problems in when the module isn't importable until after the build (for example, extension modules that need to be compiled). The ast parsing is really helpful in these cases, and it would be nice if it could also handle type annotations.

Pull Request Checklist

karlotness and others added 5 commits June 18, 2022 22:24
When walking the ast of a module, look for AnnAssign nodes in addition
to Assign to support assignments with type annotations, for example.
Since we have to read different attributes, split the generators into
a for loop. Existing ast.Assign nodes follow the same processing as
before.
@abravalheri
Copy link
Contributor

abravalheri commented Jun 19, 2022

Thank you very much for the contribution @karlotness (and the awesome attention to details, I completely forgot that the AST will handle typed assignments differently).

I notice that the is one edge case that might break:

attr: str

The AST also parses this statement as an ast.AnnAssign, but the value is set to None. If we try to parse None via ast.literal_eval we probably get an exception.

I will propose a change directly to your repository via PR (if you merge it, this PR is automatically updated). The suggestions I will send you will also include a way of having the generators back and avoid the duplication when raising the exception.

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider the fix suggested in karlotness#1 for the edge case when ast.AnnAssign has a None value

Refactor `StaticModule.__getattr__` and fix `ast.AnnAssign` edge case
@karlotness
Copy link
Contributor Author

karlotness commented Jun 19, 2022

That looks good, thanks. I hadn't considered the split type-annotation and later assignment. I took a look and you're right, looks like that would have led to an error and missed a later actual assignment. Thanks for taking a look and for the quick fix.

@abravalheri abravalheri merged commit 8e83289 into pypa:main Jun 19, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants