Merge branch 'master' into args-renamed

CONTRIBUTORS.txt

@@ -484,3 +484,5 @@ contributors:
 * Jiajunsu (victor): contributor
 
 * Andrew Haigh (nelfin): contributor
+
+* Pang Yu Shao (yushao2): contributor

ChangeLog

@@ -38,6 +38,17 @@ modules are added.
 
   Closes #3536
 
+* New checker ``consider-using-dict-items``. Emitted  when iterating over dictionary keys and then
+  indexing the same dictionary with the key within loop body.
+
+  Closes #3389
+
+* Don't emit ``import-error`` if import guarded behind ``if sys.version_info >= (x, x)``
+
+* Fix incompatibility with Python 3.6.0 caused by ``typing.Counter`` and ``typing.NoReturn`` usage
+
+  Closes #4412
+
 
 What's New in Pylint 2.8.2?
 ===========================

doc/whatsnew/2.9.rst

@@ -12,6 +12,9 @@ Summary -- Release highlights
 New checkers
 ============
 
+* ``consider-using-dict-items``: Emitted when iterating over dictionary keys and then
+  indexing the same dictionary with the key within loop body.
+
 Other Changes
 =============
 
@@ -26,3 +29,5 @@ Other Changes
 
 * A new error called ``arguments-renamed`` has been created, which identifies any changes at the parameter names
   of overridden functions. It aims to separate the functionality of ``arguments-differ``.
+
+* Fix incompatibility with Python 3.6.0 caused by ``typing.Counter`` and ``typing.NoReturn`` usage

pylint/checkers/imports.py

@@ -135,6 +135,19 @@ def _ignore_import_failure(node, modname, ignored_modules):
         if submodule in ignored_modules:
             return True
 
+    # ignore import failure if guarded by `sys.version_info` test
+    if isinstance(node.parent, astroid.If) and isinstance(
+        node.parent.test, astroid.Compare
+    ):
+        value = node.parent.test.left
+        if isinstance(value, astroid.Subscript):
+            value = value.value
+        if (
+            isinstance(value, astroid.Attribute)
+            and value.as_string() == "sys.version_info"
+        ):
+            return True
+
     return node_ignores_exception(node, ImportError)
 
 

pylint/checkers/refactoring/recommendation_checker.py

@@ -1,5 +1,6 @@
 # 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
+from typing import cast
 
 import astroid
 
@@ -26,6 +27,14 @@ class RecommendationChecker(checkers.BaseChecker):
             "method. It is enough to just iterate through the dictionary itself, as "
             'in "for key in dictionary".',
         ),
+        "C0206": (
+            "Consider iterating with .items()",
+            "consider-using-dict-items",
+            "Emitted when iterating over the keys of a dictionary and accessing the "
+            "value by index lookup."
+            "Both the key and value can be accessed by iterating using the .items() "
+            "method of the dictionary instead.",
+        ),
     }
 
     @staticmethod
@@ -53,8 +62,12 @@ def visit_call(self, node):
         if isinstance(node.parent, (astroid.For, astroid.Comprehension)):
             self.add_message("consider-iterating-dictionary", node=node)
 
-    @utils.check_messages("consider-using-enumerate")
-    def visit_for(self, node):
+    @utils.check_messages("consider-using-enumerate", "consider-using-dict-items")
+    def visit_for(self, node: astroid.For) -> None:
+        self._check_consider_using_enumerate(node)
+        self._check_consider_using_dict_items(node)
+
+    def _check_consider_using_enumerate(self, node: astroid.For) -> None:
         """Emit a convention whenever range and len are used for indexing."""
         # Verify that we have a `range([start], len(...), [stop])` call and
         # that the object which is iterated is used as a subscript in the
@@ -119,3 +132,81 @@ def visit_for(self, node):
                     continue
                 self.add_message("consider-using-enumerate", node=node)
                 return
+
+    def _check_consider_using_dict_items(self, node: astroid.For) -> None:
+        """Add message when accessing dict values by index lookup."""
+        # Verify that we have a .keys() call and
+        # that the object which is iterated is used as a subscript in the
+        # body of the for.
+
+        iterating_object_name = utils.get_iterating_dictionary_name(node)
+        if iterating_object_name is None:
+            return
+
+        # Verify that the body of the for loop uses a subscript
+        # with the object that was iterated. This uses some heuristics
+        # in order to make sure that the same object is used in the
+        # for body.
+        for child in node.body:
+            for subscript in child.nodes_of_class(astroid.Subscript):
+                subscript = cast(astroid.Subscript, subscript)
+
+                if not isinstance(subscript.value, (astroid.Name, astroid.Attribute)):
+                    continue
+
+                value = subscript.slice
+                if isinstance(value, astroid.Index):
+                    value = value.value
+                if (
+                    not isinstance(value, astroid.Name)
+                    or value.name != node.target.name
+                    or iterating_object_name != subscript.value.as_string()
+                ):
+                    continue
+                last_definition_lineno = value.lookup(value.name)[1][-1].lineno
+                if last_definition_lineno > node.lineno:
+                    # Ignore this subscript if it has been redefined after
+                    # the for loop. This checks for the line number using .lookup()
+                    # to get the line number where the iterating object was last
+                    # defined and compare that to the for loop's line number
+                    continue
+                if (
+                    isinstance(subscript.parent, astroid.Assign)
+                    and subscript in subscript.parent.targets
+                    or isinstance(subscript.parent, astroid.AugAssign)
+                    and subscript == subscript.parent.target
+                ):
+                    # Ignore this subscript if it is the target of an assignment
+                    continue
+
+                self.add_message("consider-using-dict-items", node=node)
+                return
+
+    @utils.check_messages("consider-using-dict-items")
+    def visit_comprehension(self, node: astroid.Comprehension) -> None:
+        iterating_object_name = utils.get_iterating_dictionary_name(node)
+        if iterating_object_name is None:
+            return
+
+        children = list(node.parent.get_children())
+        if node.ifs:
+            children.extend(node.ifs)
+        for child in children:
+            for subscript in child.nodes_of_class(astroid.Subscript):
+                subscript = cast(astroid.Subscript, subscript)
+
+                if not isinstance(subscript.value, (astroid.Name, astroid.Attribute)):
+                    continue
+
+                value = subscript.slice
+                if isinstance(value, astroid.Index):
+                    value = value.value
+                if (
+                    not isinstance(value, astroid.Name)
+                    or value.name != node.target.name
+                    or iterating_object_name != subscript.value.as_string()
+                ):
+                    continue
+
+                self.add_message("consider-using-dict-items", node=node)
+                return

pylint/checkers/strings.py

@@ -38,14 +38,17 @@
 import numbers
 import re
 import tokenize
-from typing import Counter, Iterable
+from typing import TYPE_CHECKING, Iterable
 
 import astroid
 
 from pylint.checkers import BaseChecker, BaseTokenChecker, utils
 from pylint.checkers.utils import check_messages
 from pylint.interfaces import IAstroidChecker, IRawChecker, ITokenChecker
 
+if TYPE_CHECKING:
+    from typing import Counter  # typing.Counter added in Python 3.6.1
+
 _AST_NODE_STR_TYPES = ("__builtin__.unicode", "__builtin__.str", "builtins.str")
 # Prefixes for both strings and bytes literals per
 # https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals
@@ -750,7 +753,8 @@ def check_for_consistent_string_delimiters(
         Args:
           tokens: The tokens to be checked against for consistent usage.
         """
-        string_delimiters: Counter[str] = collections.Counter()
+        # typing.Counter added in Python 3.6.1 so this type hint must be a comment
+        string_delimiters = collections.Counter()  # type: Counter[str]
 
         # First, figure out which quote character predominates in the module
         for tok_type, token, _, _, _ in tokens:

pylint/checkers/typecheck.py

@@ -1439,8 +1439,8 @@ def visit_call(self, node):
                         args=(display_name, callable_name),
                     )
 
-        for name in kwparams:
-            defval, assigned = kwparams[name]
+        for name, val in kwparams.items():
+            defval, assigned = val
             if (
                 defval is None
                 and not assigned

pylint/checkers/utils.py

@@ -1492,3 +1492,33 @@ def is_assign_name_annotated_with(node: astroid.AssignName, typing_name: str) ->
     ):
         return True
     return False
+
+
+def get_iterating_dictionary_name(
+    node: Union[astroid.For, astroid.Comprehension]
+) -> Optional[str]:
+    """Get the name of the dictionary which keys are being iterated over on
+    a `astroid.For` or `astroid.Comprehension` node.
+
+    If the iterating object is not either the keys method of a dictionary
+    or a dictionary itself, this returns None.
+    """
+    # Is it a proper keys call?
+    if (
+        isinstance(node.iter, astroid.Call)
+        and isinstance(node.iter.func, astroid.Attribute)
+        and node.iter.func.attrname == "keys"
+    ):
+        inferred = safe_infer(node.iter.func)
+        if not isinstance(inferred, astroid.BoundMethod):
+            return None
+        return node.iter.as_string().rpartition(".keys")[0]
+
+    # Is it a dictionary?
+    if isinstance(node.iter, (astroid.Name, astroid.Attribute)):
+        inferred = safe_infer(node.iter)
+        if not isinstance(inferred, astroid.Dict):
+            return None
+        return node.iter.as_string()
+
+    return None

requirements_test.txt

@@ -2,7 +2,7 @@
 -r requirements_test_min.txt
 coveralls~=3.0
 coverage~=5.5
-pre-commit~=2.12
+pre-commit~=2.12;python_full_version>="3.6.2"
 pyenchant~=3.2
 pytest-cov~=2.11
 pytest-profiling~=1.7

requirements_test_pre_commit.txt

@@ -1,7 +1,7 @@
 autoflake==1.4
-black==21.5b1
+black==21.5b1;python_full_version>="3.6.2"
 flake8==3.9.2
 isort==5.8.0
 mypy==0.812
-pyupgrade==2.15.0
+pyupgrade==2.15.0;python_full_version>="3.6.1"
 black-disable-checker==1.0.1

tests/functional/c/consider/consider_using_dict_items.py

@@ -0,0 +1,86 @@
+"""Emit a message for iteration through dict keys and subscripting dict with key."""
+# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods
+
+def bad():
+    a_dict = {1: 1, 2: 2, 3: 3}
+    for k in a_dict:  # [consider-using-dict-items]
+        print(a_dict[k])
+    another_dict = dict()
+    for k in another_dict:  # [consider-using-dict-items]
+        print(another_dict[k])
+
+
+def good():
+    a_dict = {1: 1, 2: 2, 3: 3}
+    for k in a_dict:
+        print(k)
+
+out_of_scope_dict = dict()
+
+def another_bad():
+    for k in out_of_scope_dict:  # [consider-using-dict-items]
+        print(out_of_scope_dict[k])
+
+def another_good():
+    for k in out_of_scope_dict:
+        k = 1
+        k = 2
+        k = 3
+        print(out_of_scope_dict[k])
+
+
+b_dict = {}
+for k2 in b_dict:  # Should not emit warning, key access necessary
+    b_dict[k2] = 2
+
+for k2 in b_dict:  # Should not emit warning, key access necessary (AugAssign)
+    b_dict[k2] += 2
+
+# Warning should be emitted in this case
+for k6 in b_dict:  # [consider-using-dict-items]
+    val = b_dict[k6]
+    b_dict[k6] = 2
+
+for k3 in b_dict:  # [consider-using-dict-items]
+    val = b_dict[k3]
+
+for k4 in b_dict.keys():  # [consider-iterating-dictionary,consider-using-dict-items]
+    val = b_dict[k4]
+
+class Foo:
+    c_dict = {}
+
+# Should emit warning when iterating over a dict attribute of a class
+for k5 in Foo.c_dict:  # [consider-using-dict-items]
+    val = Foo.c_dict[k5]
+
+c_dict = {}
+
+# Should NOT emit warning whey key used to access a different dict
+for k5 in Foo.c_dict:  # This is fine
+    val = b_dict[k5]
+
+for k5 in Foo.c_dict:  # This is fine
+    val = c_dict[k5]
+
+# Should emit warning within a list/dict comprehension
+val = {k9: b_dict[k9] for k9 in b_dict}  # [consider-using-dict-items]
+val = [(k7, b_dict[k7]) for k7 in b_dict]  # [consider-using-dict-items]
+
+# Should emit warning even when using dict attribute of a class within comprehension
+val = [(k7, Foo.c_dict[k7]) for k7 in Foo.c_dict]  # [consider-using-dict-items]
+val = any(True for k8 in Foo.c_dict if Foo.c_dict[k8])  # [consider-using-dict-items]
+
+# Should emit warning when dict access done in ``if`` portion of comprehension
+val = any(True for k8 in b_dict if b_dict[k8])  # [consider-using-dict-items]
+
+# Should NOT emit warning whey key used to access a different dict
+val = [(k7, b_dict[k7]) for k7 in Foo.c_dict]
+val = any(True for k8 in Foo.c_dict if b_dict[k8])
+
+# Should NOT emit warning, essentially same check as above
+val = [(k7, c_dict[k7]) for k7 in Foo.c_dict]
+val = any(True for k8 in Foo.c_dict if c_dict[k8])
+
+# Should emit warning, using .keys() of Foo.c_dict
+val = any(True for k8 in Foo.c_dict.keys() if Foo.c_dict[k8])  # [consider-iterating-dictionary,consider-using-dict-items]

tests/functional/c/consider/consider_using_dict_items.txt

@@ -0,0 +1,15 @@
+consider-using-dict-items:6:4:bad:Consider iterating with .items()
+consider-using-dict-items:9:4:bad:Consider iterating with .items()
+consider-using-dict-items:21:4:another_bad:Consider iterating with .items()
+consider-using-dict-items:40:0::Consider iterating with .items()
+consider-using-dict-items:44:0::Consider iterating with .items()
+consider-iterating-dictionary:47:10::Consider iterating the dictionary directly instead of calling .keys()
+consider-using-dict-items:47:0::Consider iterating with .items()
+consider-using-dict-items:54:0::Consider iterating with .items()
+consider-using-dict-items:67:0::Consider iterating with .items()
+consider-using-dict-items:68:0::Consider iterating with .items()
+consider-using-dict-items:71:0::Consider iterating with .items()
+consider-using-dict-items:72:0::Consider iterating with .items()
+consider-using-dict-items:75:0::Consider iterating with .items()
+consider-iterating-dictionary:86:25::Consider iterating the dictionary directly instead of calling .keys()
+consider-using-dict-items:86:0::Consider iterating with .items()

tests/functional/i/import_error.py

@@ -26,3 +26,12 @@
 
 # pylint: disable=no-name-in-module
 from functional.s.syntax_error import toto # [syntax-error]
+
+# Don't emit import-error if guarded behind `sys.version_info`
+import sys
+
+if sys.version_info >= (3, 9):
+    import zoneinfo
+
+if sys.version_info[:2] >= (3, 9):
+    import zoneinfo