Merge pull request #394 from Luro02/main

Implement some things
Feuermagier · Jan 23, 2024 · 05b5dc3 · 05b5dc3
2 parents c4d73aa + 8246890
commit 05b5dc3
Show file tree

Hide file tree

Showing 32 changed files with 1,532 additions and 88 deletions.
diff --git a/autograder-core/pom.xml b/autograder-core/pom.xml
@@ -99,6 +99,23 @@
             </exclusions>
         </dependency>
 
+        <dependency>
+            <groupId>fr.inria.gforge.spoon</groupId>
+            <artifactId>spoon-javadoc</artifactId>
+            <version>${spoon.version}</version>
+            <exclusions>
+                <exclusion>
+                    <groupId>org.slf4j</groupId>
+                    <artifactId>slf4j-api</artifactId>
+                </exclusion>
+                <!-- exclude jdt, because spoon is not using the latest version -->
+                <exclusion>
+                    <groupId>org.eclipse.jdt</groupId>
+                    <artifactId>org.eclipse.jdt.core</artifactId>
+                </exclusion>
+            </exclusions>
+        </dependency>
+
 
         <!-- PMD -->
         <dependency>

diff --git a/...grader-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java b/...grader-core/src/main/java/de/firemage/autograder/core/check/api/MathReimplementation.java
@@ -46,6 +46,14 @@ private static boolean isMathSqrt(CtInvocation<?> ctInvocation) {
             && SpoonUtil.isSignatureEqualTo(ctInvocation.getExecutable(), double.class, "sqrt", double.class);
     }
 
+    private static boolean isPowSqrt(CtInvocation<?> ctInvocation) {
+        return isMathPow(ctInvocation)
+            && ctInvocation.getArguments().size() == 2
+            && SpoonUtil.resolveConstant(ctInvocation.getArguments().get(1)) instanceof CtLiteral<?> ctLiteral
+            && ctLiteral.getValue() instanceof Double doubleValue
+            && doubleValue == 0.5;
+    }
+
     private static Optional<CtExpression<?>> getPow2(CtExpression<?> ctExpression) {
         if (ctExpression instanceof CtBinaryOperator<?> ctBinaryOperator
             && ctBinaryOperator.getLeftHandOperand().equals(ctBinaryOperator.getRightHandOperand())
@@ -64,12 +72,12 @@ && isMathPow(ctInvocation)
         return Optional.empty();
     }
 
-    private void checkHypot(CtExpression<?> ctExpression) {
+    private boolean checkHypot(CtExpression<?> ctExpression) {
         if (!(ctExpression instanceof CtInvocation<?> ctInvocation)
-            || !isMathSqrt(ctInvocation)
+            || (!isMathSqrt(ctInvocation) && !isPowSqrt(ctInvocation))
             || !(ctInvocation.getArguments().get(0) instanceof CtBinaryOperator<?> ctBinaryOperator)
             || ctBinaryOperator.getKind() != BinaryOperatorKind.PLUS) {
-            return;
+            return false;
         }
 
         Optional<CtExpression<?>> left = getPow2(ctBinaryOperator.getLeftHandOperand());
@@ -84,17 +92,19 @@ private void checkHypot(CtExpression<?> ctExpression) {
                 ),
                 ProblemType.COMMON_REIMPLEMENTATION_HYPOT
             );
+
+            return true;
         }
+
+        return false;
     }
 
     private void checkSqrt(CtExpression<?> ctExpression) {
         if (!(ctExpression instanceof CtInvocation<?> ctInvocation) || !isMathPow(ctInvocation)) {
             return;
         }
 
-        if (SpoonUtil.resolveCtExpression(ctInvocation.getArguments().get(1)) instanceof CtLiteral<?> ctLiteral
-            && ctLiteral.getValue() instanceof Double doubleValue
-            && doubleValue == 0.5) {
+        if (isPowSqrt(ctInvocation)) {
             addLocalProblem(
                 ctExpression,
                 new LocalizedMessage(
@@ -155,6 +165,12 @@ private void checkMaxMin(CtIf ctIf) {
             return;
         }
 
+        // check that in `if (variable <op> max) { variable = assigned; }` the max is the same as the assigned value
+        if (!condition.getLeftHandOperand().equals(thenAssignment.getAssignment())
+            && !condition.getRightHandOperand().equals(thenAssignment.getAssignment())) {
+            return;
+        }
+
         // max looks like this:
         // if (variable < max) {
         //     variable = max;
@@ -219,8 +235,10 @@ protected void enter(CtElement ctElement) {
                 if (ctElement instanceof CtExpression<?> ctExpression
                     && !ctExpression.isImplicit()
                     && ctExpression.getPosition().isValidPosition()) {
-                    checkSqrt(ctExpression);
-                    checkHypot(ctExpression);
+                    // only check for sqrt if hypot is not applicable
+                    if (!checkHypot(ctExpression)) {
+                        checkSqrt(ctExpression);
+                    }
                 }
 
                 super.enter(ctElement);

diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseFormatString.java
@@ -258,6 +258,6 @@ public void process(CtExpression<String> ctExpression) {
 
     @Override
     public Optional<Integer> maximumProblems() {
-        return Optional.of(5);
+        return Optional.of(1);
     }
 }
diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseStringFormatted.java
@@ -5,6 +5,7 @@
 import de.firemage.autograder.core.check.ExecutableCheck;
 import de.firemage.autograder.core.dynamic.DynamicAnalysis;
 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.CtExpression;
@@ -16,14 +17,16 @@
 import java.util.Map;
 import java.util.Optional;
 
-@ExecutableCheck(reportedProblems = { ProblemType.USE_STRING_FORMATTED })
+@ExecutableCheck(reportedProblems = {ProblemType.USE_STRING_FORMATTED})
 public class UseStringFormatted extends IntegratedCheck {
     private void checkCtInvocation(CtInvocation<?> ctInvocation) {
         boolean hasInvokedStringFormat = ctInvocation.getTarget() instanceof CtTypeAccess<?> ctTypeAccess
-               // ensure the method is called on java.lang.String
-               && ctInvocation.getFactory().Type().createReference(java.lang.String.class)
-                              .equals(ctTypeAccess.getAccessedType())
-               && ctInvocation.getExecutable().getSimpleName().equals("format");
+            // ensure the method is called on java.lang.String
+            && SpoonUtil.isTypeEqualTo(ctTypeAccess.getAccessedType(), java.lang.String.class)
+            && ctInvocation.getExecutable().getSimpleName().equals("format")
+            && !ctInvocation.getArguments().isEmpty()
+            // ensure the first argument is a string (this ignores String.format(Locale, String, Object...))
+            && SpoonUtil.isTypeEqualTo(ctInvocation.getArguments().get(0).getType(), java.lang.String.class);
 
         if (!hasInvokedStringFormat) {
             return;
@@ -59,6 +62,6 @@ public void process(CtInvocation<?> ctInvocation) {
 
     @Override
     public Optional<Integer> maximumProblems() {
-        return Optional.of(4);
+        return Optional.of(1);
     }
 }
diff --git a/...ader-core/src/main/java/de/firemage/autograder/core/check/comment/ThrowsJavadocCheck.java b/...ader-core/src/main/java/de/firemage/autograder/core/check/comment/ThrowsJavadocCheck.java
@@ -33,7 +33,7 @@ private void checkCtExecutable(CtExecutable<?> ctExecutable) {
         CtJavaDoc ctJavaDoc = doc.get();
 
         Set<String> documentedExceptions = ctJavaDoc.getTags().stream()
-                .filter(tag -> tag.getType() == CtJavaDocTag.TagType.THROWS && tag.getParam() != null)
+                .filter(tag -> (tag.getType() == CtJavaDocTag.TagType.THROWS || tag.getType() == CtJavaDocTag.TagType.EXCEPTION) && tag.getParam() != null)
                 .map(CtJavaDocTag::getParam)
                 .collect(Collectors.toSet());
 

diff --git a/...er-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java b/...er-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java
@@ -8,10 +8,12 @@
 import de.firemage.autograder.core.integrated.SpoonUtil;
 import de.firemage.autograder.core.integrated.StaticAnalysis;
 import spoon.reflect.code.CtComment;
+import spoon.reflect.code.CtExpression;
 import spoon.reflect.code.CtInvocation;
 import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtReturn;
 import spoon.reflect.code.CtStatement;
+import spoon.reflect.code.CtSwitchExpression;
 import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.visitor.CtScanner;
 
@@ -29,6 +31,10 @@ private boolean isAllowedStatement(CtStatement ctStatement) {
         return ctStatement instanceof CtComment;
     }
 
+    private boolean isComplexExpression(CtExpression<?> ctExpression) {
+        return ctExpression instanceof CtSwitchExpression<?,?>;
+    }
+
     private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVariableRead) {
         if (// the variable must be a local variable
             !(ctVariableRead.getVariable().getDeclaration() instanceof CtLocalVariable<?> ctLocalVariable)
@@ -39,6 +45,11 @@ private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVari
             return;
         }
 
+        if (ctLocalVariable.getDefaultExpression() != null
+            && this.isComplexExpression(ctLocalVariable.getDefaultExpression())) {
+            return;
+        }
+
         CtStatement previousStatement = SpoonUtil.getPreviousStatement(ctStatement).orElse(null);
 
         while (!ctLocalVariable.equals(previousStatement) && this.isAllowedStatement(previousStatement)) {