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.

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

Error when a class member is redefined #6686

Merged
merged 9 commits into from Apr 30, 2019

Conversation

@onlined
Copy link
Contributor

commented Apr 17, 2019

Fixes #6571.

Fix

@onlined onlined changed the title Fix Error when a class member is redefined Apr 17, 2019

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

Note that your fix broke a whole bunch of new analyzer tests.

@onlined

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Fixed.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks! Here I have some comments.

@@ -2563,6 +2563,9 @@ def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool, is_final:
if cur_node and is_final:
# Overrides will be checked in type checker.
self.fail("Cannot redefine an existing name as final", lval)
if (not lval.node and cur_node and isinstance(cur_node.node, Var) and
cur_node.node.is_inferred and explicit_type):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

TBH I don't follow the logic here, maybe add a comment explaining this?

This comment has been minimized.

Copy link
@onlined

onlined Apr 26, 2019

Author Contributor

Yes, I can write a comment. But before, let me explain: Fail if passing first time, the node exists and it's a Var node, it's type is inferred, and new assignment has an explicit type.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

Basically yes. But please try to avoid this:

x += 1  # Increment x

Describe the intent and how the code achieves it.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 29, 2019

Collaborator

@onlined Are you going to add this comment (also in the old analyzer)?

This comment has been minimized.

Copy link
@onlined

onlined Apr 29, 2019

Author Contributor

Yes, I will.

class C:
def __init__(self) -> None:
self.foo = 12
self.foo: int = 12 # E: Name 'self.foo' already defined on line 3

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

Could you please also add a test for something like this:

class C:
    foo = 12
    def __init__(self) -> None:
        self.foo: int = 12  # E: Attribute 'foo' already defined on line 2

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

You didn't add this test.

@@ -2288,6 +2288,9 @@ def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool, is_final:
if cur_node and is_final:
# Overrides will be checked in type checker.
self.fail("Cannot redefine an existing name as final", lval)
if (cur_node and isinstance(cur_node.node, Var) and cur_node.node.is_inferred and
explicit_type):
self.name_already_defined('self.' + lval.name, lval, cur_node)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

I wouldn't use this error message because self.foo is not a name, it is an attribute. I would better use Attribute "foo" already defined ....

This comment has been minimized.

Copy link
@onlined

onlined Apr 26, 2019

Author Contributor

Fixed.

@@ -2563,6 +2563,9 @@ def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool, is_final:
if cur_node and is_final:
# Overrides will be checked in type checker.
self.fail("Cannot redefine an existing name as final", lval)
if (not lval.node and cur_node and isinstance(cur_node.node, Var) and
cur_node.node.is_inferred and explicit_type):
self.name_already_defined('self.' + lval.name, lval, cur_node)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

Using this in the new analyzer is dangerous, it not not only shows the error, but also has some special code for deferring unknown names.

This comment has been minimized.

Copy link
@onlined

onlined Apr 26, 2019

Author Contributor

I couldn't understand what you meant. Could you explain it a little more?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

I actually confused with with name_not_defined(). This one is OK. Anyway, did you check that in the situation where an error is emitted, no new variable created, and the l.h.s. points to an existing one?

This comment has been minimized.

Copy link
@onlined

onlined Apr 29, 2019

Author Contributor

Yes, basically if cur_node is not None, l.h.s. points to an existing variable.

def __init__(self) -> None:
self.foo = 12
self.foo: int = 12 # E: Name 'self.foo' already defined on line 3

This comment has been minimized.

Copy link
@onlined

onlined Apr 26, 2019

Author Contributor

I am not very sure about this. Should I really add same tests to new semantic analyzer tests?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 26, 2019

Collaborator

You don't need to duplicate tests, most test are currently run with both analyzers.

This comment has been minimized.

Copy link
@onlined

onlined Apr 29, 2019

Author Contributor

Removed redundant tests.

onlined added some commits Apr 26, 2019

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

Thanks for updates! I left few more comments.

onlined added some commits Apr 29, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for updates! I have just one remaining question, otherwise this PR is ready.

@@ -2563,6 +2563,9 @@ def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool, is_final:
if cur_node and is_final:
# Overrides will be checked in type checker.
self.fail("Cannot redefine an existing name as final", lval)
if (not lval.node and cur_node and isinstance(cur_node.node, Var) and
cur_node.node.is_inferred and explicit_type):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 29, 2019

Collaborator

@onlined Are you going to add this comment (also in the old analyzer)?

@ilevkivskyi ilevkivskyi merged commit ebd0479 into python:master Apr 30, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.