-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] always add implicit None return type to __init__ method #9866
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
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
| 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
|
|
||
| @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
|
|
||
| @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
| 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
| 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 implicitNonereturn type so that programs likecompile.
Test Plan
Included a test program that doesn't compile with the existing application code.