Add inference tips for dataclass attributes

[david-yz-liu]

Steps

• For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• Write a good description on what the PR does.

Description

I'm a little sleepy 😴 so I'm going to write up a short version of the description and elaborate later (perhaps upon request only). The short version is I've added two new inference tips related to dataclasses, in astroid/brain/brain_dataclasses.py:

1. infer_dataclass_field_call, for handling calls to field to infer the default value they actually evaluate to.

2. infer_dataclass_attribute, for handling inference of dataclass instance attributes. Note: the previous transform modified the dataclass ClassDef node's locals dict:

   https://github.com/PyCQA/astroid/blob/4704ca75539c085cdd0476a8d79771a7d2b8740d/astroid/brain/brain_dataclasses.py#L58-L64

   I thought that was a bit too extreme, as it removed possible inferences that could be done for class attributes. So I removed the update to locals, and so the class variable inference "just works", without using this inference tip. See the tests I added for what I mean.

   ⚠️ In order to get the inference tip working for Unknown nodes, I had to make a few changes to the underlying node infrastructure, namely Unknown.infer and AstroidManager.visit. I'm pretty sure both of these changes are fine, but please let me know if that's an issue.

Using type annotations (?)

For the instance attribute inference, I used the type annotation to provide an alternative to the default value. E.g., x: int = 10 gets two inference results, one for Const(10) and one for Instance of type int. My sense looking at the issues for astroid/pylint is that using type annotations for inference is a possibility but hasn't really been explored. So this PR has one small way foray into using type annotations for inference, but let me know if you want to avoid that. I could yield an Uninferable instead.

Other changes

In addition to these two changes, I updated the existing inference tip (dataclass_transform) to be more robust in checking for dataclass usage (including renaming of the dataclass import), and added basic support for InitVar as well.

Also, Assign nodes shouldn't be included, only AnnAssign nodes. For dataclasses, the "magic" only occurs for class variables with type annotations.

Testing

As usual, I've tried to communicate various intentions through some tests. See also related pylint issues below.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#2600
Closes pylint-dev/pylint#2698
Closes pylint-dev/pylint#3405
Closes pylint-dev/pylint#3794

I've written "closes" even though I couldn't reproduce some of them, but I was going a bit quickly and I'd appreciate a second look.

For the field-specific ones, the commit (3e4eac7) that introduced the transform should have fixed the false positive no-member complaints because Unknown infers to Uninferable, which I think causes pylint to not emit no-member.

However, with the change in this PR, a more specific type is emitted, meaning that the false positives described in the issues should not occur, but there should be fewer false negatives too. E.g.,

from typing import Dict
from dataclasses import dataclass, field

@dataclass
class TestClass:
	foo: str
	bar: str
	baz_dict: Dict[str, str] = field(default_factory=dict)

	def some_func(self) -> None:
		for key, value in self.baz_dict.itemss():  # [no-member] typo here, "itemss" should be "items"
			print(key)
			print(value)

So IMO it'd be good to add regression tests for these issues, even if they aren't reproducible on main. Let me know if you want me to do it.

[Pierre-Sassoulas]

Thanks for the great description :D I wonder what it would be like if you were awake and it was much detailed ;)

I looked at the failing tests and it seems like what is failing is failing because it's now better than before (better inference ?). I wonder if the name node should be "Unknown" though.

Regarding the regression tests in pylint, let's add them on top of pylint-dev/pylint#4816, you should have the right to do it but if you don't or if you don't have the time, I'll do it :)

[cdce8p]

Using type annotations for inference

I'm still a bit hesitant about that. As python is a dynamic language, nothing really prevents anyone to assign anything they want. Furthermore, not all types can correctly be inferred currently. That especially applies to TypeVars and Generic types, but there might be others as well.

[david-yz-liu]

@Pierre-Sassoulas thanks, I fixed up the tests. Version 3.7 needed a different #@ marker to access the ClassDef node with a @dataclass decorator, and inferring ClassVar didn't work on Python 3.8. I can add some tests to the Pylint branch in a bit.

@cdce8p I think those are totally fair points. Personally I think it's still worthwhile to try to infer at least some basic types, but deciding this can easily be deferred to later. For now, I can change this part of the code to just yield Uninferable, if that works for you? I think this is the same inference as when inferring a function parameter with a default value: both the default value and Uninferable are yielded.

[david-yz-liu]

P.S. sorry about the style commits, I hadn't set up the pre-commit hooks in this repo. I can rebase and clean up once the branch is finalized.

[Pierre-Sassoulas]

I'm still a bit hesitant about that. As python is a dynamic language, nothing really prevents anyone to assign anything they want.

I'm more and more convinced that we must trust what the user is using in the typing. In conjunction with mypy that will tell the user about problem with it's own typing it should make pylint a lot more efficient. There's a discussion about this subject here based on a comment by @PCManticore: pylint-dev/pylint#4813

[david-yz-liu]

@Pierre-Sassoulas I added tests the branch but I didn't have write access to your repository, but I pushed to my own branch: https://github.com/david-yz-liu/pylint/tree/regression-test-533-2224-2626. I wouldn't think I have edit permissions on MRs to pylint, but maybe I just messed something up, please let me know.

The tests I added are all consistent with yielding Uninferable for instance attributes, as I suggested in my previous comment. But if you guys end up going with the proposed change as is, I'll add a couple more "false negative" tests to illustrate the stronger inference. I'll also add some tests for simple Generics like List[str], I neglected to do so in my initial PR.

Let me know which direction on the inference you'd like to go with, either way is good with me 😄

[Pierre-Sassoulas]

Hey @david-yz-liu thank you for creating all those tests in pylint. Maybe you had to be a "maintener" in order to modify the branch and "contributor" is not enough, my bad.

Regarding this change I think the way it's done right now is fine. Using typing when the inference fails us is something that we've already done in another MR if I remember correctly. I think using typing all the time even before inference for performance purpose needs to be discussed between maintainers but this is not what is done here. What do you think @hippo91 , @cdce8p ?

[cdce8p]

@Pierre-Sassoulas I understand that you would like to use typing for inference. As mentioned earlier, I'm still not convinced that's the way to go, but then again inference is not really a topic of expertise for me. Let's put it differently, as long as it doesn't cause major regressions or other unforeseen issues, go ahead with it.

[cdce8p]

I haven't been able to narrow it down just yet, but this change causes a regression for Home-Assistant

************* Module homeassistant.components.melcloud.sensor
Error: homeassistant/components/melcloud/sensor.py:118:11: E1102: description.enabled is not callable (not-callable)
Error: homeassistant/components/melcloud/sensor.py:123:11: E1102: description.enabled is not callable (not-callable)
Error: homeassistant/components/melcloud/sensor.py:131:15: E1102: description.enabled is not callable (not-callable)
************* Module homeassistant.components.influxdb.sensor
Error: homeassistant/components/influxdb/sensor.py:175:61: E1102: influx.close is not callable (not-callable)
************* Module homeassistant.components.influxdb
Error: homeassistant/components/influxdb/__init__.py:489:8: E1102: influx.close is not callable (not-callable)

https://github.com/cdce8p/ha-core/tree/f684e4df349f7b95da473a1003f708ac67284848

[cdce8p]

Opened #1129 with a short code snipped that is enough to reproduce the issue.