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

Add support for pep585 with postponed evaluation #4059

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Add support for pep585 with postponed evaluation #4059

merged 1 commit into from
Feb 15, 2021

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 2, 2021

Steps

  • 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.

Description

With PEP 585 and postponed evaluation of annotations it's possible to use list[int] and others even in versions 3.7 and 3.8 although PEP 585 was implemented in 3.9.

Currently, running pylint in a 3.7 or 3.8 environment always returns unsubscriptable-object errors. However the following example is valid

from __future__ import annotations

var: list[int]

def func(arg: dict[str, int]) -> list[int]:
  ...

The error is correct only if the type is evaluated instantly as for example with type aliases

from __future__ import annoations

InvalidAlias = list[int]

The latest mypy version 0.800 just added support for PEP 585 so pylint is the only thing holding it back for many.

--
Since I don't have much experience developing for pylint, some guidance would be much appreciated.
There is probably a better way to do this 😄

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

Closes: #3320

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.

Thank you for the MR, very clean. This change look reasonable, would it be possible maybe to check that from __future__ import annotations is present if we're under python 3.9 ? Ie. The same functional file without from __future__ import annotations would raise Error for python under 3.9 ? Also sorry for the master branch failing that also makes your CI fail, no idea how that happened, maybe a flaky test.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 2, 2021

@Pierre-Sassoulas Thanks for the quick review. Regarding your comments

would it be possible maybe to check that from future import annotations is present if we're under python 3.9 ? Ie. The same functional file without from future import annotations would raise Error for python under 3.9 ?

I believe it already does. I've added if is_postponed_evaluation_enabled(node) to supports_getitem which should already check for it. I can a a new test case though, just to make sure.

https://github.com/PyCQA/pylint/blob/1f34e3194bcfa281b9eb4b0595ec654b782405ee/pylint/checkers/utils.py#L1325-L1331

Also sorry for the master branch failing that also makes your CI fail, no idea how that happened, maybe a flaky test.

I noticed that the master branch is now passing, I will rebase the MR. Maybe that solves it.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 2, 2021

Also sorry for the master branch failing that also makes your CI fail, no idea how that happened, maybe a flaky test.

I noticed that the master branch is now passing, I will rebase the MR. Maybe that solves it.

Didn't work. I was already using the latest master. Maybe it will work for the next commit.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 2, 2021

I believe it already does. I've added if is_postponed_evaluation_enabled(node) to supports_getitem which should already check for it. I can a a new test case though, just to make sure.

It worked, as expected. I've included the checks in my last commit.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 2, 2021

Sorry for the many updates, the MR should be ready now.

@Pierre-Sassoulas
Copy link
Member

Thanks a lot ! I'll merge this as soon as the build is fixed. Apparently the problem come from the current astroid master branch. Can you please allow edit from maintainer so I can rebase your branch myself once it's fixed ? :)

@cdce8p
Copy link
Member Author

cdce8p commented Feb 3, 2021

Can you please allow edit from maintainer so I can rebase your branch myself once it's fixed ? :)

@Pierre-Sassoulas Already checked. If it doesn't work for whatever reason, just let me know. I usually can get back to it within a day.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 3, 2021

@Pierre-Sassoulas I saw that there are plans to do a new release soon pylint-dev/astroid#886 (comment).
Can you add the 2.6.1 milestone, just so it will be included?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.6.1 release milestone Feb 3, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component python 3.9 labels Feb 3, 2021
ChangeLog Outdated Show resolved Hide resolved
@cdce8p cdce8p changed the title Add support for pep585 with postponed evaulation Add support for pep585 with postponed evaluation Feb 4, 2021
@cdce8p
Copy link
Member Author

cdce8p commented Feb 4, 2021

@Pierre-Sassoulas I noticed today that I was missing checks for TypedDict, NamedTuple and dataclass. Can you take a look at it again, since adding those required a bigger code change?

@cdce8p
Copy link
Member Author

cdce8p commented Feb 4, 2021

Sorry for the back and forth, I find it quite difficult to get it right with checking so many different things which aren't really documented anywhere. However I think I got it now.

Turns out my last code change was wrong, with from __future__ import annotations TypeDict, NamedTuple and Dataclasses work mostly as expected. In particular the following does indeed work and is not a TypeError:

# Valid in Python 3.7/3.8 (TypedDict)
from __future__ import annotations

class CustomNamedTuple(NamedTuple):
    my_var: list[int]

class CustomTypedDict(TypedDict):
    my_var: list[int]

@dataclass
class CustomDataClass:
    my_var: list[int]

What doesn't work and I guess caused my confusion is:

CustomNamedTuple = typing.NamedTuple(
    "CustomNamedTuple", [("my_var", list[int])])  # [unsubscriptable-object]

CustomTypedDict = TypedDict("CustomTypedDict", my_var=list[int])  # [unsubscriptable-object]

CustomTypedDict2 = TypedDict("CustomTypedDict2", {"my_var": list[int]})  # [unsubscriptable-object]

Just to be sure I also checked it with Pylance / pyright, but, as it turns out, even it doesn't recognize all cases correctly so I also did some manual checks again.

@Pierre-Sassoulas
Copy link
Member

No worry, mypy 0.8 has only been released 2 weeks ago, this is cutting edge statistical analysis on multiple python interpreters after all ;)

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

Great PR, thank you @cdce8p !

pylint/checkers/utils.py Show resolved Hide resolved
pylint/checkers/utils.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 90.563% when pulling d877dd0 on cdce8p:pep585-postponed-evaluation into 005cf2f on PyCQA:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 90.563% when pulling d877dd0 on cdce8p:pep585-postponed-evaluation into 005cf2f on PyCQA:master.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 15, 2021

@Pierre-Sassoulas This one is ready to be merged.

@Pierre-Sassoulas Pierre-Sassoulas merged commit b6edb60 into pylint-dev:master Feb 15, 2021
@cdce8p cdce8p deleted the pep585-postponed-evaluation branch February 15, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component python 3.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pylint does not conform to PEP 585 [generics parametrisation]
5 participants