Merge pull request #312 from Luro02/main

Implement some things
Feuermagier · Nov 26, 2023 · ee758c2 · ee758c2
2 parents 1723a84 + f9c2b8e
commit ee758c2
Show file tree

Hide file tree

Showing 6 changed files with 177 additions and 33 deletions.
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java b/autograder-core/src/main/java/de/firemage/autograder/core/CodeModel.java
@@ -25,6 +25,7 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.List;
 import java.util.Optional;
@@ -144,7 +145,13 @@ private void buildModelMaybe() {
             launcher.getEnvironment().setComplianceLevel(this.file.getVersion().getVersionNumber());
             // The encoding might differ by file
             launcher.getEnvironment().setEncodingProvider(
-                (spoonFile, fileBytes) -> this.file.getCompilationUnit(Path.of(spoonFile.getPath())).charset()
+                (spoonFile, fileBytes) -> {
+                    try {
+                        return this.file.getCompilationUnit(Path.of(spoonFile.getPath())).charset();
+                    } catch (Exception e) {
+                        return StandardCharsets.UTF_8;
+                    }
+                }
             );
             Environment environment = launcher.getEnvironment();
 

diff --git a/...main/java/de/firemage/autograder/core/check/complexity/RedundantVariableBeforeReturn.java b/...main/java/de/firemage/autograder/core/check/complexity/RedundantVariableBeforeReturn.java
@@ -7,12 +7,13 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
-import spoon.processing.AbstractProcessor;
 import spoon.reflect.code.CtComment;
+import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtReturn;
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.code.CtVariableRead;
+import spoon.reflect.visitor.CtScanner;
 
 import java.util.Map;
 
@@ -28,48 +29,73 @@ private boolean isAllowedStatement(CtStatement ctStatement) {
         return ctStatement instanceof CtComment;
     }
 
+    private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVariableRead) {
+        if (// the variable must be a local variable
+            !(ctVariableRead.getVariable().getDeclaration() instanceof CtLocalVariable<?> ctLocalVariable)
+            // it should not have any annotations (e.g. @SuppressWarnings("unchecked"))
+            || !ctLocalVariable.getAnnotations().isEmpty()
+            // the variable must only be used in the return statement
+            || SpoonUtil.findUsesOf(ctLocalVariable).size() != 1) {
+            return;
+        }
+
+        CtStatement previousStatement = SpoonUtil.getPreviousStatement(ctStatement).orElse(null);
+
+        while (!ctLocalVariable.equals(previousStatement) && this.isAllowedStatement(previousStatement)) {
+            previousStatement = SpoonUtil.getPreviousStatement(previousStatement).orElse(null);
+        }
+
+        if (previousStatement == null) {
+            return;
+        }
+
+        if (previousStatement.equals(ctLocalVariable)) {
+            this.addLocalProblem(
+                ctStatement,
+                new LocalizedMessage(
+                    "redundant-variable",
+                    Map.of(
+                        "name", ctLocalVariable.getSimpleName(),
+                        "value", ctLocalVariable.getDefaultExpression().prettyprint()
+                    )
+                ),
+                ProblemType.REDUNDANT_VARIABLE_BEFORE_RETURN
+            );
+        }
+    }
 
     @Override
     protected void check(StaticAnalysis staticAnalysis, DynamicAnalysis dynamicAnalysis) {
-        staticAnalysis.processWith(new AbstractProcessor<CtReturn<?>>() {
+        staticAnalysis.getModel().getRootPackage().accept(new CtScanner() {
             @Override
-            public void process(CtReturn<?> ctReturn) {
-                if (!ctReturn.getPosition().isValidPosition()
-                    || ctReturn.isImplicit()
-                    // only check returns with a variable
-                    || !(ctReturn.getReturnedExpression() instanceof CtVariableRead<?> ctVariableRead)
-                    // the variable must be a local variable
-                    || !(ctVariableRead.getVariable().getDeclaration() instanceof CtLocalVariable<?> ctLocalVariable)
-                    // it should not have any annotations (e.g. @SuppressWarnings("unchecked"))
-                    || !ctLocalVariable.getAnnotations().isEmpty()
-                    // the variable must only be used in the return statement
-                    || SpoonUtil.findUsesOf(ctLocalVariable).size() != 1) {
+            public <T> void visitCtInvocation(CtInvocation<T> ctInvocation) {
+                if (!ctInvocation.getPosition().isValidPosition()
+                    || ctInvocation.isImplicit()
+                    // only check invocations with a single variable
+                    || ctInvocation.getArguments().size() != 1
+                    || !(ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead)) {
+                    super.visitCtInvocation(ctInvocation);
                     return;
                 }
 
-                CtStatement previousStatement = SpoonUtil.getPreviousStatement(ctReturn).orElse(null);
+                checkVariableRead(ctInvocation, ctVariableRead);
 
-                while (!ctLocalVariable.equals(previousStatement) && isAllowedStatement(previousStatement)) {
-                    previousStatement = SpoonUtil.getPreviousStatement(previousStatement).orElse(null);
-                }
+                super.visitCtInvocation(ctInvocation);
+            }
 
-                if (previousStatement == null) {
+            @Override
+            public <T> void visitCtReturn(CtReturn<T> ctReturn) {
+                if (!ctReturn.getPosition().isValidPosition()
+                    || ctReturn.isImplicit()
+                    // only check returns with a variable
+                    || !(ctReturn.getReturnedExpression() instanceof CtVariableRead<?> ctVariableRead)) {
+                    super.visitCtReturn(ctReturn);
                     return;
                 }
 
-                if (previousStatement.equals(ctLocalVariable)) {
-                    addLocalProblem(
-                        ctReturn,
-                        new LocalizedMessage(
-                            "redundant-variable",
-                            Map.of(
-                                "name", ctLocalVariable.getSimpleName(),
-                                "value", ctLocalVariable.getDefaultExpression().prettyprint()
-                            )
-                        ),
-                        ProblemType.REDUNDANT_VARIABLE_BEFORE_RETURN
-                    );
-                }
+                checkVariableRead(ctReturn, ctVariableRead);
+
+                super.visitCtReturn(ctReturn);
             }
         });
     }

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
@@ -7,6 +7,7 @@
 import de.firemage.autograder.core.integrated.IntegratedCheck;
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
+import spoon.reflect.code.CtLambda;
 import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.declaration.CtConstructor;
 import spoon.reflect.declaration.CtElement;
@@ -106,7 +107,7 @@ public <T> void visitCtConstructor(CtConstructor<T> ctConstructor) {
 
             @Override
             public <T> void visitCtParameter(CtParameter<T> ctParameter) {
-                if (SpoonUtil.isInOverriddenMethod(ctParameter) || SpoonUtil.isInMainMethod(ctParameter)) {
+                if (SpoonUtil.isInOverriddenMethod(ctParameter) || SpoonUtil.isInMainMethod(ctParameter) || ctParameter.getParent() instanceof CtLambda<?>) {
                     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
@@ -21,6 +21,7 @@
 import spoon.reflect.code.CtCase;
 import spoon.reflect.code.CtComment;
 import spoon.reflect.code.CtConstructorCall;
+import spoon.reflect.code.CtExecutableReferenceExpression;
 import spoon.reflect.code.CtExpression;
 import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtJavaDoc;
@@ -1040,6 +1041,17 @@ public boolean matches(CtAbstractInvocation<?> invocation) {
         }
     }
 
+    private record ExecutableReferenceExpressionFilter(CtExecutable<?> executable) implements Filter<CtExecutableReferenceExpression<?, ?>> {
+        @Override
+        public boolean matches(CtExecutableReferenceExpression<?, ?> expression) {
+            CtExecutableReference<?> invocationExecutable = expression.getExecutable();
+            return invocationExecutable.equals(this.executable.getReference())
+                || this.executable.equals(invocationExecutable.getExecutableDeclaration())
+                // TODO: consider removing this?
+                || invocationExecutable.isOverriding(this.executable.getReference());
+        }
+    }
+
     public static class UsesFilter implements Filter<CtElement> {
         private final Filter<CtElement> filter;
 
@@ -1080,6 +1092,17 @@ private static Filter<CtElement> buildExecutableFilter(CtExecutable<?> ctExecuta
                 (Class<CtAbstractInvocation<?>>) (Object) CtAbstractInvocation.class
             );
 
+            filter = new CompositeFilter<>(
+                FilteringOperator.UNION,
+                filter,
+                new FilterAdapter<>(
+                    // this filter finds all lambdas that reference the executable:
+                    // someMethod(MyClass::executableName)
+                    new ExecutableReferenceExpressionFilter(ctExecutable),
+                    (Class<CtExecutableReferenceExpression<?, ?>>) (Object) CtExecutableReferenceExpression.class
+                )
+            );
+
             if (ctExecutable instanceof CtMethod<?> ctMethod) {
                 // implementing an abstract method is considered a use:
                 filter = new CompositeFilter<>(

diff --git a/.../java/de/firemage/autograder/core/check/complexity/TestRedundantVariableBeforeReturn.java b/.../java/de/firemage/autograder/core/check/complexity/TestRedundantVariableBeforeReturn.java
@@ -354,4 +354,24 @@ public String get() {
 
         problems.assertExhausted();
     }
+
+    @Test
+    void testRedundantPrintln() throws IOException, LinterException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString(
+            JavaVersion.JAVA_17,
+            "Test",
+            """
+                public class Test {
+                    public static void main(String[] args) {
+                        int a = 5;
+                        System.out.println(a);
+                    }
+                }
+                """
+        ), PROBLEM_TYPES);
+
+        assertEqualsRedundant(problems.next(), "a", "5");
+
+        problems.assertExhausted();
+    }
 }
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
@@ -499,4 +499,71 @@ public static void main(String[] args) {
 
         problems.assertExhausted();
     }
+
+    @Test
+    void testUsedImplicitLambda() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "Main",
+                    """
+                    import java.util.List;
+
+                    public class Main {
+                        public Main(String string) {
+                            System.out.println(string);
+                        }
+
+                        private static String identity(String value) {
+                            return value;
+                        }
+
+                        public static void main(String[] args) {
+                            List<Main> result = List.of("Hello", "World")
+                                .stream()
+                                .map(Main::identity)
+                                .map(Main::new)
+                                .toList();
+
+                            System.out.println(result);
+                        }
+                    }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
+
+    @Test
+    void testInevitablyUnusedLambdaParam() throws LinterException, IOException {
+        ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceStrings(
+            JavaVersion.JAVA_17,
+            Map.ofEntries(
+                Map.entry(
+                    "Main",
+                    """
+                    import java.util.Map;
+
+                    public class Main {
+                        private static void foo() {
+                            Map.of("Hello", "World").computeIfPresent("Hello", (key, value) -> {
+                                //                                              ^^^ unused, but there is no way to avoid it
+                                return value + "!";
+                            });
+                        }
+
+                        public static void main(String[] args) {
+                            foo();
+                        }
+                    }
+                    """
+                )
+            )
+        ), PROBLEM_TYPES);
+
+        problems.assertExhausted();
+    }
 }