From e0416ac3b18626744f2f0f1b936a0eb70881e7e6 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Mon, 11 Jul 2022 16:15:25 -0700 Subject: [PATCH] Adding error message for unreachable code --- .../config/UserFacingErrorMessages.java | 14 +++++++++++ .../build/AbstractApexVertexBuilder.java | 8 +------ .../ApexStandardLibraryVertexBuilder.java | 3 +-- .../salesforce/graph/build/GremlinUtil.java | 24 +++++++++++++++++++ .../graph/build/MethodPathBuilderVisitor.java | 12 ++++++++-- .../graph/build/MethodPathBuilderTest.java | 22 +++++++++++++++++ 6 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java new file mode 100644 index 000000000..7d94730d4 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java @@ -0,0 +1,14 @@ +package com.salesforce.config; + +/** + * Contains error message constants that will be displayed to users. TODO: move all other + * user-facing messages here. + */ +public final class UserFacingErrorMessages { + + /** UserActionException * */ + + // format: filename,defined type, line number + public static final String UNREACHABLE_CODE = + "Please remove unreachable code to proceed with analysis: %s,%s:%d"; +} diff --git a/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java index ddf737c4b..30ee78a57 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java @@ -1,12 +1,10 @@ package com.salesforce.graph.build; -import com.google.common.collect.ImmutableSet; import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.apex.jorje.JorjeNode; import com.salesforce.collections.CollectionUtil; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.Schema; -import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -27,10 +25,6 @@ @SuppressWarnings("PMD.AbstractClassWithoutAbstractMethod") abstract class AbstractApexVertexBuilder { private static final Logger LOGGER = LogManager.getLogger(AbstractApexVertexBuilder.class); - protected static final Set ROOT_VERTICES = - ImmutableSet.builder() - .addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES)) - .build(); protected final GraphTraversalSource g; protected AbstractApexVertexBuilder(GraphTraversalSource g) { @@ -66,7 +60,7 @@ private void buildVertices(JorjeNode node, Vertex vNodeParam, String fileName) { // The engine assumes that only certain types of nodes can be roots. We need to enforce // that assumption and // fail noisily if it's violated. - if (!ROOT_VERTICES.contains(vNode.label())) { + if (!GremlinUtil.ROOT_VERTICES.contains(vNode.label())) { throw new UnexpectedException("Unexpected root vertex of type " + vNode.label()); } vNode.property(Schema.FILE_NAME, fileName); diff --git a/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java index 8486cd02f..141ad1e71 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java @@ -51,8 +51,7 @@ protected Object adjustPropertyValue(JorjeNode node, String key, Object value) { Object result = value; final boolean isRootVertexNameName = - AbstractApexVertexBuilder.ROOT_VERTICES.contains(node.getLabel()) - && key.equals(Schema.NAME); + GremlinUtil.ROOT_VERTICES.contains(node.getLabel()) && key.equals(Schema.NAME); if (isRootVertexNameName || key.equals(Schema.DEFINING_TYPE)) { result = getFullName(currentPackage, (String) value); if (LOGGER.isTraceEnabled()) { diff --git a/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java b/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java index a34fc84eb..ffbfdec16 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java @@ -1,14 +1,19 @@ package com.salesforce.graph.build; +import com.google.common.collect.ImmutableSet; +import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.Schema; +import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import org.apache.tinkerpop.gremlin.process.traversal.Order; import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -17,6 +22,11 @@ * and labels. */ public final class GremlinUtil { + protected static final Set ROOT_VERTICES = + ImmutableSet.builder() + .addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES)) + .build(); + public static Long getId(Map vertexProperties) { return (Long) vertexProperties.get(T.id); } @@ -105,5 +115,19 @@ public static Optional getParent(GraphTraversalSource g, Vertex vertex) } } + public static String getFileName(GraphTraversalSource g, Vertex vertex) { + if (ROOT_VERTICES.contains(vertex.label())) { + return vertex.value(Schema.FILE_NAME); + } + List verticesWithFileName = + g.V(vertex).repeat(__.in(Schema.CHILD)).until(__.has(Schema.FILE_NAME)).toList(); + + if (verticesWithFileName.isEmpty()) { + return "UNKNOWN_FILENAME"; + } + + return verticesWithFileName.get(0).value(Schema.FILE_NAME); + } + private GremlinUtil() {} } diff --git a/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java b/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java index 5d7ba6c86..247b811e5 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java +++ b/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java @@ -2,8 +2,10 @@ import com.google.common.collect.ImmutableSet; import com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.config.UserFacingErrorMessages; import com.salesforce.exception.TodoException; import com.salesforce.exception.UnexpectedException; +import com.salesforce.exception.UserActionException; import com.salesforce.graph.Schema; import com.salesforce.graph.vertex.SFVertexFactory; import java.util.ArrayList; @@ -165,7 +167,7 @@ private void _visit(Vertex vertex, Vertex parent, boolean lastChild) { currentInnerScope.pop(); nextVertexInOuterScope.pop(); } - } catch (TodoException | UnexpectedException ex) { + } catch (UserActionException | TodoException | UnexpectedException ex) { throw ex; } catch (Exception ex) { throw new UnexpectedException(g, vertex, ex); @@ -592,7 +594,13 @@ private void addEdgeFromPreviousSibling(Vertex vertex) { private void addEdge(String name, Vertex from, Vertex to) { if (NodeType.TERMINAL_VERTEX_LABELS.contains(from.label())) { - throw new UnexpectedException("Vertex is terminal. vertex=" + vertexToString(from)); + // Ask user to fix unreachable code + throw new UserActionException( + String.format( + UserFacingErrorMessages.UNREACHABLE_CODE, + GremlinUtil.getFileName(g, to), + to.value(Schema.DEFINING_TYPE), + to.value(Schema.BEGIN_LINE))); } if (LOGGER.isTraceEnabled()) { diff --git a/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java b/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java index ddef8934d..cbddbcc22 100644 --- a/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java +++ b/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java @@ -3,16 +3,19 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.core.IsEqual.equalTo; import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.hamcrest.core.IsNot.not; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import com.salesforce.TestUtil; import com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.exception.UserActionException; import com.salesforce.graph.ApexPath; import com.salesforce.graph.Schema; import com.salesforce.graph.ops.ApexPathUtil; @@ -2736,6 +2739,25 @@ public void testMethodSwitchStatement() { MatcherAssert.assertThat(paths, hasSize(equalTo(3))); } + @Test + public void testUnreachableCodeDetection() { + final String sourceCode = + "public class MyClass {\n" + + " String doSomething() {\n" + + " return 'hello';\n" + + " System.debug('This line is unreachable');\n" + + " }\n" + + "}\n"; + + UserActionException thrown = + assertThrows( + UserActionException.class, + () -> GraphBuildTestUtil.buildGraph(g, sourceCode), + "UserActionException should've been thrown before this point"); + + MatcherAssert.assertThat(thrown.getMessage(), containsString("MyClass:4")); + } + /** * Asserts that the number of {@link Schema#CFG_PATH} edges matches the number of vertex pairs * provided and that the edges match.