From ce282072d8f8ae5590c814f1cffa513045733e89 Mon Sep 17 00:00:00 2001 From: Lucas <24826124+Luro02@users.noreply.github.com> Date: Sun, 26 Nov 2023 10:00:29 +0100 Subject: [PATCH 1/4] fix #285 --- .../main/java/de/firemage/autograder/core/CodeModel.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 index 1d1514e4..3d67cd5f 100644 --- 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(); From 4bc2ceffa6f074103b2b5c000f29ac28d14546b9 Mon Sep 17 00:00:00 2001 From: Lucas <24826124+Luro02@users.noreply.github.com> Date: Sun, 26 Nov 2023 11:03:03 +0100 Subject: [PATCH 2/4] fix #291 --- .../autograder/core/integrated/SpoonUtil.java | 23 ++++++++++++ .../TestUnusedCodeElementCheck.java | 37 +++++++++++++++++++ 2 files changed, 60 insertions(+) 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 index 4cca03d8..6dda541a 100644 --- 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> { + @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 { private final Filter filter; @@ -1080,6 +1092,17 @@ private static Filter buildExecutableFilter(CtExecutable ctExecuta (Class>) (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>) (Object) CtExecutableReferenceExpression.class + ) + ); + if (ctExecutable instanceof CtMethod ctMethod) { // implementing an abstract method is considered a use: filter = new CompositeFilter<>( diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java index ff9cee37..4d52dd68 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java @@ -499,4 +499,41 @@ 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
result = List.of("Hello", "World") + .stream() + .map(Main::identity) + .map(Main::new) + .toList(); + + System.out.println(result); + } + } + """ + ) + ) + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } } From 0c0eb8f41b485d384f63e7f67caa845516c191b5 Mon Sep 17 00:00:00 2001 From: Lucas <24826124+Luro02@users.noreply.github.com> Date: Sun, 26 Nov 2023 11:15:43 +0100 Subject: [PATCH 3/4] fix #309 --- .../unnecessary/UnusedCodeElementCheck.java | 3 +- .../TestUnusedCodeElementCheck.java | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java index 41f7a409..0b462fab 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/unnecessary/UnusedCodeElementCheck.java +++ b/autograder-core/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 void visitCtConstructor(CtConstructor ctConstructor) { @Override public void visitCtParameter(CtParameter 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/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java index 4d52dd68..be056d9b 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/unnecessary/TestUnusedCodeElementCheck.java @@ -536,4 +536,34 @@ public static void main(String[] args) { 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(); + } } From f9c2b8e98ca598e78fc0b7c4770cb6b872b4ffad Mon Sep 17 00:00:00 2001 From: Lucas <24826124+Luro02@users.noreply.github.com> Date: Sun, 26 Nov 2023 11:28:15 +0100 Subject: [PATCH 4/4] implement #310 --- .../RedundantVariableBeforeReturn.java | 88 ++++++++++++------- .../TestRedundantVariableBeforeReturn.java | 20 +++++ 2 files changed, 77 insertions(+), 31 deletions(-) diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariableBeforeReturn.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariableBeforeReturn.java index e2d26527..c95a331d 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariableBeforeReturn.java +++ b/autograder-core/src/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>() { + 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 void visitCtInvocation(CtInvocation 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 void visitCtReturn(CtReturn 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/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariableBeforeReturn.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariableBeforeReturn.java index e0cb9854..08e10c15 100644 --- a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantVariableBeforeReturn.java +++ b/autograder-core/src/test/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(); + } }