Fix #1555: Fix false negatives for no-member from self-referencing assignments

[jacobtylerwalls]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Emits no-member when attempting to assign an instance attribute to itself before it has been defined, since at the moment of attempted assignment, there is no such member.

Closes #1555

[coveralls]

Pull Request Test Coverage Report for Build 2037093064

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

---

Totals
Change from base Build 2036902764:	0.003%
Covered Lines:	15297
Relevant Lines:	16244

---

💛 - Coveralls

[Pierre-Sassoulas]

LGTM, thank you :)

[Pierre-Sassoulas]

LGTM, but there's something strange with the coverage report : 86 unchanged lines in 4 files lost coverage., do you know why ?

[DanielNoord]

@jacobtylerwalls Thanks for your work again! Looks like you found some additional cases as well. Should we add a test for dataclasses? If we change the comprehension to a for loop that if statement would become uncovered as well (I think).

@Pierre-Sassoulas coveralls has been having problems recently. I noticed these comments as well, even on trivial changes that touched almost no code. The comment might not be correct.

[jacobtylerwalls]

Should we add a test for dataclasses?

That line resulted from fixing failing tests in no_member_dataclasses.py, so I think we're good on test coverage there.

even on trivial changes that touched almost no code.

See for example #5540.

[DanielNoord]

@jacobtylerwalls Do you want to do the exception handler move in this PR? Or do it in a later PR?

Edit: I missed your earlier reply. Waiting is fine for me. You make a very good point 😄

We should also wait for a resolution on the astroid utils location discussion.

[DanielNoord]

I'm removing this from 2.13 as it is blocked by discussion in astroid. Namely: pylint-dev/astroid#1306 and subsequently whether we wan't to create a special Instance for dataclass. If we did we wouldn't need the astroid util as we can just do isinstance(node, bases.Dataclass).
Also adding requires Astroid update

[DanielNoord]

Possible fix for the problems identified here in pylint-dev/astroid#1391

[DanielNoord]

@jacobtylerwalls dev of astroid now has is_dataclass on all ClassDef nodes. You could change the code in this PR to use it so that when we release 2.10 it's just a matter of upgrading the dependency and rebasing.
We might be able to get this in 2.13 then as welL!

[jacobtylerwalls]

Great, thank you for doing the astroid PR!

[jacobtylerwalls]

@DanielNoord I made the changes, expecting CI to fail, and was greeted by a very unwanted ✅ .

What happened?
This PR makes no changes to the silencing of AttributeError, which is why this didn't fail by depending on the unreleased astroid changes. We discussed cleaning up those excepts here. At this point I'm inclined to clean them up in this PR, since I am learning about the edge cases after all.

So with that cleaned up, this would crash again before updating astroid (good), but I was still expecting more substantive failures. At first, it looked like I didn't need the newfangled is_dataclass attribute, because checking isinstance(attr_parent.parent, nodes.ClassDef) was enough. But why? Well, with non-dataclasses, these blocks are short-circuited[1], and jump to _emit_no_member(), which silences the warning with this explanation:

https://github.com/PyCQA/pylint/blob/b31dd5623966069386b9df3ccc08e2834aca621d/pylint/checkers/typecheck.py#L515-L518

This goes against what you and I discussed here, so it's worth a double-check.

Question: Do you think we should move forward with emitting no-member for possibly never-assigned type annotations? (Currently undefined-variable is not raised by the changes in #5158, because they are in different scopes.) Or reduce the scope of this PR back to just dealing with self-referencing assignments?

class PossibleNoMember:
    def __init__(self):
        self.member: bool

    def set_member(member):
        self.member = member

    def depend_on_member():
        print(self.member)  # should this raise no-member? it will not raise undefined-variable

[1] -- These patterns are very intricate -- so after cleaning up the excepts we may be able to write a test case that will require using is_dataclass after all.

[jacobtylerwalls]

I have a feeling the answer is going to be "create possibly-no-member".

[jacobtylerwalls]

Well, from the blame there's a good argument to leave these alone, see discussion at #2945 and #3167.

[DanielNoord]

#4194 is interesting as well as it fixes #3167, but there is no discussion about this potentially being undefined.

I'm not sure. I think we actually use this pattern ourself:
We create a PyLinter() with some attributes typed but not assigned.
We call some methods (but not the __init__) that set the unassigned attributes.
We call some methods that use the previously unassigned attributes.

Personally I feel that that is worth a message, although as you said perhaps it should be possibly-no-member. This pattern is very error-prone and only because I started to type the PyLinter class these potential issues arose. Warning users about this, and having them think about not assigning attributes in the __init__ is a good thing imo. However, this is probably used a little too often to suddenly start emitting no-member warnings everywhere...
Perhaps it would indeed be best to re-rescope this PR back to the original intent and create a new issue in which we discuss adding possibly-no-member? We could make 2.14 the possible-... update 😄

[Pierre-Sassoulas]

Need a rebase and moving the changelog to 2.14