SONARPY-1521 S4143: Fix FP when there is different collections value …

…assignment for same key (#1609)
SonarSource · Oct 30, 2023 · e710dfc · e710dfc
1 parent bb5944c
commit e710dfc
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 13 deletions.
diff --git a/python-checks/src/main/java/org/sonar/python/checks/OverwrittenCollectionEntryCheck.java b/python-checks/src/main/java/org/sonar/python/checks/OverwrittenCollectionEntryCheck.java
@@ -21,16 +21,20 @@
 
 import java.util.AbstractMap;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.IssueLocation;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.tree.AssignmentStatement;
 import org.sonar.plugins.python.api.tree.Expression;
 import org.sonar.plugins.python.api.tree.HasSymbol;
@@ -46,8 +50,6 @@
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.tree.Tree.Kind;
 import org.sonar.plugins.python.api.tree.UnaryExpression;
-import org.sonar.plugins.python.api.IssueLocation;
-import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.python.tree.TreeUtils;
 
 @Rule(key = "S4143")
@@ -87,12 +89,10 @@ private static CollectionWrite collectionWrite(AssignmentStatement assignment, E
       SliceExpression sliceExpression = (SliceExpression) expression;
       String key = key(sliceExpression.sliceList().children());
       return collectionWrite(assignment, sliceExpression.object(), key, sliceExpression.leftBracket(), sliceExpression.rightBracket());
-
     } else if (expression.is(Kind.SUBSCRIPTION)) {
       SubscriptionExpression subscription = (SubscriptionExpression) expression;
       String key = key(subscription.subscripts().children());
       return collectionWrite(assignment, subscription.object(), key, subscription.leftBracket(), subscription.rightBracket());
-
     } else {
       return null;
     }
@@ -110,15 +110,38 @@ private static CollectionWrite collectionWrite(AssignmentStatement assignment, E
       }
     }
 
-    if (collection instanceof HasSymbol) {
-      Symbol symbol = ((HasSymbol) collection).symbol();
-      if (symbol != null) {
-        CollectionKey collectionKey = new CollectionKey(symbol, key);
-        return new CollectionWrite(collectionKey, lBracket, rBracket, assignment, collection);
-      }
+
+    var collectionSymbols = getCollectionSymbol(collection);
+    if (!collectionSymbols.isEmpty()) {
+      var collectionKey = new CollectionKey(collectionSymbols, key);
+      return new CollectionWrite(collectionKey, lBracket, rBracket, assignment, collection);
+    } else {
+      return null;
     }
+  }
 
-    return null;
+  private static List<Symbol> getCollectionSymbol(Expression collection) {
+    if (collection.is(Kind.CALL_EXPR)
+      || TreeUtils.hasDescendant(collection, t -> t.is(Kind.CALL_EXPR, Kind.SUBSCRIPTION, Kind.SLICE_EXPR))) {
+      return List.of();
+    }
+    var names = findNames(collection);
+    return names.stream()
+      .map(HasSymbol::symbol)
+      .filter(Objects::nonNull)
+      .collect(Collectors.toList());
+  }
+
+  private static List<Name> findNames(Tree tree) {
+    if (tree.is(Kind.NAME)) {
+      return List.of((Name) tree);
+    } else {
+      return tree.children()
+        .stream()
+        .map(OverwrittenCollectionEntryCheck::findNames)
+        .flatMap(Collection::stream)
+        .collect(Collectors.toList());
+    }
   }
 
   @CheckForNull
@@ -175,9 +198,9 @@ private static void reportOverwrites(SubscriptionContext ctx, Map<CollectionKey,
     });
   }
 
-  private static class CollectionKey extends AbstractMap.SimpleImmutableEntry<Symbol, String> {
+  private static class CollectionKey extends AbstractMap.SimpleImmutableEntry<List<Symbol>, String> {
 
-    private CollectionKey(Symbol collection, String key) {
+    private CollectionKey(List<Symbol> collection, String key) {
       super(collection, key);
     }
 

diff --git a/python-checks/src/test/resources/checks/overwrittenCollectionEntryCheck.py b/python-checks/src/test/resources/checks/overwrittenCollectionEntryCheck.py
@@ -101,3 +101,36 @@ def used_collection(list1):
 
     list[3] = 42
     list[3] = list[1] # FN
+
+def same_key_multiple_collections(bar_list, foo_entry, bar_entry, foo_value, bar_value):
+    import pandas as pd
+    a = {}
+    a.foo_matrix = pd.DataFrame(index=bar_list, columns=bar_list)
+    a.bar_matrix = pd.DataFrame(index=bar_list, columns=bar_list)
+
+    a.foo_matrix.loc[foo_entry, bar_entry] = foo_value
+    a.foo_matrix.loc[foo_entry, bar_entry] = foo_value # Noncompliant
+    a.foo_matrix.at[foo_entry, bar_entry] = foo_value
+
+class Record:
+    def __init__(self, names):
+        """Initialize class."""
+        self._names = names
+        self._count_info = {}
+        self._label_info = {}
+        for name in self._names:
+            self._count_info[name] = 0
+            self._count_info[name] = 2 # Noncompliant
+            self._label_info[name] = None
+
+def sub_collection():
+    defs = anno.getanno(symbol_a, anno.Static.ORIG_DEFINITIONS)
+    defs[0].directives[directive_key] = {
+        'test_arg': parser.parse_expression('foo'),
+        'other_arg': parser.parse_expression('bar'),
+    }
+    defs[1].directives[directive_key] = {
+        'test_arg': parser.parse_expression('foo'),
+        'other_arg': parser.parse_expression('baz'),
+    }
+