diff --git a/its/ruling/src/test/resources/sonar-server/squid-S3824.json b/its/ruling/src/test/resources/sonar-server/squid-S3824.json deleted file mode 100644 index c44a8f49af8..00000000000 --- a/its/ruling/src/test/resources/sonar-server/squid-S3824.json +++ /dev/null @@ -1,33 +0,0 @@ -{ -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/component/MapBasedDbIdsRepository.java':[ -47, -64, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/formula/FormulaExecutorComponentVisitor.java':[ -223, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/measure/MapBasedRawMeasureRepository.java':[ -163, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/duplication/ws/DuplicationsJsonWriter.java':[ -71, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/es/Facets.java':[ -182, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/issue/notification/NewIssuesStatistics.java':[ -53, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/permission/index/PermissionIndexerDao.java':[ -240, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/plugins/ws/PluginUpdateAggregator.java':[ -52, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/startup/RegisterMetrics.java':[ -125, -], -'org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/util/cache/MemoryCache.java':[ -45, -], -} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java index 5890ed5dd34..20650aa2c4b 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java +++ b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java @@ -85,7 +85,6 @@ import org.sonar.java.se.checks.DivisionByZeroCheck; import org.sonar.java.se.checks.InvariantReturnCheck; import org.sonar.java.se.checks.LocksNotUnlockedCheck; -import org.sonar.java.se.checks.MapComputeIfAbsentOrPresentCheck; import org.sonar.java.se.checks.NoWayOutLoopCheck; import org.sonar.java.se.checks.NonNullSetToNullCheck; import org.sonar.java.se.checks.NullDereferenceCheck; @@ -408,7 +407,6 @@ public static List> getJavaChecks() { .add(StaticMethodCheck.class) .add(ForLoopUsedAsWhileLoopCheck.class) .add(MultilineBlocksCurlyBracesCheck.class) - .add(MapComputeIfAbsentOrPresentCheck.class) .add(EnumMapCheck.class) .add(FileCreateTempFileCheck.class) .add(BooleanInversionCheck.class) diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.html deleted file mode 100644 index 4a3e3de4239..00000000000 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.html +++ /dev/null @@ -1,20 +0,0 @@ -

It's a common pattern to test the result of a java.util.Map.get() against null before proceeding with adding or changing -the value in the map. However the java.util.Map API offers a significantly better alternative in the form of the -computeIfPresent() and computeIfAbsent() methods. Using these instead leads to cleaner and more readable code.

-

Note that this rule is automatically disabled when the project's sonar.java.source is not 8.

-

Noncompliant Code Example

-
-V value = map.get(key);
-if (value == null) {  // Noncompliant
-  value = V.createFor(key);
-  if (value != null) {
-    map.put(key, value);
-  }
-}
-return value;
-
-

Compliant Solution

-
-return map.computeIfAbsent(key, k -> V.createFor(k));
-
- diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.json deleted file mode 100644 index 07488b03067..00000000000 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3824_java.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "title": "\"Map.get\" and value test should be replaced with single method call", - "type": "CODE_SMELL", - "status": "ready", - "remediation": { - "func": "Constant\/Issue", - "constantCost": "10min" - }, - "tags": [ - "java8" - ], - "defaultSeverity": "Major" -} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/Sonar_way_profile.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/Sonar_way_profile.json index b8498f1fe23..984b742c546 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/Sonar_way_profile.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/Sonar_way_profile.json @@ -273,7 +273,6 @@ "S3655", "S3725", "S3776", - "S3824", "S3878", "S3923", "S3958", diff --git a/java-frontend/src/main/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheck.java b/java-frontend/src/main/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheck.java deleted file mode 100644 index 5937ab23307..00000000000 --- a/java-frontend/src/main/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheck.java +++ /dev/null @@ -1,184 +0,0 @@ -/* - * SonarQube Java - * Copyright (C) 2012-2017 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.java.se.checks; - -import com.google.common.collect.LinkedListMultimap; -import com.google.common.collect.Multimap; - -import org.sonar.check.Rule; -import org.sonar.java.JavaVersionAwareVisitor; -import org.sonar.java.cfg.CFG; -import org.sonar.java.matcher.MethodMatcher; -import org.sonar.java.matcher.TypeCriteria; -import org.sonar.java.se.CheckerContext; -import org.sonar.java.se.ExplodedGraph; -import org.sonar.java.se.ExplodedGraph.Node; -import org.sonar.java.se.FlowComputation; -import org.sonar.java.se.ProgramState; -import org.sonar.java.se.constraint.ObjectConstraint; -import org.sonar.java.se.symbolicvalues.SymbolicValue; -import org.sonar.plugins.java.api.JavaFileScannerContext; -import org.sonar.plugins.java.api.JavaVersion; -import org.sonar.plugins.java.api.tree.MethodInvocationTree; -import org.sonar.plugins.java.api.tree.MethodTree; -import org.sonar.plugins.java.api.tree.Tree; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; - -@Rule(key = "S3824") -public class MapComputeIfAbsentOrPresentCheck extends SECheck implements JavaVersionAwareVisitor { - - private static final MethodMatcher MAP_GET = mapMethod("get", TypeCriteria.anyType()); - private static final MethodMatcher MAP_PUT = mapMethod("put", TypeCriteria.anyType(), TypeCriteria.anyType()); - - private final Multimap mapGetInvocations = LinkedListMultimap.create(); - private final List checkIssues = new ArrayList<>(); - - @Override - public boolean isCompatibleWithJavaVersion(JavaVersion version) { - return version.isJava8Compatible(); - } - - @Override - public void init(MethodTree methodTree, CFG cfg) { - mapGetInvocations.clear(); - checkIssues.clear(); - } - - private static MethodMatcher mapMethod(String methodName, TypeCriteria... parameterTypes) { - return MethodMatcher.create().typeDefinition(TypeCriteria.subtypeOf("java.util.Map")).name(methodName).parameters(parameterTypes); - } - - @Override - public ProgramState checkPostStatement(CheckerContext context, Tree syntaxNode) { - if (syntaxNode.is(Tree.Kind.METHOD_INVOCATION)) { - MethodInvocationTree mit = (MethodInvocationTree) syntaxNode; - if (MAP_GET.matches(mit)) { - ProgramState psBeforeInvocation = context.getNode().programState; - ProgramState psAfterInvocation = context.getState(); - - SymbolicValue keySV = psBeforeInvocation.peekValue(0); - SymbolicValue mapSV = psBeforeInvocation.peekValue(1); - SymbolicValue valueSV = psAfterInvocation.peekValue(); - - mapGetInvocations.put(mapSV, new MapGetInvocation(valueSV, keySV, mit)); - } - } - return super.checkPostStatement(context, syntaxNode); - } - - @Override - public ProgramState checkPreStatement(CheckerContext context, Tree syntaxNode) { - if (syntaxNode.is(Tree.Kind.METHOD_INVOCATION)) { - MethodInvocationTree mit = (MethodInvocationTree) syntaxNode; - if (MAP_PUT.matches(mit)) { - ProgramState ps = context.getState(); - - SymbolicValue keySV = ps.peekValue(1); - SymbolicValue mapSV = ps.peekValue(2); - mapGetInvocations.get(mapSV).stream() - .filter(getOnSameMap -> getOnSameMap.withSameKey(keySV)) - .findAny() - .ifPresent(getOnSameMap -> { - ObjectConstraint constraint = ps.getConstraint(getOnSameMap.value, ObjectConstraint.class); - if (constraint != null) { - checkIssues.add(new CheckIssue(context.getNode(), getOnSameMap.mit, mit, getOnSameMap.value, constraint)); - } - }); - } - } - return super.checkPreStatement(context, syntaxNode); - } - - @Override - public void checkEndOfExecution(CheckerContext context) { - SECheck check = this; - checkIssues.stream().filter(checkIssue -> checkIssue.isOnlyPossibleIssueForReportTree(checkIssues)).forEach(issue -> issue.report(context, check)); - } - - private static class CheckIssue { - private final ExplodedGraph.Node node; - - private final MethodInvocationTree getInvocation; - private final MethodInvocationTree putInvocation; - - private final SymbolicValue value; - private final ObjectConstraint valueConstraint; - - private CheckIssue(Node node, MethodInvocationTree getInvocation, MethodInvocationTree putInvocation, SymbolicValue value, ObjectConstraint valueConstraint) { - this.node = node; - - this.getInvocation = getInvocation; - this.putInvocation = putInvocation; - - this.value = value; - this.valueConstraint = valueConstraint; - } - - private boolean isOnlyPossibleIssueForReportTree(List otherIssues) { - return otherIssues.stream().noneMatch(this::differentIssueOnSameTree); - } - - private boolean differentIssueOnSameTree(CheckIssue otherIssue) { - return this != otherIssue && getInvocation.equals(otherIssue.getInvocation) && valueConstraint != otherIssue.valueConstraint; - } - - private void report(CheckerContext context, SECheck check) { - context.reportIssue(getInvocation, check, issueMsg(), flows()); - } - - private String issueMsg() { - return String.format("Replace this \"Map.get()\" and condition with a call to \"Map.%s()\".", - valueConstraint == ObjectConstraint.NULL ? "computeIfAbsent" : "computeIfPresent"); - } - - private Set> flows() { - // build nullness flows for value constraint - Set> flows = FlowComputation.flow(node, value, Collections.singletonList(ObjectConstraint.class)); - // enrich each flow with both map method invocations - return flows.stream().map(flow -> { - List newFlow = new ArrayList<>(flow); - newFlow.add(new JavaFileScannerContext.Location("'Map.get()' is invoked.", getInvocation.methodSelect())); - newFlow.add(0, new JavaFileScannerContext.Location("'Map.put()' is invoked with same key.", putInvocation.methodSelect())); - return Collections.unmodifiableList(newFlow); - }).collect(Collectors.toSet()); - } - } - - private static class MapGetInvocation { - private final SymbolicValue value; - private final SymbolicValue key; - private final MethodInvocationTree mit; - - private MapGetInvocation(SymbolicValue value, SymbolicValue key, MethodInvocationTree mit) { - this.value = value; - this.key = key; - this.mit = mit; - } - - private boolean withSameKey(SymbolicValue key) { - return this.key.equals(key); - } - } -} diff --git a/java-frontend/src/test/files/se/MapComputeIfAbsentOrPresentCheck.java b/java-frontend/src/test/files/se/MapComputeIfAbsentOrPresentCheck.java deleted file mode 100644 index 9b822589afc..00000000000 --- a/java-frontend/src/test/files/se/MapComputeIfAbsentOrPresentCheck.java +++ /dev/null @@ -1,142 +0,0 @@ -import com.google.common.base.Preconditions; - -import java.util.Map; -import java.util.Objects; - -abstract class A { - - void foo(Map map, String key) { - // Noncompliant@+1 [[flows=computeIfAbsent]] {{Replace this "Map.get()" and condition with a call to "Map.computeIfAbsent()".}} - Object value = map.get(key); // flow@computeIfAbsent [[order=1]] {{'Map.get()' is invoked.}} - if (value == null) { // flow@computeIfAbsent [[order=2]] {{Implies 'value' can be null.}} - map.put(key, new Object()); // flow@computeIfAbsent [[order=3]] {{'Map.put()' is invoked with same key.}} - } - } - - void bar(Map map, String key) { - Object value = map.get(key); // Noncompliant {{Replace this "Map.get()" and condition with a call to "Map.computeIfPresent()".}} - if (value != null) { - map.put(key, new Object()); - } - } - - void del(Map map, String key) { - Object value = map.get(key); // Noncompliant - FP? - Preconditions.checkState(value == null, "Value should always be null!"); - map.put(key, new Object()); - } - - void thr(Map map, String key) { - Object value = map.get(key); // Noncompliant - FP? - if (value == null) { - throw new IllegalStateException("Value should always be null!"); - } - map.put(key, new Object()); - } - - void qix(Map map, String key) { - Object value = map.get(key); // Noncompliant - if (Objects.isNull(value)) { - map.put(key, new Object()); - doSomething(value); // required to keep 'value' alive and be able to retrieve constraint... - } - } - - void tmp(Map map1, Map map2, String key) { - Object value = map1.get(key); // Compliant - different maps - if (value == null) { - map2.put(key, new Object()); - } - } - - void gul(Map map, String key1, String key2) { - Object value = map.get(key1); // Compliant - different keys - if (value == null) { - map.put(key2, new Object()); - } - } - - void gro(Map map, String key) { - Object value = map.get(key); // Noncompliant - reassigning everything - Object value2 = value; - if (value2 == null) { - String key2 = key; - Map map2 = map; - map2.put(key2, new Object()); - } - } - - void til(Map map, String key) { - Object value = map.get(key); // Compliant - no constraint on value - map.put(key, new Object()); - } - - void lol(Map map, String key1, String key2) { - Object value = map.get(key1); // Noncompliant - if (value == null) { - map.get(key2); - map.put(key1, new Object()); - } - } - - void sal(Map map, String key) { - Object value = map.get(key); // Compliant - you can reach put with NULL and NOT_NULL constraint on 'value' - if (value == null) { - doSomething(); - } - map.put(key, new Object()); - doSomething(value); - } - - void dbl(Map map, String key1, String key2) { - Object value = map.get(key1); // Compliant - you can reach each put with NULL and NOT_NULL constraint on 'value' - if (value == null) { - map.put(key1, new Object()); - } else { - map.put(key1, new Object()); - } - doSomething(value); - - // Noncompliant@+1 [[flows=computeIfPresent]] {{Replace this "Map.get()" and condition with a call to "Map.computeIfPresent()".}} - value = map.get(key2); // flow@computeIfPresent {{'Map.get()' is invoked.}} - if (value == null) { // flow@computeIfPresent {{Implies 'value' is not null.}} - map.put(key1, new Object()); - } else { - map.put(key2, new Object()); // flow@computeIfPresent {{'Map.put()' is invoked with same key.}} - } - doSomething(value); - } - - void nmp(MyMap map, String key) { - Object value; - - value = map.get(); // Compliant - not the targeted 'put' and 'get' methods - if (value == null) { - map.put(new Object()); - } - - value = map.get(key); // Compliant - not the targeted 'put' method - if (value == null) { - map.put(new Object()); - } - - value = map.get(); // Compliant - not the targeted 'get' method - if (value == null) { - map.put(key, new Object()); - } - } - - abstract void doSomething(Object... objects); - -} - -abstract class MyMap implements Map { - - public V get() { - return null; - } - - public void put(Object o) { - // do nothing - } -} diff --git a/java-frontend/src/test/java/org/sonar/java/se/ExplodedGraphWalkerTest.java b/java-frontend/src/test/java/org/sonar/java/se/ExplodedGraphWalkerTest.java index 96800282c23..88ac12bc262 100644 --- a/java-frontend/src/test/java/org/sonar/java/se/ExplodedGraphWalkerTest.java +++ b/java-frontend/src/test/java/org/sonar/java/se/ExplodedGraphWalkerTest.java @@ -22,6 +22,7 @@ import com.google.common.reflect.ClassPath; import org.junit.Test; + import org.sonar.java.cfg.CFG; import org.sonar.java.resolve.SemanticModel; import org.sonar.java.se.checks.BooleanGratuitousExpressionsCheck; @@ -30,7 +31,6 @@ import org.sonar.java.se.checks.DivisionByZeroCheck; import org.sonar.java.se.checks.InvariantReturnCheck; import org.sonar.java.se.checks.LocksNotUnlockedCheck; -import org.sonar.java.se.checks.MapComputeIfAbsentOrPresentCheck; import org.sonar.java.se.checks.NonNullSetToNullCheck; import org.sonar.java.se.checks.NullDereferenceCheck; import org.sonar.java.se.checks.OptionalGetBeforeIsPresentCheck; @@ -409,8 +409,7 @@ public void eg_walker_factory_default_checks() throws IOException { ConditionalUnreachableCodeCheck.class, BooleanGratuitousExpressionsCheck.class, InvariantReturnCheck.class, - StreamNotConsumedCheck.class, - MapComputeIfAbsentOrPresentCheck.class + StreamNotConsumedCheck.class ) .map(Class::getSimpleName) .collect(Collectors.toSet()); diff --git a/java-frontend/src/test/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheckTest.java b/java-frontend/src/test/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheckTest.java deleted file mode 100644 index a2f5b110c8b..00000000000 --- a/java-frontend/src/test/java/org/sonar/java/se/checks/MapComputeIfAbsentOrPresentCheckTest.java +++ /dev/null @@ -1,30 +0,0 @@ -/* - * SonarQube Java - * Copyright (C) 2012-2017 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.java.se.checks; - -import org.junit.Test; -import org.sonar.java.se.JavaCheckVerifier; - -public class MapComputeIfAbsentOrPresentCheckTest { - @Test - public void test() { - JavaCheckVerifier.verify("src/test/files/se/MapComputeIfAbsentOrPresentCheck.java", new MapComputeIfAbsentOrPresentCheck()); - } -}