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

Restrict type inference of dataclass attributes. #1130

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

david-yz-liu
Copy link
Contributor

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

The change in #1126 used type annotations to infer Instance values for instance attributes of dataclasses:

https://github.com/PyCQA/astroid/blob/f5f1125e31b85f2c2002189c8ad95a691fd56c83/astroid/brain/brain_dataclasses.py#L82-L111

This works well for most types, but type annotations from typing are different: they don't exactly represent new classes themselves, but really are "hints" about what the type of a variable should be. But the current code isn't aware of this, and so creates Instances of these type annotation classes.

For generic collection types like typing.List, this isn't an issue, since astroid proxies these to the corresponding built-in collection (like list), see astroid/brain/brain_typing.py (I think, haven't looked into the details). But for other types this is an issue, since 1-to-1 proxying doesn't always make sense.

In #1129, an attribute is inferred to be an Instance of typing.Callable, but that class doesn't have a __call__ method, leading to a not-callable pylint error. This caused a regression because the previously-inferred value, Uninferable, was more permissive and caused pylint to skip these kinds of typechecks.

So for now I've created an allowed list of typing types for which astroid support inference, and currently included only the generic collection types. All other type annotations from typing will revert to yielding Uninferable.

You'll also note that I've pulled out the type annotation inference code into a separate protected function, but left it in brain_dataclasses.py. It could be made public, extended, and used more generically, but... I didn't want to rock the boat, so leaving that discussion to later. 😅

More testing

On the pylint side, all of the typecheck checkers should include a new cases for dataclass attributes, which are using this new approach. Hopefully to prevent new regressions like #1129 in the future. I can work on this later, but I think I'm going to take some time off astroid/pylint after this immediate issue gets resolved.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #1129

For now, from the typing module only generic collection types are inferred:
Dict, FrozenSet, List, Set, Tuple. Astroid proxies these to the built-in
collection types (e.g., dict).

Other type annotations from typing like Callable and Union yield Uninferable;
these would need to be handled on a case by case basis.
@cdce8p
Copy link
Member

cdce8p commented Aug 16, 2021

@david-yz-liu Thanks for opening the PR! Tested it with Home-Assistant and indeed the issue seems to be fixed. Interestingly enough there now appears to be a regression with the enum code. In particular regarding your recent PR #1121.
That might be captured in the one pylint test (in main) that is also failing: arguments_renamed:

# pytest tests/test_functional.py -k arguments_renamed

E       AssertionError: Wrong results for file "arguments_renamed":
E       
E       Unexpected in testdata:
E         20: no-value-for-parameter

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.7.1 milestone Aug 16, 2021
@david-yz-liu
Copy link
Contributor Author

@cdce8p glad it worked on Home-Assistant (awesome project). The test you mentioned is fixed in pylint-dev/pylint#4816, which hopefully will be merged in soon 😄

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm this is working in pylint in current pylint :) Thank you @david-yz-liu !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 2129907 into pylint-dev:main Aug 16, 2021
@david-yz-liu david-yz-liu deleted the issue-1129 branch August 16, 2021 17:36
@cdce8p
Copy link
Member

cdce8p commented Aug 16, 2021

@cdce8p glad it worked on Home-Assistant (awesome project).

❤️ Yeah, indeed. I tend to use it for some real world testing before we merge changes. They have a bigger code base and I know some parts which helps a bit. That hopefully makes pylint and astroid more stable.

The issues are indeed all fixed now. Thank you for working on it so quickly! 🐬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression with dataclass inference
3 participants