Detect redundant ctors with higher visibility

Correctly handles protected classes as a special case where a public
constructor is actually useful. Also corrently detects qualified super
constructor invocations as not redundant.

autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantConstructorCheck.java

@@ -10,8 +10,16 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import spoon.processing.AbstractProcessor;
-import spoon.reflect.code.*;
-import spoon.reflect.declaration.*;
+import spoon.reflect.code.CtAssignment;
+import spoon.reflect.code.CtBlock;
+import spoon.reflect.code.CtFieldWrite;
+import spoon.reflect.code.CtInvocation;
+import spoon.reflect.code.CtVariableRead;
+import spoon.reflect.declaration.CtClass;
+import spoon.reflect.declaration.CtConstructor;
+import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.CtField;
+import spoon.reflect.declaration.CtRecord;
 import spoon.reflect.reference.CtParameterReference;
 import spoon.reflect.reference.CtTypeReference;
 
@@ -37,7 +45,7 @@ public void process(CtClass<?> element) {
                     var canonicalCtor = record.getConstructor(types);
 
                     if (!canonicalCtor.isImplicit()
-                        && hasDefaultVisibility(record, canonicalCtor)
+                        && hasEffectivelyDefaultVisibility(record, canonicalCtor)
                         && isDefaultBodyRecord(record, canonicalCtor)) {
                         redundantCtor = canonicalCtor;
                     }
@@ -48,7 +56,7 @@ && isDefaultBodyRecord(record, canonicalCtor)) {
 
                     if (!ctor.isImplicit()
                         && ctor.getParameters().isEmpty()
-                        && hasDefaultVisibility(element, ctor)
+                        && hasEffectivelyDefaultVisibility(element, ctor)
                         && isDefaultBody(ctor.getBody())
                         && ctor.getThrownTypes().isEmpty()) {
                         redundantCtor = ctor;
@@ -61,9 +69,22 @@ && isDefaultBody(ctor.getBody())
         });
     }
 
-    private boolean hasDefaultVisibility(CtClass<?> type, CtConstructor<?> ctor) {
+    /**
+     * Checks if the constructor visibility is effectively the same as the class
+     * visibility.
+     * <p>
+     * A constructor with higher visibility than the containing class is only ever useful
+     * with a protected (inner) class, because it can allow the class to be instantiated
+     * from a different package.
+     */
+    private boolean hasEffectivelyDefaultVisibility(CtClass<?> type, CtConstructor<?> ctor) {
         // enum constructors are always private
-        return type.isEnum() || ctor.getVisibility() == type.getVisibility();
+        if (type.isEnum() || type.isPrivate()) return true;
+
+        if (type.isPublic()) return ctor.isPublic();
+        else if (type.isProtected()) return ctor.isProtected();
+        // package-access class, only private is smaller
+        else return !ctor.isPrivate();
     }
 
     private boolean isDefaultBody(CtBlock<?> block) {
@@ -74,9 +95,12 @@ private boolean isDefaultBody(CtBlock<?> block) {
             // A constructor invocation is either this or super.
             // Because we know we are analyzing the body of a no-args constructor, it
             // cannot be a recursive this() call, but has to be a redundant super() call.
+            // If the target is not null it is a qualified super invocation, which is
+            // required and not redundant.
             .allMatch(statement -> statement instanceof CtInvocation<?> invocation
                 && invocation.getExecutable().isConstructor()
-                && invocation.getArguments().isEmpty());
+                && invocation.getArguments().isEmpty()
+                && invocation.getTarget() == null);
     }
 
     private boolean isDefaultBodyRecord(CtRecord record, CtConstructor<?> ctor) {

autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestRedundantConstructor.java

@@ -34,7 +34,34 @@ public Test() {
     }
 
     @Test
-    void testRedundantConstructorSuper() throws IOException, LinterException {
+    void testRedundantHigherVisibility() throws IOException, LinterException {
+        assertRedundant(
+            "Test",
+            """
+                class Test {
+                    public Test() {
+                    }
+                }
+                """
+        );
+    }
+
+    @Test
+    void testRedundantHigherVisibilityInner() throws IOException, LinterException {
+        assertRedundant(
+            "Outer",
+            """
+                public class Outer {
+                    private class Inner {
+                        Inner() {}
+                    }
+                }
+                """
+        );
+    }
+
+    @Test
+    void testRedundantExplicitSuper() throws IOException, LinterException {
         assertRedundant(
             "Test",
             """
@@ -63,6 +90,21 @@ public Inner() {
         );
     }
 
+    @Test
+    void testRedundantConstructorStaticInner() throws IOException, LinterException {
+        assertRedundant(
+            "Test",
+            """
+                public class Test {
+                    private static class Inner {
+                        private Inner() {
+                        }
+                    }
+                }
+                """
+        );
+    }
+
     @Test
     void testRedundantEnumConstructor() throws IOException, LinterException {
         assertRedundant(
@@ -212,6 +254,19 @@ public Test() {
         );
     }
 
+    @Test
+    void testNotRedundantThrows() throws IOException, LinterException {
+        assertNotRedundant(
+            "Test",
+            """
+                public class Test {
+                    public Test() throws java.io.IOException {
+                    }
+                }
+                """
+        );
+    }
+
     @Test
     void testNotRedundantConstructorSuperArgs() throws IOException, LinterException {
         assertRedundant(
@@ -236,6 +291,58 @@ public Child() {
         );
     }
 
+    @Test
+    void testNotRedundantQualifiedSuper() throws IOException, LinterException {
+        // taken from JLS example 8.8.7.1-2
+        assertRedundant(
+            StringSourceInfo.fromSourceStrings(JavaVersion.JAVA_17, Map.of(
+                "Outer",
+                """
+                    class Outer {
+                        class Inner {}
+                    }
+                    """,
+                "Child",
+                """
+                    class Child extends Outer.Inner {
+                        Child() {
+                            (new Outer()).super();
+                        }
+                    }
+                    """
+            )),
+            false
+        );
+    }
+
+    @Test
+    void testNotRedundantConstructorProtectedInnerClass() throws IOException, LinterException {
+        // Taken from JLS example 8.8.9-2
+        assertRedundant(
+            StringSourceInfo.fromSourceStrings(JavaVersion.JAVA_17, Map.of(
+                "a.Outer",
+                """
+                    package a;
+                    public class Outer {
+                        protected class Inner {
+                            public Inner() {}
+                        }
+                    }
+                    """,
+                "b.Child",
+                """
+                    package b;
+                    public class Child extends a.Outer {
+                        void foo() {
+                            new Inner();
+                        }
+                    }
+                    """
+            )),
+            false
+        );
+    }
+
     private void assertRedundant(String className, String source) throws LinterException, IOException {
         assertRedundant(className, source, true);
     }