OverwrittenKeyInspection

Fixes IDEA-179354 Warn if Map/Set entry is overwritten in a series of consecutive add/put calls

java/java-impl/src/com/intellij/codeInspection/OverwrittenKeyInspection.java

@@ -0,0 +1,135 @@
+// Copyright 2000-2017 JetBrains s.r.o.
+// Use of this source code is governed by the Apache 2.0 license that can be
+// found in the LICENSE file.
+package com.intellij.codeInspection;
+
+import com.intellij.codeInsight.PsiEquivalenceUtil;
+import com.intellij.openapi.fileEditor.OpenFileDescriptor;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.*;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.psi.util.PsiUtil;
+import com.siyeh.ig.callMatcher.CallMatcher;
+import com.siyeh.ig.psiutils.ExpressionUtils;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
+import org.jetbrains.annotations.Nls;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.*;
+
+import static com.intellij.util.ObjectUtils.tryCast;
+
+public class OverwrittenKeyInspection extends BaseJavaBatchLocalInspectionTool {
+  private static final CallMatcher SET_ADD =
+    CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_SET, "add").parameterCount(1);
+  private static final CallMatcher MAP_PUT =
+    CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_MAP, "put").parameterCount(2);
+
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new JavaElementVisitor() {
+      Set<PsiMethodCallExpression> analyzed = new HashSet<>();
+
+      @Override
+      public void visitMethodCallExpression(PsiMethodCallExpression call) {
+        PsiExpressionStatement statement = tryCast(call.getParent(), PsiExpressionStatement.class);
+        if (statement == null) return;
+        CallMatcher myMatcher;
+        if (SET_ADD.test(call)) {
+          myMatcher = SET_ADD;
+        }
+        else if (MAP_PUT.test(call)) {
+          myMatcher = MAP_PUT;
+        }
+        else {
+          return;
+        }
+        if (!analyzed.add(call)) return;
+
+        Object key = getKey(call);
+        if (key == null) return;
+        PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(ExpressionUtils.getQualifierOrThis(call.getMethodExpression()));
+        if (qualifier == null) return;
+        PsiVariable qualifierVar =
+          qualifier instanceof PsiReferenceExpression ? tryCast(((PsiReferenceExpression)qualifier).resolve(), PsiVariable.class) : null;
+        Map<Object, List<PsiMethodCallExpression>> map = new HashMap<>();
+        map.computeIfAbsent(key, k -> new ArrayList<>()).add(call);
+        while (true) {
+          PsiExpressionStatement nextStatement =
+            tryCast(PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class), PsiExpressionStatement.class);
+          if (nextStatement == null) break;
+          PsiMethodCallExpression nextCall = tryCast(nextStatement.getExpression(), PsiMethodCallExpression.class);
+          if (!myMatcher.test(nextCall)) break;
+          PsiExpression nextQualifier =
+            PsiUtil.skipParenthesizedExprDown(ExpressionUtils.getQualifierOrThis(nextCall.getMethodExpression()));
+          if (nextQualifier == null || !PsiEquivalenceUtil.areElementsEquivalent(qualifier, nextQualifier)) break;
+          analyzed.add(nextCall);
+          if (qualifierVar != null && VariableAccessUtils.variableIsUsed(qualifierVar, nextCall.getArgumentList())) break;
+          Object nextKey = getKey(nextCall);
+          if (nextKey != null) {
+            map.computeIfAbsent(nextKey, k -> new ArrayList<>()).add(nextCall);
+          }
+          statement = nextStatement;
+        }
+        for (List<PsiMethodCallExpression> calls : map.values()) {
+          if (calls.size() < 2) continue;
+          for (int i = 0; i < calls.size(); i++) {
+            PsiMethodCallExpression dup = calls.get(i);
+            PsiExpression arg = dup.getArgumentList().getExpressions()[0];
+            LocalQuickFix fix = null;
+            if (isOnTheFly) {
+              PsiExpression nextArg = calls.get((i + 1) % calls.size()).getArgumentList().getExpressions()[0];
+              fix = new NavigateToDuplicateFix(nextArg);
+            }
+            String message = myMatcher == SET_ADD ?
+                              InspectionsBundle.message("inspection.overwritten.key.set.message") :
+                              InspectionsBundle.message("inspection.overwritten.key.map.message");
+            holder.registerProblem(arg, message, fix);
+          }
+        }
+      }
+
+      private Object getKey(PsiMethodCallExpression call) {
+        PsiExpression key = call.getArgumentList().getExpressions()[0];
+        Object constant = ExpressionUtils.computeConstantExpression(key);
+        if (constant != null) {
+          return constant;
+        }
+        if (key instanceof PsiReferenceExpression) {
+          PsiField field = tryCast(((PsiReferenceExpression)key).resolve(), PsiField.class);
+          if (field instanceof PsiEnumConstant ||
+              field != null && field.hasModifierProperty(PsiModifier.FINAL) && field.hasModifierProperty(PsiModifier.STATIC)) {
+            return field;
+          }
+        }
+        return null;
+      }
+    };
+  }
+
+  private static class NavigateToDuplicateFix implements LocalQuickFix {
+    private final SmartPsiElementPointer<PsiExpression> myPointer;
+
+    public NavigateToDuplicateFix(PsiExpression arg) {
+      myPointer = SmartPointerManager.getInstance(arg.getProject()).createSmartPsiElementPointer(arg);
+    }
+
+    @Nls
+    @NotNull
+    @Override
+    public String getFamilyName() {
+      return InspectionsBundle.message("navigate.to.duplicate.fix");
+    }
+
+    @Override
+    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+      PsiExpression element = myPointer.getElement();
+      if (element == null) return;
+      PsiFile file = element.getContainingFile();
+      if (file == null) return;
+      int offset = element.getTextRange().getStartOffset();
+      new OpenFileDescriptor(project, file.getVirtualFile(), offset).navigate(true);
+    }
+  }
+}

java/java-tests/testData/inspection/overwrittenKey/OverwrittenKey.java

@@ -0,0 +1,42 @@
+import java.util.*;
+
+class OverwrittenKey {
+  void fillMap(Map<String, Integer> map) {
+    map.put(<warning descr="Duplicating Map key">"a"</warning>, 1);
+    map.put("b", 2);
+    map.put(<warning descr="Duplicating Map key">"c"</warning>, 3);
+    map.put("d", 4);
+    map.put(<warning descr="Duplicating Map key">"a"</warning>, 5);
+    map.put(<warning descr="Duplicating Map key">"a"</warning>, 6);
+    map.put("e", 7);
+    map.put("f", 8);
+    map.put(<warning descr="Duplicating Map key">"c"</warning>, 9);
+  }
+
+  void fillSet(HashSet<Integer> set, HashSet<Integer> set2) {
+    set.add(5234);
+    set.add(<warning descr="Duplicating Set element">5235</warning>);
+    set.add(3452);
+    set.add(3256);
+    set.add(<warning descr="Duplicating Set element">4635</warning>);
+    set.add(<warning descr="Duplicating Set element">4635</warning>);
+    set.add(3252);
+    set.add(2352);
+    set.add(5253);
+    set.add(<warning descr="Duplicating Set element">5235</warning>);
+    set.add(2145);
+    set2.add(2145);
+  }
+
+  enum Test {
+    A,B,C,D,E
+  }
+
+  Map<Test, String> map = new HashMap<Test, String>() {{
+    put(Test.A, "a");
+    put(Test.B, "b");
+    this.put(<warning descr="Duplicating Map key">Test.C</warning>, "c");
+    put(<warning descr="Duplicating Map key">Test.C</warning>, "d");
+    put(Test.E, "e");
+  }};
+}

java/java-tests/testSrc/com/intellij/java/codeInspection/OverwrittenKeyInspectionTest.java

@@ -0,0 +1,33 @@
+// Copyright 2000-2017 JetBrains s.r.o.
+// Use of this source code is governed by the Apache 2.0 license that can be
+// found in the LICENSE file.
+package com.intellij.java.codeInspection;
+
+import com.intellij.JavaTestUtil;
+import com.intellij.codeInspection.InspectionProfileEntry;
+import com.intellij.codeInspection.OverwrittenKeyInspection;
+import com.intellij.testFramework.LightProjectDescriptor;
+import com.siyeh.ig.LightInspectionTestCase;
+import org.jetbrains.annotations.NotNull;
+
+public class OverwrittenKeyInspectionTest extends LightInspectionTestCase {
+  public void testOverwrittenKey() {
+    doTest();
+  }
+
+  @Override
+  protected InspectionProfileEntry getInspection() {
+    return new OverwrittenKeyInspection();
+  }
+
+  @Override
+  protected String getBasePath() {
+    return JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/overwrittenKey/";
+  }
+
+  @NotNull
+  @Override
+  protected LightProjectDescriptor getProjectDescriptor() {
+    return JAVA_8;
+  }
+}

platform/platform-resources-en/src/messages/InspectionsBundle.properties

@@ -909,4 +909,7 @@ unused.import.display.name=Unused import
 inspection.fuse.stream.operations.fix.family.name=Fuse more statements to the Stream API chain
 inspection.fuse.stream.operations.fix.name=Fuse {0} into the Stream API chain
 inspection.fuse.stream.operations.message=Stream may be extended replacing {0}
-inspection.fuse.stream.operations.display.name=Subsequent steps can be fused into Stream API chain
+inspection.fuse.stream.operations.display.name=Subsequent steps can be fused into Stream API chain
+inspection.overwritten.key.set.message=Duplicating Set element
+inspection.overwritten.key.map.message=Duplicating Map key
+navigate.to.duplicate.fix=Navigate to duplicate

resources-en/src/inspectionDescriptions/OverwrittenKey.html

@@ -0,0 +1,15 @@
+<html>
+<body>
+Warns if <code>Map</code> key or <code>Set</code> element was overwritten in the sequence of add/put calls. This usually
+occurs due to copy-paste error. Example:
+<pre>
+  map.put("A", 1);
+  map.put("B", 2);
+  map.put("C", 3);
+  map.put("D", 4);
+  map.put("A", 5); // duplicating key "A", overwrites previously written entry
+</pre>
+<!-- tooltip end -->
+<p><small>New in 2017.3</small></p>
+</body>
+</html>

resources/src/META-INF/IdeaPlugin.xml

@@ -883,6 +883,11 @@
                      groupKey="group.names.code.style.issues" enabledByDefault="true" level="WARNING"
                      implementationClass="com.intellij.codeInspection.CollectionAddAllCanBeReplacedWithConstructorInspection"
                      displayName="Redundant 'Collection.addAll()' call"/>
+    <localInspection groupPath="Java" language="JAVA" shortName="OverwrittenKey"
+                     groupBundle="messages.InspectionsBundle"
+                     groupKey="group.names.probable.bugs" enabledByDefault="true" level="WARNING"
+                     implementationClass="com.intellij.codeInspection.OverwrittenKeyInspection"
+                     displayName="Overwritten Map key or Set element"/>
     <localInspection groupPath="Java,Java language level migration aids" language="JAVA" shortName="AnonymousHasLambdaAlternative" displayName="Anonymous type has shorter lambda alternative"
                      groupKey="group.names.language.level.specific.issues.and.migration.aids8" groupBundle="messages.InspectionsBundle" enabledByDefault="true" level="WARNING"
                      implementationClass="com.intellij.codeInspection.AnonymousHasLambdaAlternativeInspection" />