Skip to content

Commit

Permalink
Fixing spotbugs#3159 - Method call graph ignores constructors and non…
Browse files Browse the repository at this point in the history
…-parametric void methods (spotbugs#3160)

* Add failing test (spotbugs#3159)

* Call graph must include also constuctors and other non-parametric void methods. All these methods are missing to be properly sorted by TopologicalSort (spotbugs#3159)

* Fix test class to be package private (spotbugs#3159)

* Simplify. The private static method cannot be called from other places anyway (spotbugs#3159)

* Isolate test data into an inner class (spotbugs#3159)

* Refactor test class to use @beforeeach and @AfterEach (spotbugs#3159)

* Another test for testing a shortcut in the call graph (spotbugs#3159)

* Update CHANGELOG.md (spotbugs#3159)

---------

Co-authored-by: Jeremy Landis <jeremylandis@hotmail.com>
  • Loading branch information
topolik and hazendaz authored Oct 31, 2024
1 parent 28e4cdf commit 38c88f9
Show file tree
Hide file tree
Showing 4 changed files with 198 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
- Recognize some classes as immutable, fixing EI_EXPOSE and MS_EXPOSE FPs ([#3137](https://github.com/spotbugs/spotbugs/pull/3137))
- Do not report UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR for fields initialized in method annotated with TestNG's @BeforeClass. ([#3152](https://github.com/spotbugs/spotbugs/issues/3152))
- Fixed detector `FindReturnRef` not finding references exposed from nested and inner classes ([#2042](https://github.com/spotbugs/spotbugs/issues/2042))
- Fix call graph, include non-parametric void methods ([#3160](https://github.com/spotbugs/spotbugs/pull/3160))

### Cleanup
- Cleanup thread issue and regex issue in test-harness ([#3130](https://github.com/spotbugs/spotbugs/issues/3130))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package edu.umd.cs.findbugs.classfile;

import edu.umd.cs.findbugs.*;
import edu.umd.cs.findbugs.ba.AnalysisContext;
import edu.umd.cs.findbugs.ba.ClassContext;
import edu.umd.cs.findbugs.classfile.impl.ClassFactory;
import org.apache.bcel.classfile.Method;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;

/**
* @author Tomas Polesovsky
*/
class MethodsCallOrderTest {

private PrintingBugReporter bugReporter;
private IClassFactory classFactory;
private IClassPath classPath;

@BeforeEach
void setUp() {
bugReporter = new PrintingBugReporter();
classFactory = ClassFactory.instance();
classPath = classFactory.createClassPath();

IAnalysisCache analysisCache = classFactory.createAnalysisCache(classPath, bugReporter);
Global.setAnalysisCacheForCurrentThread(analysisCache);
FindBugs2.registerBuiltInAnalysisEngines(analysisCache);

Project project = new Project();
AnalysisContext analysisContext = new AnalysisContext(project);
AnalysisContext.setCurrentAnalysisContext(analysisContext);
}

@AfterEach
void tearDown() {
Global.setAnalysisCacheForCurrentThread(null);
}

void load(String codeBaseLocation) throws CheckedAnalysisException, IOException, InterruptedException {
IClassPathBuilder builder = classFactory.createClassPathBuilder(bugReporter);
builder.addCodeBase(classFactory.createFilesystemCodeBaseLocator(codeBaseLocation), true);
builder.build(classPath, new NoOpFindBugsProgress());
}

@Test
void testSimpleTest() {
try {
load("../spotbugsTestCases/build/classes/java/main/MethodsCallOrder$SimpleTest.class");
} catch (CheckedAnalysisException | IOException | InterruptedException e) {
fail("Unable to add MethodsCallOrder$SimpleTest.class to the build: " + e.getMessage(), e);
}

ClassDescriptor classDescriptor = DescriptorFactory.createClassDescriptor("MethodsCallOrder$SimpleTest");

List<String> actual = new ArrayList<>();

try {
ClassContext classContext = Global.getAnalysisCache().getClassAnalysis(ClassContext.class, classDescriptor);
for (Method method : classContext.getMethodsInCallOrder()) {
actual.add(method.getName() + method.getSignature());
}
} catch (CheckedAnalysisException e) {
fail("Unable to get ClassContext analysis for MethodsCallOrder: " + e.getMessage(), e);
}

List<String> expected = Arrays.asList(new String[] {
"method4()V",
"method3()V",
"method2()V",
"method1()V",
"<init>(Ljava/lang/Object;)V",
"<init>()V",
"staticMethod4()V",
"staticMethod3()V",
"staticMethod2()V",
"staticMethod1()V",
"<clinit>()V" });

assertEquals(expected, actual);
}

@Test
void testShortcutPathTest() {
try {
load("../spotbugsTestCases/build/classes/java/main/MethodsCallOrder$ShortcutPathTest.class");
} catch (CheckedAnalysisException | IOException | InterruptedException e) {
fail("Unable to add MethodsCallOrder$SimpleTest.class to the build: " + e.getMessage(), e);
}

ClassDescriptor classDescriptor = DescriptorFactory.createClassDescriptor("MethodsCallOrder$ShortcutPathTest");

List<String> actual = new ArrayList<>();

try {
ClassContext classContext = Global.getAnalysisCache().getClassAnalysis(ClassContext.class, classDescriptor);
for (Method method : classContext.getMethodsInCallOrder()) {
actual.add(method.getName() + method.getSignature());
}
} catch (CheckedAnalysisException e) {
fail("Unable to get ClassContext analysis for MethodsCallOrder: " + e.getMessage(), e);
}

List<String> expected = Arrays.asList(new String[] {
"methodC()V",
"methodB()V",
"methodA()V",
"<init>()V"
});

assertEquals(expected, actual);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@
*/
public class SelfMethodCalls {

private static boolean interestingSignature(String signature) {
return !"()V".equals(signature);
}

public static <T> MultiMap<T, T> getSelfCalls(final ClassDescriptor classDescriptor, final Map<String, T> methods) {
final MultiMap<T, T> map = new MultiMap<>(HashSet.class);

Expand All @@ -62,7 +58,7 @@ public MethodVisitor visitMethod(final int access, final String name, final Stri
return new MethodVisitor(FindBugsASM.ASM_VERSION) {
@Override
public void visitMethodInsn(int opcode, String owner, String name2, String desc2, boolean itf) {
if (owner.equals(classDescriptor.getClassName()) && interestingSignature(desc2)) {
if (owner.equals(classDescriptor.getClassName())) {
T from = methods.get(name + desc + ((access & Opcodes.ACC_STATIC) != 0));
T to = methods.get(name2 + desc2 + (opcode == Opcodes.INVOKESTATIC));
map.add(from, to);
Expand Down
74 changes: 74 additions & 0 deletions spotbugsTestCases/src/java/MethodsCallOrder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* @author Tomas Polesovsky
*/
public class MethodsCallOrder {

static class SimpleTest {
static {
staticMethod1();
}

public static void staticMethod1() {
staticMethod2();
}

public static void staticMethod2() {
staticMethod3();
}

public static void staticMethod3() {
staticMethod4();
}

public static void staticMethod4() {
new MethodsCallOrder.SimpleTest();
}

public SimpleTest() {
this(null);
}

public SimpleTest(Object o) {
method1();
}

public void method1() {
method2();
}

public void method2() {
method3();
}

public void method3() {
method4();
}

public void method4() {
System.out.println("HERE WE GO!");
}
}

static class ShortcutPathTest {
public ShortcutPathTest() {
methodA();
}

public void methodA(){
if (System.currentTimeMillis() % 2 == 1) {
methodB();
} else {
methodC();
}
}

public void methodB(){
methodC();
}

public void methodC(){
System.out.println("HERE WE GO!");
}
}

}

0 comments on commit 38c88f9

Please sign in to comment.