Skip to content

Commit

Permalink
Remove Never annotations when merging pyi files.
Browse files Browse the repository at this point in the history
`Never` is a valid type annotation, but it's most likely wrong if it's inferred by pytype, and it has a chain effect that all downstream code starts to get treated as unreachable.

Fixing pytype to not infer `Never` on return types should be better, but this should be dealt as a separate fix.

PiperOrigin-RevId: 684355324
  • Loading branch information
h-joo authored and copybara-github committed Oct 10, 2024
1 parent a370f22 commit 65ad8fb
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 7 deletions.
22 changes: 15 additions & 7 deletions pytype/tools/merge_pyi/merge_pyi.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,28 @@ def _merge_csts(*, py_tree, pyi_tree):
).transform_module(py_tree)


class RemoveAnyTransformer(cst.CSTTransformer):
"""Transform away every `Any` annotations in function returns and variable assignments."""
class RemoveAnyNeverTransformer(cst.CSTTransformer):
"""Transform away every `Any` and `Never` annotations in function returns and variable assignments.
def _is_any_annotation(self, annotation: expression.Annotation | None):
For putting 'Any's, it's basically a no-op, and it doesn't help readability
so better not put anything when pytype gives up.
Having 'Never' annotated on function returns and variables is valid, but
they're most likely wrong if it's inferred by pytype, and it has a chain
effect that all downstream code starts to get treated as unreachable.
"""

def _is_any_or_never(self, annotation: expression.Annotation | None):
return (
annotation
and isinstance(annotation, expression.Name)
and annotation.value == "Any"
and annotation.value in ("Any", "Never")
)

def leave_FunctionDef(
self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef
) -> cst.CSTNode:
if original_node.returns and self._is_any_annotation(
if original_node.returns and self._is_any_or_never(
original_node.returns.annotation
):
return updated_node.with_changes(returns=None)
Expand All @@ -53,7 +61,7 @@ def leave_FunctionDef(
def leave_AnnAssign(
self, original_node: cst.AnnAssign, updated_node: cst.AnnAssign
) -> cst.CSTNode:
if self._is_any_annotation(original_node.annotation):
if self._is_any_or_never(original_node.annotation):
return cst.Assign(
targets=[cst.AssignTarget(target=updated_node.target)],
value=updated_node.value,
Expand Down Expand Up @@ -100,7 +108,7 @@ def merge_sources(*, py: str, pyi: str) -> str:
py_cst = cst.parse_module(py)
pyi_cst = (
cst.parse_module(pyi)
.visit(RemoveAnyTransformer())
.visit(RemoveAnyNeverTransformer())
.visit(RemoveTrivialTypesTransformer())
)
merged_cst = _merge_csts(py_tree=py_cst, pyi_tree=pyi_cst)
Expand Down
4 changes: 4 additions & 0 deletions pytype/tools/merge_pyi/test_data/func_annot.pep484.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
def f() -> int:
return 1

# The Never type in pyi file should not be merged here.
def g():
raise Exception("hi")
4 changes: 4 additions & 0 deletions pytype/tools/merge_pyi/test_data/func_annot.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
def f():
return 1

# The Never type in pyi file should not be merged here.
def g():
raise Exception("hi")
3 changes: 3 additions & 0 deletions pytype/tools/merge_pyi/test_data/func_annot.pyi
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
from typings import Never

def f() -> int: ...
def g() -> Never: ...

0 comments on commit 65ad8fb

Please sign in to comment.