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.

Sign up for GitHub

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

Do not parse default values as types #1350

Closed
wants to merge 2 commits into from
Closed

Do not parse default values as types #1350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a390977
Do not parse default values as types
JelleZijlstra Jan 12, 2023
f8b68e8
better fix, thanks Martin
JelleZijlstra Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pytype/pyi/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ def visit_Pyval(self, node):
if node.type == "str" and not self.subscripted:
return self.convert_late_annotation(node.value)

def visit_arg(self, node):
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

return self.visit ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JelleZijlstra JelleZijlstra Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@martindemello martindemello Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


def visit_Tuple(self, node):
return tuple(node.elts)

Expand Down
7 changes: 7 additions & 0 deletions pytype/pyi/parser_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,13 @@ def test_params(self):
self.check("def foo(x: int, y: str, z: bool,) -> int: ...",
"def foo(x: int, y: str, z: bool) -> int: ...")

def test_defaults(self):
self.check("""
def f(x: int = 3, y: str = "y") -> None: ...
""", """
def f(x: int = ..., y: str = ...) -> None: ...
""")

def test_star_params(self):
self.check("def foo(*, x) -> str: ...")
self.check("def foo(x: int, *args) -> str: ...")
Expand Down