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.

Sign up for GitHub

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

Jump to bottom

VariableChecker now accounts for attribute lookups in type comments #4604

Merged
merged 2 commits into from
Jun 27, 2021
Merged

VariableChecker now accounts for attribute lookups in type comments #4604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7766e61
VariableChecker now accounts for attribute lookups in type comments
superbobry Jun 22, 2021
751cf4c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,3 +503,5 @@ contributors:
* Markus Siebenhaar: contributor

* Lorena Buciu (lorena-b): contributor

* Sergei Lebedev (superbobry): contributor
5 changes: 5 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,11 @@ Closes #4555

Closes #4023

* Fix ``unused-import`` false positive for imported modules referenced in
attribute lookups in type comments.

Closes #4603


What's New in Pylint 2.8.3?
===========================
Expand Down
4 changes: 4 additions & 0 deletions pylint/checkers/variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1826,6 +1826,10 @@ def _store_type_annotation_node(self, type_annotation):
self._type_annotation_names.append(type_annotation.name)
return

if isinstance(type_annotation, astroid.Attribute):
self._store_type_annotation_node(type_annotation.expr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could change this to a more restrictive form

if isinstance(type_annotation, astroid.Attribute):
    if not isinstance(type_anntoation.expr, astroid.Name):
         return
    self._type_annotation_names.append(type_annotation.expr.name)

Copy link
Member

Choose a reason for hiding this comment

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

Hello, could you maybe add a use case so it's easier to understand what this would prevent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current check covers all of

# type: foo.Bar
# type: foo.bar.Boo
# type: Foo[T].Bar
# type: Foo[S, T].Bar

The restricted check only allows # type: foo.Bar. I personally prefer the current check to the restricted one, but I don't feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation :) I also prefer handling all the use cases. Would it be possible to add those 4 examples in the functional tests in tests/functional ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added them to variables_test.py. Would you prefer me also add them to tests/functional?

Copy link
Member

Choose a reason for hiding this comment

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

I like functional because there's no tests code around it, only the code and the expected output, but as long as there is a test it's ok :)

return

if not isinstance(type_annotation, astroid.Subscript):
return

Expand Down
2 changes: 2 additions & 0 deletions pylint/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/master/LICENSE

import platform
import sys

import astroid
Expand All @@ -11,6 +12,7 @@
PY39_PLUS = sys.version_info[:2] >= (3, 9)
PY310_PLUS = sys.version_info[:2] >= (3, 10)

IS_PYPY = platform.python_implementation() == "PyPy"

PY_EXTS = (".py", ".pyc", ".pyo", ".pyw", ".so", ".dll")

Expand Down
20 changes: 20 additions & 0 deletions tests/checkers/unittest_variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@
import os
import re
import sys
import unittest
from pathlib import Path

import astroid

from pylint.checkers import variables
from pylint.constants import IS_PYPY
from pylint.interfaces import UNDEFINED
from pylint.testutils import CheckerTestCase, Message, linter, set_config

Expand Down Expand Up @@ -191,6 +193,24 @@ def my_method(self) -> MyType:
with self.assertNoMessages():
self.walk(module)

@unittest.skipIf(IS_PYPY, "PyPy does not parse type comments")
def test_attribute_in_type_comment(self):
"""Ensure attribute lookups in type comments are accounted for.

https://github.com/PyCQA/pylint/issues/4603
"""
module = astroid.parse(
"""
import foo
from foo import Bar, Boo
a = ... # type: foo.Bar
b = ... # type: foo.Bar[Boo]
c = ... # type: Bar.Boo
"""
)
with self.assertNoMessages():
self.walk(module)


class TestVariablesCheckerWithTearDown(CheckerTestCase):

Expand Down