KEMBAR78
[mypyc] always add implicit None return type to __init__ method by thomasjohns · Pull Request #9866 · python/mypy · GitHub
Skip to content

Conversation

@thomasjohns
Copy link
Contributor

Description

Fixes mypyc/mypyc#792.

Always give __init__ an implicit None return type so that programs like

class C:
    def __init__(self):  # missing `-> None` annotation
        pass

compile.

Test Plan

Included a test program that doesn't compile with the existing application code.

@thomasjohns thomasjohns changed the title always add implicit None return type to __init__ method [mypyc] always add implicit None return type to __init__ method Dec 31, 2020
Copy link
Collaborator

@JukkaL JukkaL left a 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.

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__':
Copy link
Collaborator

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.)

Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Collaborator

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).

Copy link
Collaborator

@JukkaL JukkaL left a 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.)

[case testInitMethodWithMissingNoneReturnAnnotation]
class C:
def __init__(self):
pass
Copy link
Collaborator

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
Copy link
Collaborator

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).

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
Copy link
Collaborator

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.

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Collaborator

@JukkaL JukkaL left a 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.

@JukkaL JukkaL merged commit 0c5936e into python:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C compilation fails if '__init__' has no return type

2 participants