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

Fix a crash when inferring a typing.TypeVar call. #2239

Merged

Conversation

mbyrnepr2
Copy link
Member

Closes pylint-dev/pylint#8802

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
βœ“ πŸ”¨ Refactoring
πŸ“œ Docs

Description

Closes pylint-dev/pylint#8802

Copy link
Member Author

@mbyrnepr2 mbyrnepr2 Jul 6, 2023

Choose a reason for hiding this comment

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

Created this module since it felt like all brain_typing tests were going into brain/test_brain.py - let me know what you think.

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #2239 (ea5dd00) into main (cdeb259) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2239   +/-   ##
=======================================
  Coverage   92.80%   92.80%           
=======================================
  Files          94       94           
  Lines       10941    10941           
=======================================
  Hits        10154    10154           
  Misses        787      787           
Flag Coverage Ξ”
linux 92.61% <100.00%> (ΓΈ)
pypy 90.89% <100.00%> (ΓΈ)
windows 92.37% <100.00%> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/brain/brain_typing.py 86.66% <100.00%> (ΓΈ)

@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review July 6, 2023 14:53
@jacobtylerwalls jacobtylerwalls added this to the 2.15.6 milestone Jul 6, 2023
@jacobtylerwalls
Copy link
Member

Let's backport. Shall we then move the changelog to 2.15.6?

@@ -134,7 +134,14 @@ def infer_typing_typevar_or_newtype(node, context_itton=None):
raise UseInferenceDefault

typename = node.args[0].as_string().strip("'")
node = extract_node(TYPING_TYPE_TEMPLATE.format(typename))
node = ClassDef(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we add the Meta class as base? Then we still have the __getitem__ inference correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for such quick feedbackπŸ‘.
I started a short vacation right after opening this so I’ll think about a test case when I can.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @DanielNoord I've taken a look finally :)

It seems to me there is no need for __getitem__ to be attached to the astroid node representing either typing.TypeVar or typing.NewType since neither of these are subscriptable. What do you think?
Similar point for __args__ since that is used to fetch the arguments used in subscripting.

As a sidenote - __getitem__ is already part of nodes.ClassDef since it inherits that method from LocalsDictNodeNG.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay! Can we then remove that code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took a look into that just now. The template is used by this function which needs the __args__ member since the function is inferring subscriptable types.
As an illustration, If I were to remove the code and use a similar ClassDef construction for the code above, this unitest fails:
FAILED astroid/tests/brain/test_brain.py::TypingBrain::test_has_dunder_args - astroid.exceptions.InferenceError: StopIteration raised without any error information.
In other words, running Pylint on this code:

from typing import Union
NumericTypes = Union[int, float]
NumericTypes.__args__ #@

...gives: E1101: Class 'Union' has no '__args__' member (no-member)

ChangeLog Outdated Show resolved Hide resolved
@mbyrnepr2 mbyrnepr2 force-pushed the pylint_issue_8802_typing_typevar_crash branch from e845e19 to 017c218 Compare July 13, 2023 13:01
@mbyrnepr2 mbyrnepr2 force-pushed the pylint_issue_8802_typing_typevar_crash branch 2 times, most recently from f8562cb to 5b671d3 Compare July 14, 2023 11:18
@mbyrnepr2 mbyrnepr2 force-pushed the pylint_issue_8802_typing_typevar_crash branch from 5b671d3 to ea5dd00 Compare July 16, 2023 12:03
@jacobtylerwalls jacobtylerwalls removed their request for review July 16, 2023 16:23
@mbyrnepr2 mbyrnepr2 merged commit 89dfb48 into pylint-dev:main Jul 17, 2023
@mbyrnepr2 mbyrnepr2 deleted the pylint_issue_8802_typing_typevar_crash branch July 17, 2023 10:39
github-actions bot pushed a commit that referenced this pull request Jul 17, 2023
Pierre-Sassoulas pushed a commit that referenced this pull request Jul 17, 2023
…peVar` call. (#2254)

* Fix a crash when inferring a `typing.TypeVar` call. (#2239)

Closes pylint-dev/pylint#8802

(cherry picked from commit 89dfb48)

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
mbyrnepr2 added a commit to mbyrnepr2/astroid that referenced this pull request Sep 25, 2023
@Pierre-Sassoulas Pierre-Sassoulas added backported Assigned once the backport is done and removed backport maintenance/2.15.x labels Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Assigned once the backport is done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pylint crashes with an invalid TypeVar definition
4 participants