Do not parse default values as types#1350
Closed
JelleZijlstra wants to merge 2 commits intogoogle:mainfrom
Closed
Conversation
Contributor
|
@martindemello Could you take a look at this one? Based on the other usages of visit(), it looks like it has a return value, and I'm not sure if discarding that is okay or not. |
Contributor
|
you're right, annotation visitor methods should return a value because of this usage: https://github.com/google/pytype/blob/main/pytype/pyi/parser.py#L315 |
pytype/pyi/parser.py
Outdated
| # Visit only the annotation, not the default (which may be a | ||
| # string but should not be interpreted as a type annotation). | ||
| if node.annotation is not None: | ||
| self.visit(node.annotation) |
Contributor
There was a problem hiding this comment.
return self.visit ...
Contributor
Author
There was a problem hiding this comment.
Then we'd return a different type which seems wrong, right? I imagine I'll have to construct a new ast.arg with the visited annotation.
Contributor
There was a problem hiding this comment.
hm, i don't think so. if you look at convert_node_annotations it's only ever modifying the annotations field of the node, not the node itself.
Contributor
Author
There was a problem hiding this comment.
But then this code should be useless, because there cannot be an arg node inside an annotation (unless there's a lambda in the annotation I suppose). Indeed, I just checked and the test I added passes even without this change. I'll have to go back and figure out the real source of the crash.
Contributor
There was a problem hiding this comment.
ah yes :) i did wonder why the change was needed in the first place but i figured you had found some odd corner case in the ast that we weren't handling well. if you can share the file the parser crashes on i can take a look as well.
Contributor
Author
There was a problem hiding this comment.
Thanks! It's from python/typeshed#9501. There are a lot of crashes, but the one I posted below points to https://github.com/python/typeshed/blob/7f85a0cd903f51a63030620be8ac9e1b52a73d94/stdlib/email/header.pyi line 26 which has continuation_ws: str = " ", in a default.
Looking at the stack trace again I do think I am approximately in the right place here, but not sure why the test doesn't reproduce the crash.
Contributor
There was a problem hiding this comment.
okay, found the issue. here's the fix you need:
--- a/google3/third_party/py/pytype/pyi/parser.py
+++ b/google3/third_party/py/pytype/pyi/parser.py
@@ -129,6 +129,10 @@ def _attribute_to_name(node: ast3.Attrib
class AnnotationVisitor(visitor.BaseVisitor):
"""Converts typed_ast annotations to pytd."""
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ self._node_children[self._ast.arguments] = ["args", "kwonlyargs"]
+
def show(self, node):
it will prevent iterating over the defaults.
Contributor
There was a problem hiding this comment.
the crash was specifically due to the empty/whitespace-only string, btw, it was trying to read it as an annotation and failing to find one. your test passed because it used "y" as a default, which is a syntactically valid type annotation as well.
Contributor
Author
There was a problem hiding this comment.
Thanks, good catch! I think your proposed fix isn't quite right because it would exclude annotations on posonly args, *args, and **kwargs. I'll push a version that handles these.
Contributor
Author
|
Traceback I'm trying to fix for reference: Details |
Contributor
Author
|
The CI failure seems spurious, some package failed to download. |
martindemello
approved these changes
Jan 13, 2023
Contributor
martindemello
left a comment
martindemello
left a comment
There was a problem hiding this comment.
thanks, looks good! we'll merge it internally and push back to github.
martindemello
pushed a commit
that referenced
this pull request
Jan 13, 2023
See python/typeshed#9501. Resolves #1350 PiperOrigin-RevId: 501717379
martindemello
pushed a commit
that referenced
this pull request
Jan 13, 2023
See python/typeshed#9501. Resolves #1350 PiperOrigin-RevId: 501717379
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See python/typeshed#9501.