Merge pull request #5331 from yushao2/fix--false-negative-5323

fix(consider-interating-dictionary): fix false negatives

6 files changed, 62 insertions, 4 deletions

diff --git a/ChangeLog b/ChangeLog
index c2ac1d4bb..4ba15898b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -66,6 +66,11 @@ Release date: 2021-11-24
 
   Closes #4412 #5287
 
+* Fix false negative for ``consider-iterating-dictionary`` during membership checks encapsulated in iterables
+  or ``not in`` checks
+
+  Closes #5323
+
 * Fix ``install graphiz`` message which isn't needed for puml output format.
 
 * ``MessageTest`` of the unittest ``testutil`` now requires the ``confidence`` attribute
diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst
index 378b4103b..ab3d1d01f 100644
--- a/doc/whatsnew/2.13.rst
+++ b/doc/whatsnew/2.13.rst
@@ -20,6 +20,11 @@ Extensions
 Other Changes
 =============
 
+* Fix false negative for ``consider-iterating-dictionary`` during membership checks encapsulated in iterables
+  or ``not in`` checks
+
+  Closes #5323
+
 * Require Python ``3.6.2`` to run pylint.
 
   Closes #5065
diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py
index 210513ddc..98b2973d8 100644
--- a/pylint/checkers/refactoring/recommendation_checker.py
+++ b/pylint/checkers/refactoring/recommendation_checker.py
@@ -83,13 +83,15 @@ class RecommendationChecker(checkers.BaseChecker):
             return
         if node.func.attrname != "keys":
             return
+        comp_ancestor = utils.get_node_first_ancestor_of_type(node, (nodes.Compare,))
         if (
             isinstance(node.parent, (nodes.For, nodes.Comprehension))
-            or isinstance(node.parent, nodes.Compare)
+            or comp_ancestor
             and any(
                 op
-                for op, comparator in node.parent.ops
-                if op == "in" and comparator is node
+                for op, comparator in comp_ancestor.ops
+                if op in {"in", "not in"}
+                and (comparator in node.node_ancestors() or comparator is node)
             )
         ):
             inferred = utils.safe_infer(node.func)
@@ -97,7 +99,6 @@ class RecommendationChecker(checkers.BaseChecker):
                 inferred.bound, nodes.Dict
             ):
                 return
-
             self.add_message("consider-iterating-dictionary", node=node)
 
     def _check_use_maxsplit_arg(self, node: nodes.Call) -> None:
diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index c7f2cec92..c0bbe918e 100644
--- a/pylint/checkers/utils.py
+++ b/pylint/checkers/utils.py
@@ -75,6 +75,7 @@ from typing import (
     Set,
     Tuple,
     Type,
+    TypeVar,
     Union,
 )
 
@@ -280,6 +281,8 @@ SUBSCRIPTABLE_CLASSES_PEP585 = frozenset(
     )
 )
 
+T_Node = TypeVar("T_Node", bound=nodes.NodeNG)
+
 
 class NoSuchArgumentError(Exception):
     pass
@@ -1690,3 +1693,13 @@ def returns_bool(node: nodes.NodeNG) -> bool:
         and isinstance(node.value, nodes.Const)
         and node.value.value in {True, False}
     )
+
+
+def get_node_first_ancestor_of_type(
+    node: nodes.NodeNG, ancestor_type: Tuple[Type[T_Node]]
+) -> Optional[T_Node]:
+    """Return the first parent node that is any of the provided types (or None)"""
+    for ancestor in node.node_ancestors():
+        if isinstance(ancestor, ancestor_type):
+            return ancestor
+    return None
diff --git a/tests/functional/c/consider/consider_iterating_dictionary.py b/tests/functional/c/consider/consider_iterating_dictionary.py
index d3cc75f4d..70a5d3187 100644
--- a/tests/functional/c/consider/consider_iterating_dictionary.py
+++ b/tests/functional/c/consider/consider_iterating_dictionary.py
@@ -66,3 +66,29 @@ VAR = 1 in {}.keys() # [consider-iterating-dictionary]
 VAR = 1 in {}
 VAR = 1 in dict()
 VAR = [1, 2] == {}.keys() in {False}
+
+# Additional membership checks
+# See: https://github.com/PyCQA/pylint/issues/5323
+metadata = {}
+if "a" not in list(metadata.keys()): # [consider-iterating-dictionary]
+    print(1)
+if "a" not in metadata.keys(): # [consider-iterating-dictionary]
+    print(1)
+if "a" in list(metadata.keys()): # [consider-iterating-dictionary]
+    print(1)
+if "a" in metadata.keys(): # [consider-iterating-dictionary]
+    print(1)
+
+
+class AClass:
+    def a_function(self):
+        class InnerClass:
+            def another_function(self):
+                def inner_function():
+                    another_metadata = {}
+                    print("a" not in list(another_metadata.keys())) # [consider-iterating-dictionary]
+                    print("a" not in another_metadata.keys()) # [consider-iterating-dictionary]
+                    print("a" in list(another_metadata.keys())) # [consider-iterating-dictionary]
+                    print("a" in another_metadata.keys()) # [consider-iterating-dictionary]
+                return  inner_function()
+        return InnerClass().another_function()
diff --git a/tests/functional/c/consider/consider_iterating_dictionary.txt b/tests/functional/c/consider/consider_iterating_dictionary.txt
index 305b38e52..f251fa286 100644
--- a/tests/functional/c/consider/consider_iterating_dictionary.txt
+++ b/tests/functional/c/consider/consider_iterating_dictionary.txt
@@ -16,3 +16,11 @@ consider-iterating-dictionary:40:60:40:71::Consider iterating the dictionary dir
 consider-iterating-dictionary:43:8:43:21::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
 consider-iterating-dictionary:45:8:45:17::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
 consider-iterating-dictionary:65:11:65:20::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:73:19:73:34::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:75:14:75:29::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:77:15:77:30::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:79:10:79:25::Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:89:42:89:65:AClass.a_function.InnerClass.another_function.inner_function:Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:90:37:90:60:AClass.a_function.InnerClass.another_function.inner_function:Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:91:38:91:61:AClass.a_function.InnerClass.another_function.inner_function:Consider iterating the dictionary directly instead of calling .keys():UNDEFINED
+consider-iterating-dictionary:92:33:92:56:AClass.a_function.InnerClass.another_function.inner_function:Consider iterating the dictionary directly instead of calling .keys():UNDEFINED