Make self-cls-assignment consider tuple assign. & ignore typing.cast()

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Found while working on pylint-dev/astroid#1217

I don't think typing.cast() should raise this warning. While I was at it I thought I might as well fix this checker for tuple assignments as well.

I would like for this to go in 2.12 so we can get pylint-dev/astroid#1217 in astroid 2.8.5 and thereby in 2.12.1.

[coveralls]

Pull Request Test Coverage Report for Build 1417458494

• 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.005%) to 93.341%

---

Totals
Change from base Build 1415912636:	0.005%
Covered Lines:	13750
Relevant Lines:	14731

---

💛 - Coveralls

[Pierre-Sassoulas]

LGTM ! I only have a small question regarding the test case.

[Pierre-Sassoulas]

Why would we cast to TestTypingCast here ? Wouldn't it be used in a more complex context ?

[DanielNoord]

I just used self's class again because a similar example caused me to make this change. We can change it to something else, perhaps even a more generic Type if you want!

[DanielNoord]

@cdce8p Said that he thought the this message was actually correct for typing.cast() in pylint-dev/astroid#1217 so I think he will want to look at this as well before merging!

[cdce8p]

I would suggest not to special case cast. pylint-dev/astroid#1217 has already been fixed without it. IMO if you need to reassign self with cast you are probably doing something wrong.

It's much better to return the result directly, as now done in the astroid PR.

def func(self) -> SomeType:
    return cast(SomeType, self)

[DanielNoord]

I agree that you're probably doing something wrong when this is necessary. In our case self.is_statement is another way of doing isinstance(self, nodes.Statement).

However, I can create a contrived example where we would need to use the cast for mypy to work. Look at:

class BaseClass:
    def __init__(self):
        self.advanced = False

    def advanced_attribute(self) -> str:
        if self.advanced:
            # Will need to cast here to make this work for mypy
            attr_type = self.get_attribute_type()
            if attr_type == str:
                return "string"
            elif attr_type == int:
                return "number"
        else:
            return ""


class SimpleClass(BaseClass):
    pass


class AdvancedClass(BaseClass):
    def __init__(self, attribute):
        super().__init__()
        self.advanced = True
        self.attribute = attribute

    def get_attribute_type(self):
        return type(self.attribute)


print(AdvancedClass("string").advanced_attribute())
print(AdvancedClass(1).advanced_attribute())

So, based on the fact that a class's attribute has a certain value (which is only true for certain child classes) we know it has a certain method and we need to do something with that method that disallows immediately returning the cast.
Using such logic (attribute has a value -> method exists) should probably be avoided, but is that a reason to raise a message on a valid re-assign to self? The warning (seems to) intend to warn for changing the first parameter of a method, which typing.cast can't do.

[cdce8p]

I agree that you're probably doing something wrong when this is necessary. In our case self.is_statement is another way of doing isinstance(self, nodes.Statement).

However, I can create a contrived example where we would need to use the cast for mypy to work.
...

You don't need to use cast there. Although I admit that it would certainly be easier. But IMO that's where pylint: disable can be used. The key in your example is to annotate self with a Protocol class.

from typing import Any, Generic, Literal, Protocol, TypeVar

T = TypeVar("T")


class HasAdvancedAttribute(Protocol[T]):
    advanced: Literal[True] = True
    attribute: T

    def get_attribute_type(self) -> type[T]:
        ...


class BaseClass:
    def __init__(self) -> None:
        self.advanced = False

    def advanced_attribute(self: HasAdvancedAttribute[Any]) -> str:
        if self.advanced:
            attr_type = self.get_attribute_type()
            if attr_type == str:
                return "string"
            if attr_type == int:
                return "number"
        return ""


class SimpleClass(BaseClass):
    pass


class AdvancedClass(BaseClass, Generic[T]):
    def __init__(self, attribute: T) -> None:
        super().__init__()
        self.advanced: Literal[True] = True
        self.attribute: T = attribute

    def get_attribute_type(self) -> type[T]:
        return type(self.attribute)


print(AdvancedClass("string").advanced_attribute())
print(AdvancedClass(1).advanced_attribute())

[DanielNoord]

Your example would indeed remove the need for typing.cast but wouldn't it still be in the spirit of this checker to ignore it? As far as I know it doesn't actually change the value it is assigning to, so wouldn't that remove the need for the current message?

[cdce8p]

Your example would indeed remove the need for typing.cast but wouldn't it still be in the spirit of this checker to ignore it? As far as I know it doesn't actually change the value it is assigning to, so wouldn't that remove the need for the current message?

Technically yet, but I wonder how common it really is that we need to special case it. Keep in mind, we start with typing.cast and before long we get requests to allow users to add their own function names. At some point it's just not worth it. I searched for self-cls-assignment in Home Assistant to see if it's disabled somewhere but couldn't find anything.

In the end, I still believe the warning provides valuable feedback. A dev should really know what he/she is doing to assign the return of cast to self and in those cases adding pylint: disable is a good alternative.

[DanielNoord]

Fair enough! I'll remove the special casing and leave the handling of tuple assignments in as it is a nice enhancement of the checker.

[cdce8p]

@DanielNoord I wondering, can you open a new PR for it? This discussion here has been quite extensive so it might be worth preserving it.

[DanielNoord]

Will do!