From 2ef62ab856804e51a7bf3a2b5ce93fd110a5ed40 Mon Sep 17 00:00:00 2001 From: Michael Gumowski Date: Tue, 19 Sep 2017 18:26:44 +0200 Subject: [PATCH] SONARJAVA-2473 Ignore exception messages when computing flows for S2095, S2222 and S3546 (#1686) --- .../org/sonar/java/se/FlowComputation.java | 79 +++++++++++-------- .../checks/CustomUnclosedResourcesCheck.java | 3 +- .../java/se/checks/LocksNotUnlockedCheck.java | 2 +- .../se/checks/UnclosedResourcesCheck.java | 2 +- ...esourcesCheckWithoutExceptionMessages.java | 26 ++++++ .../se/checks/UnclosedResourcesCheckTest.java | 5 ++ 6 files changed, 83 insertions(+), 34 deletions(-) create mode 100644 java-frontend/src/test/files/se/UnclosedResourcesCheckWithoutExceptionMessages.java diff --git a/java-frontend/src/main/java/org/sonar/java/se/FlowComputation.java b/java-frontend/src/main/java/org/sonar/java/se/FlowComputation.java index c7f81e4c203..6a5d921d0e2 100644 --- a/java-frontend/src/main/java/org/sonar/java/se/FlowComputation.java +++ b/java-frontend/src/main/java/org/sonar/java/se/FlowComputation.java @@ -85,13 +85,15 @@ public class FlowComputation { private final Predicate terminateTraversal; private final Set symbolicValues; private final List> domains; + private final boolean skipExceptionMessages; private FlowComputation(Set symbolicValues, Predicate addToFlow, - Predicate terminateTraversal, List> domains) { + Predicate terminateTraversal, List> domains, boolean skipExceptionMessages) { this.addToFlow = addToFlow; this.terminateTraversal = terminateTraversal; this.symbolicValues = symbolicValues; this.domains = domains; + this.skipExceptionMessages = skipExceptionMessages; } private static Set computedFrom(@Nullable SymbolicValue symbolicValue) { @@ -106,8 +108,40 @@ private static Set computedFrom(@Nullable SymbolicValue symbolicV // FIXME It is assumed that this will always return set with at least one element, which could be empty, because result is consumed in other places and messages are // added to returned flows. This design should be improved. - public static Set flow(ExplodedGraph.Node currentNode, Set symbolicValues, Predicate addToFlow, - Predicate terminateTraversal, List> domains, Set symbols) { + public static Set flow(ExplodedGraph.Node currentNode, Set symbolicValues, Predicate addToFlow, Predicate terminateTraversal, + List> domains, Set symbols) { + return flow(currentNode, symbolicValues, addToFlow, terminateTraversal, domains, symbols, false); + } + + public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, List> domains) { + return flow(currentNode, setFromNullable(currentVal), constraint -> true, c -> false, domains, Collections.emptySet(), false); + } + + public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, List> domains, @Nullable Symbol trackSymbol) { + return flow(currentNode, setFromNullable(currentVal), c -> true, c -> false, domains, setFromNullable(trackSymbol), false); + } + + public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, List> domains) { + return flow(currentNode, setFromNullable(currentVal), addToFlow, c -> false, domains, Collections.emptySet(), false); + } + + public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, Predicate terminateTraversal, + List> domains) { + return flow(currentNode, setFromNullable(currentVal), addToFlow, terminateTraversal, domains, Collections.emptySet(), false); + } + + public static Set flowWithoutExceptions(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, + List> domains) { + return flow(currentNode, setFromNullable(currentVal), addToFlow, c -> false, domains, Collections.emptySet(), true); + } + + public static Set flowWithoutExceptions(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, + Predicate terminateTraversal, List> domains) { + return flow(currentNode, setFromNullable(currentVal), addToFlow, terminateTraversal, domains, Collections.emptySet(), true); + } + + private static Set flow(ExplodedGraph.Node currentNode, Set symbolicValues, Predicate addToFlow, + Predicate terminateTraversal, List> domains, Set symbols, boolean skipExceptionMessages) { Set allSymbolicValues = symbolicValues.stream() .map(FlowComputation::computedFrom) .flatMap(Set::stream) @@ -125,29 +159,10 @@ public static Set flow(ExplodedGraph.Node currentNode, Set } } } - FlowComputation flowComputation = new FlowComputation(allSymbolicValues, addToFlow, terminateTraversal, domains); + FlowComputation flowComputation = new FlowComputation(allSymbolicValues, addToFlow, terminateTraversal, domains, skipExceptionMessages); return flowComputation.run(currentNode, trackedSymbols); } - public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, List> domains) { - return flow(currentNode, currentVal, constraint -> true, domains); - } - - public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, List> domains, - @Nullable Symbol trackSymbol) { - return flow(currentNode, setFromNullable(currentVal), c -> true, c -> false, domains, setFromNullable(trackSymbol)); - } - - public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, - List> domains) { - return flow(currentNode, currentVal, addToFlow, c -> false, domains); - } - - public static Set flow(ExplodedGraph.Node currentNode, @Nullable SymbolicValue currentVal, Predicate addToFlow, - Predicate terminateTraversal, List> domains) { - return flow(currentNode, setFromNullable(currentVal), addToFlow, terminateTraversal, domains, Collections.emptySet()); - } - private static Set setFromNullable(@Nullable T val) { return val == null ? ImmutableSet.of() : ImmutableSet.of(val); } @@ -296,14 +311,16 @@ Stream addEdge(ExplodedGraph.Edge edge) { PSet newTrackSymbols = newTrackedSymbols(edge); SameConstraints newSameConstraints = newTrackSymbols == trackedSymbols ? sameConstraints : new SameConstraints(sameConstraints, newTrackSymbols); - flowFromThrownException(edge).ifPresent(loc -> { - flowBuilder.setAsExceptional(); - flowBuilder.add(loc); - }); - flowFromCaughtException(edge).ifPresent(loc -> { - flowBuilder.setAsExceptional(); - flowBuilder.add(loc); - }); + if (!skipExceptionMessages) { + flowFromThrownException(edge).ifPresent(loc -> { + flowBuilder.setAsExceptional(); + flowBuilder.add(loc); + }); + flowFromCaughtException(edge).ifPresent(loc -> { + flowBuilder.setAsExceptional(); + flowBuilder.add(loc); + }); + } Set learnedConstraints = learnedConstraints(edge); Flow lcFlow = flowFromLearnedConstraints(edge, filterRedundantObjectDomain(learnedConstraints)); diff --git a/java-frontend/src/main/java/org/sonar/java/se/checks/CustomUnclosedResourcesCheck.java b/java-frontend/src/main/java/org/sonar/java/se/checks/CustomUnclosedResourcesCheck.java index df1178cb845..5e18d36fe37 100644 --- a/java-frontend/src/main/java/org/sonar/java/se/checks/CustomUnclosedResourcesCheck.java +++ b/java-frontend/src/main/java/org/sonar/java/se/checks/CustomUnclosedResourcesCheck.java @@ -119,7 +119,8 @@ public void checkEndOfExecutionPath(CheckerContext context, ConstraintManager co private void processUnclosedSymbolicValue(ExplodedGraph.Node node, SymbolicValue sv) { List> domains = Lists.newArrayList(CustomResourceConstraint.class); - FlowComputation.flow(node, sv, OPENED::equals, domains).stream().flatMap(Flow::stream) + FlowComputation.flowWithoutExceptions(node, sv, OPENED::equals, domains).stream() + .flatMap(Flow::firstFlowLocation) .filter(location -> location.syntaxNode.is(Tree.Kind.NEW_CLASS, Tree.Kind.METHOD_INVOCATION)) .forEach(this::reportIssue); } diff --git a/java-frontend/src/main/java/org/sonar/java/se/checks/LocksNotUnlockedCheck.java b/java-frontend/src/main/java/org/sonar/java/se/checks/LocksNotUnlockedCheck.java index d20bcf2d69a..a1607822c01 100644 --- a/java-frontend/src/main/java/org/sonar/java/se/checks/LocksNotUnlockedCheck.java +++ b/java-frontend/src/main/java/org/sonar/java/se/checks/LocksNotUnlockedCheck.java @@ -184,7 +184,7 @@ public ProgramState checkPostStatement(CheckerContext context, Tree syntaxNode) public void checkEndOfExecutionPath(CheckerContext context, ConstraintManager constraintManager) { ExplodedGraph.Node node = context.getNode(); context.getState().getValuesWithConstraints(LockConstraint.LOCKED).stream() - .flatMap(lockedSv -> FlowComputation.flow(node, lockedSv, LockConstraint.LOCKED::equals, LockConstraint.UNLOCKED::equals, LOCK_CONSTRAINT_DOMAIN).stream()) + .flatMap(lockedSv -> FlowComputation.flowWithoutExceptions(node, lockedSv, LockConstraint.LOCKED::equals, LockConstraint.UNLOCKED::equals, LOCK_CONSTRAINT_DOMAIN).stream()) .flatMap(Flow::firstFlowLocation) .forEach(this::reportIssue); } diff --git a/java-frontend/src/main/java/org/sonar/java/se/checks/UnclosedResourcesCheck.java b/java-frontend/src/main/java/org/sonar/java/se/checks/UnclosedResourcesCheck.java index 6df2f0f104e..cf875b6da1e 100644 --- a/java-frontend/src/main/java/org/sonar/java/se/checks/UnclosedResourcesCheck.java +++ b/java-frontend/src/main/java/org/sonar/java/se/checks/UnclosedResourcesCheck.java @@ -165,7 +165,7 @@ private static Set symbolicValuesToReport(CheckerContext context) } private void processUnclosedSymbolicValue(ExplodedGraph.Node node, SymbolicValue sv) { - FlowComputation.flow(node, sv, OPEN::equals, RESOURCE_CONSTRAINT_DOMAIN).stream() + FlowComputation.flowWithoutExceptions(node, sv, OPEN::equals, RESOURCE_CONSTRAINT_DOMAIN).stream() .flatMap(Flow::firstFlowLocation) .filter(location -> location.syntaxNode.is(Tree.Kind.NEW_CLASS, Tree.Kind.METHOD_INVOCATION)) .forEach(this::reportIssue); diff --git a/java-frontend/src/test/files/se/UnclosedResourcesCheckWithoutExceptionMessages.java b/java-frontend/src/test/files/se/UnclosedResourcesCheckWithoutExceptionMessages.java new file mode 100644 index 00000000000..555e4643b22 --- /dev/null +++ b/java-frontend/src/test/files/se/UnclosedResourcesCheckWithoutExceptionMessages.java @@ -0,0 +1,26 @@ +package org.foo; + +import java.io.FileWriter; +import java.io.Writer; +import java.util.List; + +abstract class A { + void foo(List elements) { + for (A element : elements) { + try { + String packageName = substring(element.name(), "."); + + Writer writer = new FileWriter(""); // Noncompliant + + put("packageName", packageName); + + writer.close(); + } catch (Exception e) { + } + } + } + + abstract String substring(String str, String separator); + abstract String name(); + abstract void put(String s, Object o); +} diff --git a/java-frontend/src/test/java/org/sonar/java/se/checks/UnclosedResourcesCheckTest.java b/java-frontend/src/test/java/org/sonar/java/se/checks/UnclosedResourcesCheckTest.java index 7f928c914f7..aa838d4256f 100644 --- a/java-frontend/src/test/java/org/sonar/java/se/checks/UnclosedResourcesCheckTest.java +++ b/java-frontend/src/test/java/org/sonar/java/se/checks/UnclosedResourcesCheckTest.java @@ -68,4 +68,9 @@ public void test_value_as_string_for_open_resource_constraints() throws Exceptio public void test_streams() throws Exception { JavaCheckVerifier.verify("src/test/files/se/UnclosedResourcesCheckStreams.java", new UnclosedResourcesCheck()); } + + @Test + public void skip_exception_messages() throws Exception { + JavaCheckVerifier.verify("src/test/files/se/UnclosedResourcesCheckWithoutExceptionMessages.java", new UnclosedResourcesCheck()); + } }