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

Prevent unused-import for if t.TYPE_CHECKING #6787

Merged
merged 1 commit into from Jun 6, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Jun 1, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

The check for typing.TYPE_CHECKING was not robust against importing typing as t. Nor against importing keyboarding as typing. This could lead to false positives for unused-import.

Follow-up: we have the following plethora of utils. Would be good to standardize and deprecate some for 2.15 (not a backport task, though):

  • is_typing_guard()
  • is_node_in_typing_guarded_import_block()
  • in_type_checking_block() <-- this one edited in this PR

Closes #3846

@jacobtylerwalls jacobtylerwalls added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer False Positive 🦟 A message is emitted but nothing is wrong with the code labels Jun 1, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.14.1 milestone Jun 1, 2022
@jacobtylerwalls
Copy link
Member Author

I had a lot of --no-verify'ing to do:

/Users/.../pylint/doc/whatsnew/2/2.1/summary.rst: #2323's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.7/summary.rst: #3836's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.13/summary.rst: #3077's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.13/summary.rst: #6028's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.14/summary.rst: #5072's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.14/summary.rst: #6087's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.14/summary.rst: #5502's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž
/Users/.../pylint/doc/whatsnew/2/2.14/summary.rst: #6488's description is not on one line, or does not respect the standard format πŸ€–πŸ‘Ž

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

coveralls commented Jun 1, 2022

Pull Request Test Coverage Report for Build 2447711665

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

Totals Coverage Status
Change from base Build 2447273033: 0.004%
Covered Lines: 16351
Relevant Lines: 17117

πŸ’› - Coveralls

@DanielNoord
Copy link
Collaborator

@jacobtylerwalls I'll keep my approval, but we're missing coverage for one line. Luckily, I think a test is pretty straightforward!

@github-actions

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls marked this pull request as draft June 1, 2022 16:27
@jacobtylerwalls
Copy link
Member Author

Thanks, primer!

@DanielNoord DanielNoord self-requested a review June 1, 2022 16:35
@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review June 1, 2022 16:38
@github-actions

This comment has been minimized.

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Jun 1, 2022

@DanielNoord that's a lot of false positives fixed in flask!


We still have some indeterminacy in pandas and sentry, though? Have you any thoughts on this?

I linted a relevant pandas file and saw no difference between astroid 2.11.4 and bleeding-edge, so I don't think that's it:

$ python3 -m pylint tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/indexes/multi/test_reshape.py
************* Module pandas.tests.indexes.multi.test_reshape
tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/indexes/multi/test_reshape.py:126:4: R0204: Redefinition of result type from pandas.core.indexes.base.Index to pandas.core.indexes.multi.MultiIndex (redefined-variable-type)
tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/indexes/multi/test_reshape.py:131:4: R0204: Redefinition of expected type from pandas.core.indexes.base.Index to pandas.core.indexes.multi.MultiIndex (redefined-variable-type)

------------------------------------------------------------------
Your code has been rated at 9.80/10 (previous run: 9.80/10, +0.00)

@DanielNoord
Copy link
Collaborator

It seems the output of redefined-variable-type is not deterministic.

See the first lines for pandas. That's the same line, but a different message.

@jacobtylerwalls
Copy link
Member Author

Relevant: pylint-dev/astroid#1107. Maybe these changes should go there. And not backport this but get in astroid2.12/pylint2.15? With the deprecations to get rid of the extra methods.

@jacobtylerwalls jacobtylerwalls marked this pull request as draft June 1, 2022 18:27
@DanielNoord
Copy link
Collaborator

Relevant: PyCQA/astroid#1107. Maybe these changes should go there. And not backport this but get in astroid2.12/pylint2.15? With the deprecations to get rid of the extra methods.

Ah I had been looking for that issue in pylint but wasn't able to find it.

Yeah, this seems like it might fit better upstream.

@jacobtylerwalls jacobtylerwalls modified the milestones: 2.14.1, 2.15.0 Jun 1, 2022
@jacobtylerwalls jacobtylerwalls added Needs astroid update Needs an astroid update (probably a release too) before being mergable Work in progress and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 1, 2022
@Pierre-Sassoulas
Copy link
Member

I'm looking into the changelog hook error. It's strange that the result is not the same depending on the environment. Could you add the verbose option and give me the result @jacobtylerwalls ?

@DanielNoord DanielNoord removed their request for review June 2, 2022 08:37
@jacobtylerwalls
Copy link
Member Author

>>> content
":Release: 2.1\n:Date: 2018-08-01\n\nSummary -- Release highlights\n=============================\n\n* This release mostly includes fixes for bugs found after the launch of 2.0.\n\nNew checkers\n============\n\n* A new check was added, ``misplaced-format-function``.\n\n  This message is emitted when pylint detects that a format function is called on non str object.\n  This can occur due to wrong placement of closing bracket, e.g\n\n  .. code-block:: python\n\n    print('value: {}').format(123) # bad\n\n    print('value: {}'.format(123)) # good\n\n\nOther Changes\n=============\n\n* ``try-except-raise`` check was demoted from an error to a warning, as part of issue #2323.\n\n* Correctly handle the new name of the Python implementation of the ``abc`` module.\n\n  In Python 3.7, the ``abc`` module has both a C implementation as well as a Python one,\n  but the Python implementation has a different file name that what ``pylint`` was expecting,\n  resulting in some checks getting confused.\n\n* Modules with ``__getattr__`` are exempted by default from ``no-member``\n\n  There's no easy way to figure out if a module has a particular member when\n  the said module uses ``__getattr__``, which is a new addition to Python 3.7.\n  Instead we assume the safe thing to do, in the same way we do for classes,\n  and skip those modules from checking.\n\n\n* ``invalid name`` is no longer triggered for function and attribute names longer\n  than 30 characters. The upper limit was removed completely.\n\n\n* Fix false-positive ``undefined-variable`` for self referential class name in lamdbas\n\n* ``no-else-return`` also specifies the type of the branch that is causing the error.\n\n* Fixed inconsistent behaviour for bad-continuation on first line of file.\n\n* Fixed a bug where ``pylint`` was not able to disable certain messages on the last line through\n  the global disable option.\n\n* ``pylint`` no longer emits ``useless-return`` when it finds a single statement that is the ``return`` itself\n\n  We still want to be explicit when a function is supposed to return\n  an optional value; even though ``pass`` could still work, it's not explicit\n  enough and the function might look like it's missing an implementation.\n\n* Fixed a bug where ``pylint`` was crashing when being unable to infer the value of an argument to ``next()``\n\n\n* ``pylint`` no longer emit ``not-an-iterable`` when dealing with async iterators.\n\n* ``pylint`` gained the ability to specify a default docstring type for when the check cannot guess the type\n\n  For this we added a ``--default-docstring-type`` command line option.\n"
>>> from re import Pattern
>>> 
>>> VALID_ISSUES_KEYWORDS = ["Refs", "Closes", "Follow-up in", "Fixes part of"]
>>> ISSUE_NUMBER_PATTERN = r"#\d{1,5}"
>>> VALID_ISSUE_NUMBER_PATTERN = r"\*[\S\s]*?" + ISSUE_NUMBER_PATTERN
>>> ISSUES_KEYWORDS = "|".join(VALID_ISSUES_KEYWORDS)
>>> PREFIX_CHANGELOG_PATTERN = (
...     rf"(\*\s[\S[\n ]+?]*\n\n\s\s({ISSUES_KEYWORDS})) (PyCQA/astroid)?"
... )
>>> VALID_CHANGELOG_PATTERN = PREFIX_CHANGELOG_PATTERN + ISSUE_NUMBER_PATTERN
>>> 
>>> ISSUE_NUMBER_COMPILED_PATTERN = re.compile(ISSUE_NUMBER_PATTERN)
>>> VALID_CHANGELOG_COMPILED_PATTERN: Pattern[str] = re.compile(VALID_CHANGELOG_PATTERN)
>>> VALID_ISSUE_NUMBER_COMPILED_PATTERN: Pattern[str] = re.compile(
...     VALID_ISSUE_NUMBER_PATTERN
... )
>>> VALID_CHANGELOG_COMPILED_PATTERN.findall(content)
[]

@jacobtylerwalls
Copy link
Member Author

(The only additional output in verbose mode was a bunch of LGTM)

@jacobtylerwalls
Copy link
Member Author

Yeah, this seems like it might fit better upstream.

Ah, nvm -- the util in astroid is already deprecated. I do think it's better in pylint. So this is ready for review again.

@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review June 6, 2022 02:18
@jacobtylerwalls jacobtylerwalls removed Work in progress Needs astroid update Needs an astroid update (probably a release too) before being mergable labels Jun 6, 2022
@jacobtylerwalls jacobtylerwalls modified the milestones: 2.15.0, 2.14.1 Jun 6, 2022
@jacobtylerwalls jacobtylerwalls added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Jun 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2022

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on flask:
The following messages are no longer emitted:

  1. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/blueprints.py#L13
  2. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/ctx.py#L15
  3. unused-import:
    Unused SessionMixin imported from sessions
    https://github.com/pallets/flask/blob/main/src/flask/ctx.py#L16
  4. unused-import:
    Unused Request imported from wrappers
    https://github.com/pallets/flask/blob/main/src/flask/ctx.py#L17
  5. unused-import:
    Unused typing_extensions imported as te
    https://github.com/pallets/flask/blob/main/src/flask/sessions.py#L15
  6. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/sessions.py#L16
  7. unused-import:
    Unused Request imported from wrappers
    https://github.com/pallets/flask/blob/main/src/flask/sessions.py#L17
  8. unused-import:
    Unused Response imported from wrappers
    https://github.com/pallets/flask/blob/main/src/flask/sessions.py#L17
  9. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/logging.py#L10
  10. unused-import:
    Unused typing_extensions imported as te
    https://github.com/pallets/flask/blob/main/src/flask/app.py#L69
  11. unused-import:
    Unused Blueprint imported from blueprints
    https://github.com/pallets/flask/blob/main/src/flask/app.py#L70
  12. unused-import:
    Unused FlaskClient imported from testing
    https://github.com/pallets/flask/blob/main/src/flask/app.py#L71
  13. unused-import:
    Unused FlaskCliRunner imported from testing
    https://github.com/pallets/flask/blob/main/src/flask/app.py#L72
  14. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/templating.py#L14
  15. unused-import:
    Unused Scaffold imported from scaffold
    https://github.com/pallets/flask/blob/main/src/flask/templating.py#L15
  16. unused-import:
    Unused Response imported from wrappers
    https://github.com/pallets/flask/blob/main/src/flask/scaffold.py#L24
  17. unused-import:
    Unused WSGIApplication imported from _typeshed.wsgi
    https://github.com/pallets/flask/blob/main/src/flask/typing.py#L5
  18. unused-import:
    Unused Headers imported from werkzeug.datastructures
    https://github.com/pallets/flask/blob/main/src/flask/typing.py#L6
  19. unused-import:
    Unused Response imported from werkzeug.wrappers
    https://github.com/pallets/flask/blob/main/src/flask/typing.py#L7
  20. unused-import:
    Unused Rule imported from werkzeug.routing
    https://github.com/pallets/flask/blob/main/src/flask/wrappers.py#L12
  21. unused-import:
    Unused Response imported from werkzeug.wrappers as BaseResponse
    https://github.com/pallets/flask/blob/main/src/flask/helpers.py#L23
  22. unused-import:
    Unused typing_extensions imported as te
    https://github.com/pallets/flask/blob/main/src/flask/helpers.py#L25
  23. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/globals.py#L8
  24. unused-import:
    Unused _AppCtxGlobals imported from ctx
    https://github.com/pallets/flask/blob/main/src/flask/globals.py#L9
  25. unused-import:
    Unused SessionMixin imported from sessions
    https://github.com/pallets/flask/blob/main/src/flask/globals.py#L10
  26. unused-import:
    Unused Request imported from wrappers
    https://github.com/pallets/flask/blob/main/src/flask/globals.py#L11
  27. unused-import:
    Unused TestResponse imported from werkzeug.test
    https://github.com/pallets/flask/blob/main/src/flask/testing.py#L18
  28. unused-import:
    Unused Flask imported from app
    https://github.com/pallets/flask/blob/main/src/flask/testing.py#L20

This comment was generated for commit b7d14fb

@jacobtylerwalls jacobtylerwalls merged commit 107a891 into pylint-dev:main Jun 6, 2022
@jacobtylerwalls jacobtylerwalls deleted the robust-typing-id branch June 6, 2022 13:09
@DanielNoord DanielNoord added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unused-imports uses hardcoded list for typing.TYPE_CHECKING
4 participants