Show class that originally defined attribute in output by ucodery · Pull Request #6806 · python/mypy

ucodery · 2019-05-08T23:39:59Z

If a subclass or its instance attempts to assign an incompatible type
display the class that originally defined the type.

Fixes issue #2500

If a subclass or its instance attempts to assign an incompatible type display the class that originally defined the type.

ilevkivskyi

Thanks for PR! Here I left some comments, I will review the tests in the next round.

ilevkivskyi · 2019-05-09T17:45:44Z

+                    lvalue_bases.append(metacls)
+                    attribute_is_defined = (attribute_is_defined or
+                                            metacls.has_readable_member(context.name))
+            if attribute_is_defined and context.name not in direct_lvalue_vars:


Why do you need to check for attribute_is_defined, to avoid this extra message if the type is from a __getattr__()? Please add a comment. Naively, if an attribute is not defined at all, we can't know its type.

Right, so this was to not bother with the work of traversing base classes if the attribute doesn't exist, since we presumably won't find it. It was a short circuit break.

ilevkivskyi · 2019-05-09T17:51:50Z

            # the type of accessing the attribute (even after the override).
-            rvalue_type = self.check_simple_assignment(get_type, rvalue, context)
+            rvalue_type = self.check_parent_member_assignment(instance_type, get_type,
+                                                              attribute_type, rvalue, context)


Why do you need to pass both attribute_type and get_type here? This looks wrong, I think you should only pass get_type.

On a second thought, it seems to me, we may not show this additional info if descriptor protocol is involved, it may only add confusion, because the definition a user will see in the base class, is the descriptor type. But this is not a strong opinion.

get_type (i.e. lvalue_type in the next method) is used only when it falls back on check_simple_assignment and get_type is what was used previously so I didn't change that.
I had considered not doing the fallback in check_parent_member_assignment, then it wouldn't need its lvalue_type arg at all, but it would have to return Optional[Type] and all callers would have to implement the fall back. I thought this was less desirable.

ilevkivskyi

Thanks for updates! This looks better, I still have few questions/comments.

ilevkivskyi · 2019-06-02T23:12:27Z

            # the type of accessing the attribute (even after the override).
-            rvalue_type = self.check_simple_assignment(get_type, rvalue, context)
+            rvalue_type = self.check_parent_member_assignment(instance_type, get_type,
+                                                              lvalue, attribute_type, rvalue)


Do you have a test case for a situation where __get__ is involved?

ilevkivskyi · 2019-06-02T23:13:07Z


        # Here we just infer the type, the result should be type-checked like a normal assignment.
-        # For this we use the rvalue as type context.
+        # For this we use the rvalue as type lvalue.


The mass replace context -> lvalue went wrong.

ilevkivskyi · 2019-06-02T23:20:52Z

+    def check_parent_member_assignment(self, lvalue_base_type: Type,
+                                       lvalue_type: Type, lvalue: MemberExpr,
+                                       rvalue_type: Type, rvalue: Expression) -> Type:
+        """Type inherited member assignment.


I can't parse this as English sentence, could you please reformulate?

ilevkivskyi · 2019-06-02T23:22:05Z

+        """Type inherited member assignment.
+
+        This defers to check_simple_assignment if the attribute is local to the instance_type,
+        otherwise it will inform the user where the attribute type was originally defined.


I would say explicitly that it does exactly the same as check_simple_assignment(), but adds better error message if possible.

ilevkivskyi · 2019-06-02T23:25:25Z

+        Return the inferred rvalue_type.
+        """
+        if (isinstance(lvalue_base_type, Instance) and
+                lvalue.name not in lvalue_base_type.type.names):


Could you invert this check and put the last return here? This will save one indent level for the whole function body.

ilevkivskyi · 2019-06-02T23:33:36Z

+                            lvalue_type, rvalue, lvalue,
+                            msg=message_registry.INCOMPATIBLE_TYPES_IN_ASSIGNMENT,
+                            lvalue_name=message_registry.METACLASS_MISMATCH.format(
+                                metaclass.type.defn.name))


This whole part is duplicated below. Can you try factoring it out?

ilevkivskyi · 2019-06-02T23:34:36Z

+                        message_registry.METACLASS_MISMATCH.format(
+                            metaclass.type.defn.name))
+                    lvalue_base_type = metaclass
+            if lvalue_base_type.has_readable_member(lvalue.name):


Is this just a performance optimization? If yes, I would remove it. It will not give much speed-up and the logic in this method is already hard to follow.

ilevkivskyi · 2019-06-02T23:35:33Z

+A()().x = "" # E: Incompatible types in assignment (expression has type "str", variable has type "int")
+A(False)().x = "" # E: Incompatible types in assignment (expression has type "str", variable has type "int")
+A()()().x = "" # E: Incompatible types in assignment (expression has type "str", variable has type "int")
+A(False)()().x = "" # E: Incompatible types in assignment (expression has type "str", variable has type "int")


Why do you need al these test cases? What do they test?

ilevkivskyi · 2019-06-02T23:35:46Z

+A(True)().b = False
+A(True)(None).b = True
+A(True)(None).b = 1 # E: Incompatible types in assignment (expression has type "int", variable has type "bool")
+A(True)()(False).b = "" # E: Incompatible types in assignment (expression has type "str", variable has type "bool")


Same question here.

ilevkivskyi · 2019-06-02T23:36:49Z

+[case testRecursiveCallable]
+from typing import Optional
+class A:
+    x = 0 # Type: int


You don't need to use type comments in tests.

This is not a type comment because of a typo (uppercase T)

It is not needed here

hauntsaninja · 2020-09-27T23:53:02Z

Closing this PR, since it's gotten stale and there are a number of merge conflicts. But you're most welcome to pick it back up :-)

ucodery added 3 commits May 6, 2019 10:44

Ignore virtualenv for commits and tests

9f12405

Show class that originally defined attribute in output

46cf534

If a subclass or its instance attempts to assign an incompatible type display the class that originally defined the type.

Merge branch 'master' of https://github.com/python/mypy into issue2500

4cb85e8

ilevkivskyi reviewed May 9, 2019

View reviewed changes

better naming in check_parent_member_assignment

2969caa

ilevkivskyi reviewed Jun 2, 2019

View reviewed changes

hauntsaninja closed this Sep 27, 2020

JukkaL mentioned this pull request Apr 22, 2023

For incompatible attribute assignment, show class that defines attribute #2500

Open

97littleleaf11 mentioned this pull request May 12, 2023

Show class that originally defined attribute in output #15230

Open

Uh oh!

Uh oh!

Conversation

ucodery commented May 8, 2019

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hauntsaninja commented Sep 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants