VariableChecker now accounts for attribute lookups in type comments

[superbobry]

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

Prior to this commit VariableChecker did not recurse into attribute lookups in type comments. This lead to false positive unused-import messages in e.g.

import collections
d = ...  # type: collections.OrderedDict

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Fixes #4603.

[superbobry]

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)

[Pierre-Sassoulas]

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

[superbobry]

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.

[Pierre-Sassoulas]

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 ?

[superbobry]

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

[Pierre-Sassoulas]

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 :)

[coveralls]

Coverage increased (+0.003%) to 92.039% when pulling 751cf4c on superbobry:fix-attribute-in-type-comment into 1e55ae6 on PyCQA:master.

[Pierre-Sassoulas]

Thank you for the fix !

[Pierre-Sassoulas]

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

[superbobry]

Is it common that tests fail on PyPy but pass on CPython? I had a quick look locally, and apparently the Assign node in the test lacks a type annotation on PyPy. Have you encountered anything similar before?

[Pierre-Sassoulas]

Ha, we never encountered this before. but by your description of the problem, it might be due to the latest astroid, we added a LOT of typing recently. Do you think adding the proper typing for Assign would fix the problem with pypy ?

[superbobry]

Sorry if my earlier comment about PyPy was misleading. I was not referring to type annotations in of the Assign node, but rather the type_annotation attribute of Assign, which is non-None on CPython and None on PyPy for the same input.

[Pierre-Sassoulas]

It might be a problem in astroid that do not understand the code properly when the interpreter is pypi. I think we can handle the two case and add a @skipif for pypi. Because the fix seems non trivial.

[superbobry]

I have a theory about what's going on

• the ast module in PyPy does not yet support type comment parsing (added in CPython 3.8);
• typed_ast does not build on recent PyPy versions;
• astroid fails to import typed_ast on PyPy and falls back to ast.

I have disabled the new test on PyPy.

[Pierre-Sassoulas]

Thank you for this fix, much appreciated :) !