Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Common static analysis issues #35

Merged
merged 1 commit into from
Oct 21, 2023
Merged

refactor: Common static analysis issues #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/main/java/org/openrewrite/analysis/InvocationMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,8 @@ public boolean isSelect(Cursor cursor) {
public boolean isAnyArgument(Cursor cursor) {
return asExpression(
cursor,
expression -> nearestMethodCall(cursor).map(call -> {
return call.getArguments().contains(expression)
&& matcher.matches(call); // Do the matcher.matches(...) last as this can be expensive
knutwannheden marked this conversation as resolved.
Show resolved Hide resolved
}).orElse(false)
expression -> nearestMethodCall(cursor).map(call -> call.getArguments().contains(expression)
&& matcher.matches(call)).orElse(false)
);
}

Expand Down
37 changes: 17 additions & 20 deletions src/main/java/org/openrewrite/analysis/controlflow/ControlFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,11 @@ public J.Block visitBlock(J.Block block, P p) {
}

@Override
public J.Synchronized visitSynchronized(J.Synchronized _sync, P p) {
public J.Synchronized visitSynchronized(J.Synchronized sync, P p) {
addCursorToBasicBlock();
visit(_sync.getLock(), p);
visit(_sync.getBody(), p);
return _sync;
visit(sync.getLock(), p);
visit(sync.getBody(), p);
return sync;
}

@Override
Expand Down Expand Up @@ -353,20 +353,17 @@ public J.Case visitCase(J.Case _case, P p) {
ControlFlowNode.ConditionNode conditionNode = currentAsBasicBlock().addConditionNodeTruthFirst();
current = Stream.concat(Stream.of(conditionNode), caseFlow.stream()).collect(Collectors.toSet());
caseFlow.clear();
switch (_case.getType()) {
case Statement:
if (_case.getStatements().isEmpty()) {
// Visit a fake empty statement to ensure that the ConditionNode has a true successor
visit(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY), p);
} else {
visitStatementList(_case.getStatements(), p);
}
break;
case Rule:
visit(_case.getBody(), p);
breakFlow.add(currentAsBasicBlock());
current = Collections.emptySet();
break;
Comment on lines -356 to -369
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why what you have is better?

if (_case.getType() == J.Case.Type.Statement) {
if (_case.getStatements().isEmpty()) {
// Visit a fake empty statement to ensure that the ConditionNode has a true successor
visit(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY), p);
} else {
visitStatementList(_case.getStatements(), p);
}
} else if (_case.getType() == J.Case.Type.Rule) {
visit(_case.getBody(), p);
breakFlow.add(currentAsBasicBlock());
current = Collections.emptySet();
}
caseFlow.addAll(current);
current = Collections.singleton(conditionNode);
Expand All @@ -393,7 +390,7 @@ public J.Case visitCase(J.Case _case, P p) {
private static Set<ControlFlowNode.ConditionNode> allAsConditionNodesMissingTruthFirst(Set<? extends ControlFlowNode> nodes) {
return nodes.stream().map(controlFlowNode -> {
if (controlFlowNode instanceof ControlFlowNode.ConditionNode) {
return ((ControlFlowNode.ConditionNode) controlFlowNode);
return (ControlFlowNode.ConditionNode) controlFlowNode;
} else {
return controlFlowNode.addConditionNodeTruthFirst();
}
Expand All @@ -403,7 +400,7 @@ private static Set<ControlFlowNode.ConditionNode> allAsConditionNodesMissingTrut
private static Set<ControlFlowNode.ConditionNode> allAsConditionNodesMissingFalseFirst(Set<? extends ControlFlowNode> nodes) {
return nodes.stream().map(controlFlowNode -> {
if (controlFlowNode instanceof ControlFlowNode.ConditionNode) {
return ((ControlFlowNode.ConditionNode) controlFlowNode);
return (ControlFlowNode.ConditionNode) controlFlowNode;
} else {
return controlFlowNode.addConditionNodeFalseFirst();
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/analysis/controlflow/ControlFlowIllegalStateException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ static class Message {
Set<ControlFlowNode> predecessors;
Cursor cursor;

static class MessageBuilder {
static final class MessageBuilder {
private final String message;
// LinkedHashMap to preserve order of insertion1
private final LinkedHashMap<String, ControlFlowNode> nodes = new LinkedHashMap<>();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/analysis/controlflow/ControlFlowJavaPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ public J visitEmpty(J.Empty empty, PrintOutputCapture<P> pPrintOutputCapture) {
}

static class ControlFlowPrintOutputCapture<P> extends PrintOutputCapture<P> {
boolean enabled = false;
boolean enabled;

public ControlFlowPrintOutputCapture(P p) {
super(p);
Expand Down
13 changes: 6 additions & 7 deletions src/main/java/org/openrewrite/analysis/controlflow/ControlFlowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,10 @@ String toVisualizerString() {
// Self loathing
J.Case caseStatement = (J.Case) condition;
String caseString = caseStatement.toString();
switch (caseStatement.getType()) {
case Statement:
return caseString.substring(0, caseString.indexOf(":") + 1);
case Rule:
return caseString.substring(0, caseString.indexOf("->") + 2);
if (caseStatement.getType() == J.Case.Type.Statement) {
return caseString.substring(0, caseString.indexOf(":") + 1);
} else if (caseStatement.getType() == J.Case.Type.Rule) {
return caseString.substring(0, caseString.indexOf("->") + 2);
}
}
return condition.toString();
Expand Down Expand Up @@ -424,7 +423,7 @@ static final class Start extends ControlFlowNode implements GraphTerminator {
@Getter
private final GraphType graphType;

private ControlFlowNode successor = null;
private ControlFlowNode successor;

@Override
protected void _addSuccessorInternal(ControlFlowNode successor) {
Expand Down Expand Up @@ -470,7 +469,7 @@ public String toString() {
static final class End extends ControlFlowNode implements GraphTerminator {
@Getter
private final GraphType graphType;
private ControlFlowNode successor = null;
private ControlFlowNode successor;

@Override
Set<ControlFlowNode> getSuccessors() {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/openrewrite/analysis/controlflow/ControlFlowSummaryDotVisualizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public String visualizeAsDotfile(String name, boolean darkMode, ControlFlowSumma
.map(NodeToNodeText::new)
.sorted()
.collect(Collectors.toList());
int vizSrc = -1, vizSink = -1;
int vizSrc = -1;
int vizSink = -1;
for (int i = 0; i < nodeToNodeText.size(); i++) {
NodeToNodeText toNodeText = nodeToNodeText.get(i);
ControlFlowNode node = toNodeText.node;
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/org/openrewrite/analysis/controlflow/ControlFlowVisualizationVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public J.Block visitBlock(J.Block block, P p) {
private static class ControlFlowMarkingVisitor<P> extends JavaIsoVisitor<P> {
private final String label;
private final Map<J, ? extends ControlFlowNode> nodeToBlock;
private int nodeNumber = 0;
private int nodeNumber;
private final Map<ControlFlowNode, Integer> nodeNumbers = new HashMap<>();

@Override
Expand Down Expand Up @@ -128,7 +128,9 @@ public Statement visitStatement(Statement statement, P p) {
} else {
return SearchResult.found(statement, "" + number + label);
}
} else return statement;
} else {
return statement;
}
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/analysis/controlflow/Guard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ private static Optional<JavaType> getTypeSafe(Cursor c, Expression e) {
}
} else if (e instanceof J.MethodInvocation) {
J.MethodInvocation methodInvocation = (J.MethodInvocation) e;
if (methodInvocation.getSimpleName().equals("equals")) {
if ("equals".equals(methodInvocation.getSimpleName())) {
return Optional.of(JavaType.Primitive.Boolean);
}
}
Expand Down
20 changes: 7 additions & 13 deletions src/main/java/org/openrewrite/analysis/dataflow/ExternalFlowModels.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ boolean isAdditionalTaintStep(
}

@AllArgsConstructor
final static class OptimizedFlowModels {
static final class OptimizedFlowModels {
private final Map<AdditionalFlowStepPredicate, Set<FlowModel>> value;
private final Map<AdditionalFlowStepPredicate, Set<FlowModel>> taint;

Expand Down Expand Up @@ -243,32 +243,26 @@ private Map<AdditionalFlowStepPredicate, Set<FlowModel>> optimize(Collection<Flo
flowFromArgumentIndexToReturn
.entrySet()
.stream()
.map(entry -> {
return new PredicateToFlowModels(forFlowFromArgumentIndexToReturn(
.map(entry -> new PredicateToFlowModels(forFlowFromArgumentIndexToReturn(
entry.getKey(),
entry.getValue()
), entry.getValue());
});
), entry.getValue()));
Stream<PredicateToFlowModels> flowFromArgumentIndexToQualifierStream =
flowFromArgumentIndexToQualifier
.entrySet()
.stream()
.map(entry -> {
return new PredicateToFlowModels(forFlowFromArgumentIndexToQualifier(
.map(entry -> new PredicateToFlowModels(forFlowFromArgumentIndexToQualifier(
entry.getKey(),
entry.getValue()
), entry.getValue());
});
), entry.getValue()));
Stream<PredicateToFlowModels> flowFromArgumentIndexToArgumentIndexStream =
flowFromArgumentIndexToArgumentIndex
.entrySet()
.stream()
.map(entry -> {
return new PredicateToFlowModels(forFlowFromArgumentIndexToArgumentIndex(
.map(entry -> new PredicateToFlowModels(forFlowFromArgumentIndexToArgumentIndex(
entry.getKey(),
entry.getValue()
), entry.getValue());
});
), entry.getValue()));

Stream<PredicateToFlowModels> s1 = Stream.concat(flowFromArgumentIndexToReturnStream, flowFromArgumentIndexToQualifierStream);
return Stream.concat(s1, flowFromArgumentIndexToArgumentIndexStream)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/analysis/search/FindFlowBetweenMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public GlobalDataFlow.Accumulator getInitialValue(ExecutionContext ctx) {


String flow = this.flow == null ? "Data" : this.flow;
if (flow.equals("Taint")) {
if ("Taint".equals(flow)) {
return GlobalDataFlow.accumulator(new TaintFlowSpec() {

@Override
Expand Down
8 changes: 6 additions & 2 deletions src/main/java/org/openrewrite/analysis/trait/Top.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ public interface Top {
UUID getId();

static boolean equals(Top e1, @Nullable Object other) {
if (e1 == other) return true;
if (other == null || e1.getClass() != other.getClass()) return false;
if (e1 == other) {
return true;
}
if (other == null || e1.getClass() != other.getClass()) {
return false;
}
Top e2 = (Top) other;
return e1.getId().equals(e2.getId());
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/analysis/trait/member/StaticInitializerMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* A static initializer is a method that contains all static
* field initializations and static initializer blocks.
*/
public class StaticInitializerMethod extends InitializerMethodBase {
public final class StaticInitializerMethod extends InitializerMethodBase {
private StaticInitializerMethod(Cursor cursor) {
super(cursor);
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/openrewrite/analysis/dataflow/FindLocalFlowPathsStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ public boolean isSource(DataFlowNode srcNode) {
.map("42"::equals)
.orSome(() -> srcNode
.asExpr(MethodAccess.class)
.map(ma -> ma.getSimpleName()
.equals("source"))
.map(ma -> "source"
.equals(ma.getSimpleName()))
.orSome(false));
}

Expand All @@ -70,7 +70,7 @@ public boolean isSink(DataFlowNode sinkNode) {
@Override
public boolean isBarrierGuard(Guard guard, boolean branch) {
if (guard.getExpression() instanceof J.MethodInvocation) {
return branch && ((J.MethodInvocation) guard.getExpression()).getName().getSimpleName().equals("guard");
return branch && "guard".equals(((J.MethodInvocation) guard.getExpression()).getName().getSimpleName());
}
return super.isBarrierGuard(guard, branch);
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/openrewrite/analysis/dataflow/ParameterDataflowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public void defaults(RecipeSpec spec) {
public boolean isSource(DataFlowNode srcNode) {
return srcNode
.asParameter()
.map(p -> p.getName().equals("source"))
.map(p -> "source".equals(p.getName()))
.orSome(false);
}

Expand Down
10 changes: 5 additions & 5 deletions src/test/resources/dataflow-functional-tests/ArchiveAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public class ArchiveAnalyzer extends AbstractFileTypeAnalyzer {
/**
* The parent directory for the individual directories per archive.
*/
private File tempFileLocation = null;
private File tempFileLocation;
/**
* The max scan depth that the analyzer will recursively extract nested
* archives.
Expand All @@ -93,7 +93,7 @@ public class ArchiveAnalyzer extends AbstractFileTypeAnalyzer {
/**
* The file filter used to filter supported files.
*/
private FileFilter fileFilter = null;
private FileFilter fileFilter;
/**
* The set of things we can handle with Zip methods
*/
Expand Down Expand Up @@ -315,7 +315,7 @@ private void extractAndAnalyze(Dependency dependency, Engine engine, int scanDep
extractAndAnalyze(d, engine, scanDepth + 1);
}
} else {
dependencySet.stream().filter((sub) -> sub.getFilePath().startsWith(tmpDir.getAbsolutePath())).forEach((sub) -> {
dependencySet.stream().filter(sub -> sub.getFilePath().startsWith(tmpDir.getAbsolutePath())).forEach(sub -> {
final String displayPath = String.format("%s%s",
dependency.getFilePath(),
sub.getActualFilePath().substring(tmpDir.getAbsolutePath().length()));
Expand Down Expand Up @@ -361,13 +361,13 @@ private void addDisguisedJarsToDependencies(Dependency dependency, Engine engine
org.apache.commons.io.FileUtils.copyFile(dependency.getActualFile(), tmpLoc);
final List<Dependency> dependencySet = findMoreDependencies(engine, tmpLoc);
if (dependencySet != null && !dependencySet.isEmpty()) {
dependencySet.forEach((d) -> {
dependencySet.forEach(d -> {
//fix the dependency's display name and path
if (d.getActualFile().equals(tmpLoc)) {
d.setFilePath(dependency.getFilePath());
d.setDisplayFileName(dependency.getFileName());
} else {
d.getRelatedDependencies().stream().filter((rel) -> rel.getActualFile().equals(tmpLoc)).forEach((rel) -> {
d.getRelatedDependencies().stream().filter(rel -> rel.getActualFile().equals(tmpLoc)).forEach(rel -> {
rel.setFilePath(dependency.getFilePath());
rel.setDisplayFileName(dependency.getFileName());
});
Expand Down
20 changes: 8 additions & 12 deletions src/test/resources/dataflow-functional-tests/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,13 +256,13 @@ public static String byteCountToDisplaySize(long size) {
String displaySize;

if (size / ONE_GB > 0) {
displaySize = String.valueOf(size / ONE_GB) + " GB";
displaySize = size / ONE_GB + " GB";
} else if (size / ONE_MB > 0) {
displaySize = String.valueOf(size / ONE_MB) + " MB";
displaySize = size / ONE_MB + " MB";
} else if (size / ONE_KB > 0) {
displaySize = String.valueOf(size / ONE_KB) + " KB";
displaySize = size / ONE_KB + " KB";
} else {
displaySize = String.valueOf(size) + " bytes";
displaySize = size + " bytes";
}
return displaySize;
}
Expand Down Expand Up @@ -376,7 +376,7 @@ public static Collection<File> listFiles(
}

//Find files
Collection<File> files = new java.util.LinkedList<File>();
Collection<File> files = new java.util.LinkedList<>();
innerListFiles(files, directory,
FileFilterUtils.or(effFileFilter, effDirFilter));
return files;
Expand Down Expand Up @@ -615,7 +615,7 @@ public static File[] toFiles(URL[] urls) {
for (int i = 0; i < urls.length; i++) {
URL url = urls[i];
if (url != null) {
if (url.getProtocol().equals("file") == false) {
if ("file".equals(url.getProtocol()) == false) {
throw new IllegalArgumentException(
"URL could not be converted to a File: " + url);
}
Expand Down Expand Up @@ -1046,7 +1046,7 @@ public static void copyDirectory(File srcDir, File destDir,
if (canonicalDestDir.startsWith(srcDir.getCanonicalPath())) {
File[] srcFiles = filter == null ? srcDir.listFiles() : srcDir.listFiles(filter);
if (srcFiles != null && srcFiles.length > 0) {
exclusionList = new ArrayList<String>(srcFiles.length);
exclusionList = new ArrayList<>(srcFiles.length);
for (File srcFile : srcFiles) {
File copiedFile = new File(destDir, srcFile.getName());
exclusionList.add(copiedFile.getCanonicalPath());
Expand Down Expand Up @@ -2206,10 +2206,6 @@ public static boolean isSymlink(File file) throws IOException {
fileInCanonicalDir = new File(canonicalDir, file.getName());
}

if (fileInCanonicalDir.getCanonicalFile().equals(fileInCanonicalDir.getAbsoluteFile())) {
return false;
} else {
return true;
}
return !(fileInCanonicalDir.getCanonicalFile().equals(fileInCanonicalDir.getAbsoluteFile()));
}
}
Loading
Loading