fix bug for unused code detection #393

Feuermagier · Jan 23, 2024 · acf5f55 · acf5f55
1 parent bbce095
commit acf5f55
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 28 deletions.
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UnusedImport.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/UnusedImport.java
@@ -19,8 +19,6 @@
 import spoon.reflect.declaration.CtImportKind;
 import spoon.reflect.declaration.CtPackage;
 import spoon.reflect.declaration.CtType;
-import spoon.reflect.reference.CtExecutableReference;
-import spoon.reflect.reference.CtFieldReference;
 import spoon.reflect.reference.CtPackageReference;
 import spoon.reflect.reference.CtReference;
 import spoon.reflect.reference.CtTypeReference;
@@ -108,28 +106,8 @@ public List<CtReference> visitReference(JavadocReference reference) {
         }
     }
 
-    private static CtElement getReferenceDeclaration(CtReference ctReference) {
-        // this might be null if the reference is not in the source path
-        // for example, when the reference points to a java.lang type
-        CtElement target = ctReference.getDeclaration();
-
-        if (target == null && ctReference instanceof CtTypeReference<?> ctTypeReference) {
-            target = ctTypeReference.getTypeDeclaration();
-        }
-
-        if (target == null && ctReference instanceof CtExecutableReference<?> ctExecutableReference) {
-            target = ctExecutableReference.getExecutableDeclaration();
-        }
-
-        if (target == null && ctReference instanceof CtFieldReference<?> ctFieldReference) {
-            target = ctFieldReference.getFieldDeclaration();
-        }
-
-        return target;
-    }
-
     private static boolean isReferencingTheSameElement(CtReference left, CtReference right) {
-        return left.equals(right) || Objects.equals(getReferenceDeclaration(left), getReferenceDeclaration(right));
+        return left.equals(right) || Objects.equals(SpoonUtil.getReferenceDeclaration(left), SpoonUtil.getReferenceDeclaration(right));
     }
 
     @SuppressWarnings("unchecked")
@@ -171,7 +149,7 @@ private void checkImport(CtImport ctImport, CtCompilationUnit ctCompilationUnit,
             return;
         }
 
-        CtElement element = getReferenceDeclaration(ctImport.getReference());
+        CtElement element = SpoonUtil.getReferenceDeclaration(ctImport.getReference());
 
         // types from the same package are imported implicitly
         //

diff --git a/...e/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java b/...e/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java
@@ -12,6 +12,7 @@
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtField;
+import spoon.reflect.declaration.CtInterface;
 import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtModifiable;
 import spoon.reflect.declaration.CtNamedElement;
@@ -49,6 +50,14 @@ private void checkUnused(StaticAnalysis staticAnalysis, CtNamedElement ctElement
         if (isUnused) {
             // do not report unused elements if there is no main method in the model and the element is accessible
             // (i.e. not private)
+            if (ctElement instanceof CtParameter<?>
+                && ctElement.getParent() instanceof CtTypeMember ctTypeMember
+                && !ctTypeMember.getDeclaringType().isPrivate()
+                // check if there is no main method in the model
+                && !staticAnalysis.getCodeModel().hasMainMethod()) {
+                return;
+            }
+
             if (ctElement instanceof CtModifiable ctModifiable
                 && !ctModifiable.isPrivate()
                 && ctModifiable instanceof CtTypeMember ctTypeMember
@@ -85,7 +94,8 @@ public <T> void visitCtLocalVariable(CtLocalVariable<T> ctLocalVariable) {
 
             @Override
             public <T> void visitCtMethod(CtMethod<T> ctMethod) {
-                if (SpoonUtil.isOverriddenMethod(ctMethod) || SpoonUtil.isMainMethod(ctMethod)) {
+                if (SpoonUtil.isOverriddenMethod(ctMethod)
+                    || SpoonUtil.isMainMethod(ctMethod)) {
                     super.visitCtMethod(ctMethod);
                     return;
                 }
@@ -107,7 +117,10 @@ public <T> void visitCtConstructor(CtConstructor<T> ctConstructor) {
 
             @Override
             public <T> void visitCtParameter(CtParameter<T> ctParameter) {
-                if (SpoonUtil.isInOverriddenMethod(ctParameter) || SpoonUtil.isInMainMethod(ctParameter) || ctParameter.getParent() instanceof CtLambda<?>) {
+                if (SpoonUtil.isInOverriddenMethod(ctParameter)
+                    || SpoonUtil.isInMainMethod(ctParameter)
+                    || ctParameter.getParent() instanceof CtLambda<?>
+                    || ctParameter.getParent(CtInterface.class) != null) {
                     super.visitCtParameter(ctParameter);
                     return;
                 }

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java b/autograder-core/src/main/java/de/firemage/autograder/core/integrated/SpoonUtil.java
@@ -1046,6 +1046,26 @@ public boolean matches(CtElement element) {
     }
 
 
+    public static CtElement getReferenceDeclaration(CtReference ctReference) {
+        // this might be null if the reference is not in the source path
+        // for example, when the reference points to a java.lang type
+        CtElement target = ctReference.getDeclaration();
+
+        if (target == null && ctReference instanceof CtTypeReference<?> ctTypeReference) {
+            target = ctTypeReference.getTypeDeclaration();
+        }
+
+        if (target == null && ctReference instanceof CtExecutableReference<?> ctExecutableReference) {
+            target = ctExecutableReference.getExecutableDeclaration();
+        }
+
+        if (target == null && ctReference instanceof CtFieldReference<?> ctFieldReference) {
+            target = ctFieldReference.getFieldDeclaration();
+        }
+
+        return target;
+    }
+
     private static final Filter<CtElement> EXPLICIT_ELEMENT_FILTER = ctElement -> !ctElement.isImplicit();
 
     /**
@@ -1058,8 +1078,7 @@ public boolean matches(CtElement element) {
     private record BetterVariableAccessFilter<T extends CtVariableAccess<?>>(CtVariable<?> ctVariable) implements Filter<T> {
         @Override
         public boolean matches(T element) {
-            return element.getVariable().equals(this.ctVariable.getReference())
-                || (element.getVariable().getDeclaration() != null && element.getVariable().getDeclaration().equals(this.ctVariable));
+            return getReferenceDeclaration(element.getVariable()) != null && getReferenceDeclaration(element.getVariable()) == this.ctVariable;
         }
     }
 

diff --git a/...c/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java b/...c/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java
@@ -595,4 +595,76 @@ public class MyException extends RuntimeException {
 
         problems.assertExhausted();
     }
+
+    @Test
+    void testUnusedInterfaceParameter() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "Main",
+                    """
+                    public class Main implements A {
+                        public void a(String string) {
+                            System.out.println("Hello");
+                        }
+
+                        public static void main(String[] args) {
+                            Main main = new Main();
+                            main.a("World");
+                        }
+                    }
+                    """
+                ),
+                Map.entry(
+                    "A",
+                    """
+                    public interface A {
+                        void a(String string);
+                    }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testUnusedParameterWhenMethodIsUsed() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "Main",
+                    """
+                    public class Main {
+                        public void b(String parameterName) {
+                            B b = new B();
+                            b.b(parameterName);
+                        }
+
+                        public static void main(String[] args) {
+                            Main main = new Main();
+                            // main.a("World");
+                            main.b("");
+                        }
+                    }
+                    """
+                ),
+                Map.entry(
+                    "B",
+                    """
+                    public class B {
+                        void b(String parameterName) {}
+                    }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        assertEqualsUnused("parameterName", problems.next());
+
+        problems.assertExhausted();
+    }
 }