feuermagier / autograder Goto Github PK

This seems hard to detect, but there might be some good ways through spoon:

• get all assignments of a variable and check if only literals of the same type are assigned to it (note: see redundant array lint for how to compare ctliterals, because short s = 0 is doing an implicit cast, so the rhs is of type int, which may be problematic for comparison)
• in most cases the variable is read from stdin, so instead one could look for a closed set of possible values the program compares the variable to
• this can be switching over all possible values (maybe with constants, so a constant resolver would be nice, see #22)
• comparing it with an if would be another option

Example for obvious case:

private static final char[] VALID = {'R', 'x', 'X', '|', '\\', '/', '_', '*', ' '};

Potential false-positives

String weekday = "monday";
switch (weekday) {
  case "monday" -> return WeekDay.MONDAY,
  case "tuesday" -> return WeekDay.TUESDAY,
  case "wednesday" -> return WeekDay.Wednesday,
  // ...
  default -> throw new IllegalStateException(),
}

With #33 only interfaces are detected. One can do constants classes with enums and classes as well.

The line
this.stringTerrainInputList = new ArrayList<String>();
created the custom comment "Verwende den 'diamond operator'" ("Use the 'diamond operator'"), but should have been "Explizite Typangabe 'String' kann mit <> ersetzt werden" ("Explicit type argument 'String' can be replaced with <>") (text copied from IntelliJ).

Did you considered adding an appropriate license to this project. Otherwise, it will be hard to use :)

I've had an assignment with the following code example (paraphrased):

public class Test {
    private static boolean someVariable = false;

    public static void main(String[] args) {
        for (int i = 0; i < 50; i++) {
            someVariable = someOtherMethodThatReturnsABoolean();
        }
    }
}

The variable declaration got annotated saying it should be an instance attribute, however, because the static main method uses the variable, it needs to be static from my understanding.

The following warning is generated each time the tests are run:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

I think this can be fixed by adding this to the pom.xml:

       <dependency>
            <groupId>org.slf4j</groupId>
            <artifactId>slf4j-api</artifactId>
            <version>2.0.5</version>
       </dependency>
       <dependency>
            <groupId>org.slf4j</groupId>
            <artifactId>slf4j-simple</artifactId>
            <version>2.0.5</version>
       </dependency>

(I don't know if the simple one is the right logger)

For example:

enum WeekDay {
  MONDAY,
  TUESDAY;

  // ...
}

switch (weekday) {
  case MONDAY -> 1,
  case TUESDAY -> 2,
  // ...
}

could be either a Map<WeekDay, Integer> or an associated attribute of the enum.

• Tri-states like Option<bool> or nullable Boolean should be avoided
• Optional should never be an argument, because of nullability resulting in tri-state
• #135
• Shadowing of variables should be avoided
• #136
• Labels on Loops should be avoided in general, because it makes control flow harder to read (https://stackoverflow.com/questions/33689466/labels-in-java-bad-practice/33689582#33689582)
• #137
• #134

String a;
// ... some code

// then initialized when it is used:
a = "abc";

• abstract/super classes that only have methods should be interfaces, covered by #51
• regex patterns without a comment explaining what it does
• Arguments should not be final
  Why?
• harder to read
• introduces extra noise
• reassignment of arguments is almost never done and should not be dne

It would be nice if one could test error messages (especially to see if they span the correct text) and that one did not mistype a localized key.

The initial implementation of #14 proposed in #16, has a number of problems:

Minor Problems

• it does traverse code blocks multiple times and recursively to fill the Scope class
• as one might change an array based on a variable, the lint will ignore all further assignments to the default value in next calls:

Scanner scanner = new Scanner(System.in);
int index = scanner.nextInt();
int[] newArray = new int[10];
newArray[index] = 2;
newArray[0] = 0; // this assignment can obviously not be removed

• This code does not work as well:

int[] newArray = new int[10];
newArray[1] = 2;
newArray[0] = 0; // can be removed becahse index 1 ≠ 2

The restriction has been made to reduce the initial implementation complexity, but could be lifted in the future by keeping better track of which indices have been set to which values/variables (IntelliJ seems to do this through Dataflow Analysis

#22 will be helpful

Generalizing the lint

It is technically possible to make this lint work with normal variables as well.

A common use of this would be in constructors:

class A {
  private String string;

  A() {
    this.string = null; // this assignment might be removed
  }
}

Additionally one might generalize the lint to not only detect unnecessary assignments of the default value, but other values as well:

int a;
int b = a;
b = a;

1. Finding duplicates:

<E> boolean containsDuplicates(Iterable<E> collection) {
    Set<E> visited = new HashSet<>();
    for (var e : collection) {
        if (visited.contains(e)) {
            return true;
    	}
        visited.add(e);
    }
    return false;
}

vs

<E> boolean containsDuplicates(Iterable<E> collection) {
    return new HashSet<>(collection).size() < collection.size();
}

2. System.arraycopy/Arrays.copyOf

// Note: I am not 100% sure that this code is correct
<T> T[] copyArray(T[] array) {
    T[] newArray = new T[array.length];
    for (int i = 0; i < array.length; i++) {
        newArray[i] = array[i];
    }
    return newArray;
}

3. Reimplementing contains method:

boolean isValid = false;
for (var value : array) {
  if (value.equals(other)) {
    isValid = true;
    break;
  }
}

vs

boolean isValid = Arrays.asList(array).contains(other);

For example boolean is by default false, therefore

boolean[] myArray = new boolean[mySize];

for (int i = 0; i < myArray.length; i++) {
    myArray[i] = false;
}

can be written as

boolean[] myArray = new boolean[mySize];

(Note: this would be wrong if the myArray variable is accessible outside of the class, because the object identity is changed through the assignment)

The code if (a == true), if (a != true), if (a != false) and if (a == false) should be detected right now. It does not seem to resolve constants, so one can bypass this lint:

private static final boolean TRUE = true;

boolean a;
if (a == TRUE) {}

There is an easy implementation possible by

1. Processing all scanner declarations/assignments
2. Then obtaining all uses of the variable
3. and looking for a .close() call
4. Or alternatively a try-with resource statement (should not be too hard to detect as well)

This does not guarantee that the scanner is reliably closed every time and never leaks, but should cover most of the cases.

What one might consider

• some make the a Scanner static attribute and never close it
• some (especially in the beginning) avoid closing the scanner, because they are constructed multiple times through for example loops

For example

StringBuilder builder = new StringBuilder();
String x;

builder.append("[").append(x).append("]");

or

String x;

String y = "a" + x + "b";

There are an endless number of cases, where String.format might apply.

Some common cases that should be straightforward to detect:

1. Literal concatenation. The concatenation is evaluated from left-to-right, so "" + ... will always evaluate to a string. Those could be detected easily and converted to a "".formatted(...) based on the type of each argument. (Note: this should work with constants as well, because of magic strings...)
2. Chained .append() calls for StringBuilder like in the above example.

Could you deploy the artifacts to Maven Central? Then we could easily integrate them.

I am not sure that spoon is capable of inferring variables and constant values.

This capability can be useful for a number of lints, which may not trigger, because constants/variables are not resolved.

For example #14 does not lint this:

int myVariable = 0;
int[] myArray = new int[10];

myArray[0] = myVariable;

In both cases, one has no other option, except for catching/rethrowing as a checked exception.

Should be reliable to detect non-latin languages in comments.

Currently one declares on which line a lint should trigger. This has the obvious problem that adding new lines will break tests. Especially for larger tests, with hundreds of lines, it is very annoying to readjust the line number for every single error.

boolean a, b, c;
if (a) {
    doSomething();
} else if (b) {
    doSomething();
} else if (c) {
    doSomething();
} else {
    doSomethingElse();
}

can be written as

if (a || b || c) {
    doSomething();
} else {
    doSomethingElse();
}

Similarly, the following is not detected as well:

if (a) {
    doSomething1();
    return /* terminal statement */;
}

if (b) {
    doSomething2();
    return /* terminal statement */;
}

doSomething1();
return /* terminal statement */;

if (a || !b) {
    doSomething1();
    return /* terminal statement */;
}

doSomething2();
return /* terminal statement */;

throw new IllegalArgumentException();

vs

throw new IllegalArgumentException("the day should be between 1 and 31");

Might be better to ignore exceptions that are declared in the same project (which might already have a descriptive message).

Sometimes in code one can find:

@SupressWarnings("checkstyle:someRule")
class MyClass {}

or

@SupressWarnings("unchecked")
void doesUnsafeCast() {}

• subclass does not override a substantial amount of its parent's class (composition would be more appropriate)
• only attributes are inherited? (-> composition!)
• only methods are inherited -> interface with default methods
• class is not abstract (very subjective)
• abstract class constructors must have a protected or lower visibility. Not public.
• abstract class has not abstract methods

if (!(a == b)) {
}

can be written as

if (a != b) {
}

The ConcreteCollectionCheck does only check for fields and return values. This may require reimplementing the check with Spoon.

Either completely discourage them or only accept them if they are private inside the class (for example in LinkedLists which may contain Node inner classes). Most of the time it is better to use an extra file.

Tests:

• should work with inner enums, interfaces, records, classes
• should work with multiple classes/interfaces/records/enums in the same file

An interface

• should not have attributes/constants
• if it has attributes, those should be final and static (they are implicitly). If this is the case, one should suggest using a method with a default impl on the interface or use an enum/class
• should have methods (some use interfaces as a constant class), this should trigger the constant class lint:

interface MyConstants {
    final static String MY_CONSTANT = "Hello World";
    final static String MY_ERROR = "Input is wrong";
}

• Interfaces should be implemented by at least one class (easy indicator to detect that something is wrong with an interface)

Sometimes one overwrites equals, but not hashCode, which is not okay.

Issue templates are useful to improve the quality of reported issues and they make it easier to create them.
Here are some categories that might benefit from a template:

• false-positive, some code triggers a lint, but should not
• false-negative, some code does not trigger a lint
• suggesting a new lint
• diagnostics problem, the error is confusing or can be improved
• a crash/bug report

I am not sure when this is okay and when not, but it is something that could be considered.

if (a) {
    // do something
    return /* terminal */; // applies for continue and break as well
} else {
    // normal path
}

if (a) {
    // do something
    return /* terminal */; // applies for continue and break as well
}

// normal path

Makes the code easier to read.

For null checks one must use ==, so this should not be linted.

As enums only have one instance, the attributes should be final and never change state.

For example

enum MyEnum {
    MY_VARIANT;
    private final List<String> strings = new ArrayList<>();
    
    public void addString(String string) {
        this.strings.add(string);
    }
}

This can result in confusing behavior.

• @author and @version should not be in method/attribute javadoc
• @author with something that is not a u-shorthand
• TODOs in javadocs are not detected:

/**
 * TODO Write Javadoc
 * 
 * @author uxxxx
 * @version 1.0
 */

• faulty javadoc prevents lints from triggering:

/**
 *
 */
 */

/**
 *
/*
 */
 */

/*
 *
 */

It is common to add a REGEX suffix/prefix to the constant. If the constant is not used as a String, but instead compiled to a Pattern, it should be compiled to a Pattern in the constant:

private static final String MATCH_NEW_COMMAND_REGEX = "new \\d,\\d";

public void parseCommand(String command) {
    if (Pattern.matches(MATCH_NEW_COMMAND_REGEX, command)) {
        System.out.prinln("new command got called");
    }
}

vs

private static final Pattern MATCH_NEW_COMMAND_REGEX = Pattern.compile("new \\d,\\d");

public void parseCommand(String command) {
    if (MATCH_NEW_COMMAND_REGEX.matcher().matches()) {
        System.out.prinln("new command got called");
    }
}

StringBuilder builder = new StringBuilder();

builder.append('\n');

Does not trigger the lint.

This means omitting the generic:

public List getList() {
    return myList;
}

For example

class A {
    private List<String> myList;
    
    public List<String> getList() {
        return this.myList;
    }
}

instead of

class A {
    private List<String> myList;
    
    public List<String> getList() {
        return new ArrayList<>(this.myList);
    }
}

Same problem with

class A {
    private List<String> myList;
    
    public void setList(List<String> myList) {
        this.myList = myList;
    }
}

void setArray(String[] array) {
    this.array = array;
}

and

String[] getArray() {
    return this.array;
}

are not detected. Similarly for multi-dimensional arrays.

if (a) {
    doSomething();
} else {
    if (b) {
        doSomething2();
    } else {
        doSomethingElse();
    }
}

can be written as

if (a) {
    doSomething();
} else if (b) {
    doSomething2();
} else {
    doSomethingElse();
}

This might be implemented already, in this case I have found code where the lint does not trigger.

class A {
    int a;
    String[] b;

    void doSomething() {
        int a = 0;
        String[] b = new String[10];
    }
}

In the above example the variable b in doSomething is not marked as unused

A Dockerfile could be useful as well.

This code should be avoided:

int a = 3;
à = a + 5;

instead, one should write:

int a = 3;
a += 5;

Commutative operations, where a = b <op> a is a <op>= b:

• &
• |
• ^
• *
• +
  Other operations, where only a = a <op> b is a <op>= b:
• %
• -
• /
• <<
• >>

The lint should work with a = a <op> b <op> c and a = a <op1> b <op2> c as well (e.g. a = a + b - c)

e.g. ANSI and UTF-8. The reason is that Spoon doesn't accept files with different encodings, so this code checks the encoding. We could try to rewrite all files in UTF-8 and use e.g. in-memory files for the linters.

As a rule of thumb: System.out calls and Scanner should only be called in a single class.

Most commonly one calls:

• System.out.println
• System.out.print
• System.out.printf <- this one has been observed, but did not trigger the lint if it exists?
• Scanner.nextLine()
• Scanner.nextInt()

There is an issue, which causes the AutoGrader to crash.

Error:

!MESSAGE Autograder failed: java.lang.NullPointerException: Cannot invoke "spoon.reflect.declaration.CtMethod.isPublic()" because the return value of "spoon.reflect.code.CtReturn.getParent(java.lang.Class)" is null
	at de.firemage.autograder.core.check.oop.ListGetterSetterCheck$1.process(ListGetterSetterCheck.java:34)
	at de.firemage.autograder.core.check.oop.ListGetterSetterCheck$1.process(ListGetterSetterCheck.java:31)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:72)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtBlock(CtScanner.java:321)
	at spoon.support.reflect.code.CtBlockImpl.accept(CtBlockImpl.java:58)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.visitCtIf(CtScanner.java:505)
	at spoon.support.reflect.code.CtIfImpl.accept(CtIfImpl.java:36)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtBlock(CtScanner.java:321)
	at spoon.support.reflect.code.CtBlockImpl.accept(CtBlockImpl.java:58)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.visitCtConstructor(CtScanner.java:390)
	at spoon.support.reflect.declaration.CtConstructorImpl.accept(CtConstructorImpl.java:48)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtClass(CtScanner.java:357)
	at spoon.support.reflect.declaration.CtClassImpl.accept(CtClassImpl.java:63)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtPackage(CtScanner.java:679)
	at spoon.support.reflect.declaration.CtPackageImpl.accept(CtPackageImpl.java:82)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtPackage(CtScanner.java:678)
	at spoon.support.reflect.declaration.CtPackageImpl.accept(CtPackageImpl.java:82)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtPackage(CtScanner.java:678)
	at spoon.support.reflect.declaration.CtPackageImpl.accept(CtPackageImpl.java:82)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:149)
	at spoon.reflect.visitor.CtScanner.visitCtPackage(CtScanner.java:678)
	at spoon.support.reflect.declaration.CtPackageImpl.accept(CtPackageImpl.java:82)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:184)
	at spoon.reflect.visitor.CtScanner.visitCtModule(CtScanner.java:964)
	at spoon.reflect.factory.ModuleFactory$CtUnnamedModule.accept(ModuleFactory.java:96)
	at spoon.reflect.visitor.CtScanner.scan(CtScanner.java:194)
	at spoon.support.visitor.ProcessingVisitor.scan(ProcessingVisitor.java:68)
	at spoon.support.QueueProcessingManager.process(QueueProcessingManager.java:118)
	at spoon.reflect.CtModelImpl.processWith(CtModelImpl.java:126)
	at de.firemage.autograder.core.integrated.StaticAnalysis.processWith(StaticAnalysis.java:61)
	at de.firemage.autograder.core.check.oop.ListGetterSetterCheck.check(ListGetterSetterCheck.java:31)
	at de.firemage.autograder.core.integrated.IntegratedCheck.run(IntegratedCheck.java:37)
	at de.firemage.autograder.core.integrated.IntegratedAnalysis.lint(IntegratedAnalysis.java:105)
	at de.firemage.autograder.core.Linter.checkFile(Linter.java:112)
	at de.firemage.autograder.core.Linter.checkFile(Linter.java:54)
	at de.firemage.autograder.cmd.Application.call(Application.java:118)
	at de.firemage.autograder.cmd.Application.call(Application.java:35)
	at picocli.CommandLine.executeUserObject(CommandLine.java:1953)
	at picocli.CommandLine.access$1300(CommandLine.java:145)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2352)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2346)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2311)
	at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2179)
	at picocli.CommandLine.execute(CommandLine.java:2078)
	at de.firemage.autograder.cmd.Application.main(Application.java:67)``

On demand I will provide the code the grader failed on.

For example

/**
 * Does return a value.
 * 
 * @returns the value
 */
public int get() {
   // this is not documented through an `@throws` in the javadoc
   throw new IllegalArgumentException();
}

Currently the lint can detect for example int ZERO = 0 or int ONE = 1, but only because of a hardcoded list of known problems.

Most commonly problematic constants

• contain the datatype in the name String ERROR_STRING = "An error occurred.", which can be fine, but is redundant
• contain some variation of its values in the name

Here are some examples where the lint does not trigger at the moment:

class Test {
    private static final boolean TRUE = true;
    private static final boolean FALSE = false;
    
    // for those constants there are an infinite number of variants,
    // which is why I would suggest checking that if the literal content
    // is equal to the name with the correct naming convention
    //
    // e.g. "debug-path".toUpperSnakeCase().equals(attribute.name())
    private static final String UP = "up";
    private static final String DOWN = "down";
    private static final String DEBUG_PATH = "debug-path";
    
    // this is linted?
    private static final String LEFT = "left";
    // and this is not?
    private static final String RIGHT = "right";
    
    // Another example is:
    private static final String BIG_X_REGEX_FORMAT = "X"; // hard to check, because single letters are common in words...
    
    public static void doSomething() {
        MyUiClass myUiObjectClass = new MyUiClass(); // "My", "Object" and "class" are all redundant words
        UserInterface userInterface = new UserInterface(); // much better
    }
}