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
[mypyc] always add implicit None return type to __init__ method #9866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Looks good, just found one edge case that may require a minor tweak.
mypyc/irbuild/mapper.py
Outdated
@@ -132,7 +132,11 @@ def fdef_to_sig(self, fdef: FuncDef) -> FuncSignature: | |||
else: | |||
# Handle unannotated functions | |||
arg_types = [object_rprimitive for arg in fdef.arguments] | |||
ret = object_rprimitive | |||
# We at least know the return type for __init__ will be None. | |||
if fdef.name == '__init__': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could fdef
be a module-level function with the (confusing) name __init__
? Add test for this. We should allow top-level functions to return any values, even if they are named __init__
for some reason.
(You should be able to compare the FuncDef
info
attribute against FUNC_NO_INFO
to check whether it's a method.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice catch this will not work for module-level functions named __init__
! I will add a check and test for this. Thank you for the FUNC_NO_INFO
tip.
…nnotation to module-level __init__ functions
mypy/nodes.py
Outdated
@@ -683,6 +683,10 @@ def __init__(self, | |||
def name(self) -> str: | |||
return self._name | |||
|
|||
@property | |||
def is_module_level_func(self) -> bool: | |||
return self.info == FUNC_NO_INFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JukkaL my instinct was to "hide" the use of FUNC_NO_INFO
. I'm not sure if that was a good instinct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think that this is unnecessary (see below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! This looks correct now. Left some some comments about style. (Sorry, my original suggestion to use FUNC_NO_INFO
was a bad idea.)
mypyc/test-data/run-classes.test
Outdated
[case testInitMethodWithMissingNoneReturnAnnotation] | ||
class C: | ||
def __init__(self): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also try constructing an instance of C
(just make sure it produces a result).
mypy/nodes.py
Outdated
@@ -683,6 +683,10 @@ def __init__(self, | |||
def name(self) -> str: | |||
return self._name | |||
|
|||
@property | |||
def is_module_level_func(self) -> bool: | |||
return self.info == FUNC_NO_INFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think that this is unnecessary (see below).
mypy/suggestions.py
Outdated
@@ -437,7 +437,7 @@ def get_suggestion(self, mod: str, node: FuncDef) -> PyAnnotateSignature: | |||
if self.no_errors and orig_errors: | |||
raise SuggestionFailure("Function does not typecheck.") | |||
|
|||
is_method = bool(node.info) and not node.is_static | |||
is_method = not node.is_module_level_func and not node.is_static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave this as it was, since using an info
attribute as a boolean is a common idiom.
mypyc/irbuild/mapper.py
Outdated
@@ -132,7 +132,12 @@ def fdef_to_sig(self, fdef: FuncDef) -> FuncSignature: | |||
else: | |||
# Handle unannotated functions | |||
arg_types = [object_rprimitive for arg in fdef.arguments] | |||
ret = object_rprimitive | |||
# We at least know the return type for __init__ methods will be None. | |||
is_init_method = fdef.name == '__init__' and not fdef.is_module_level_func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you can just use ... and bool(fdef.info)
. Your suggestion to use a property is reasonable, but we have many checks where we do the equivalent of if fdef.info
so it would add some inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thank you that makes sense. Applying this update and the suggestions above now.
…ove test case for missing __init__ return annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now! Thanks for updates. This fixes a major issue.
Description
Fixes mypyc/mypyc#792.
Always give
__init__
an implicitNone
return type so that programs likecompile.
Test Plan
Included a test program that doesn't compile with the existing application code.