[Bug] Annotations are not added to the correct in the model

[SirYwell]

Describe the bug

I stumbled across a few issues with annotations:

1. Annotations with ElementType.TYPE_USE are added to fields and methods, rather than to the type references. From my understanding, this is a JDT bug.
2. Annotations on array types are fully ignored. E.g. Object @Nonnull [] @Nullable [] becomes Object[][]. The information seems to be provided correctly by JDT, but I wasn't able to figure out how to integrate it in the spoon code properly.
3. Annotations in front of method references, e.g. Function<?, ?> f = @Nonnull Object::toString; is added to the model but it isn't printed anymore.

I started looking into 2. and 3. but that was over my head for now. Maybe someone else knows a little bit more about the code that needs to be changed for that.

To Reproduce

Use the code fragements from above in your code and inspect and/or print it.

Input:

/*Code being processed by spoon and leading to the bug.*/

// The definition of the used annotations should look like that

@Target({ElementType.TYPE_USE})
@Retention(RetentionPolicy.RUNTIME)
@interface Nullable { }

@Target(ElementType.TYPE_USE)
@Retention(RetentionPolicy.RUNTIME)
@interface Nonnull { }

// that's the class we want to inspect/print

class M {
	private @Nullable Object @Nonnull [] field;
	public void method(final @Nullable Object @Nonnull [] @Nullable [] value) {
		Function<Object, String> toString = @Nonnull Object::toString;
	}
}

Processing with Spoon:

/*Code calling spoon on the input and triggering the bug.*/

Output:

/*Output code or stacktrace if unexpected Exception is raised*/
class M {
    @Nullable
    private Object[] field;

    public void method(@Nullable
    final Object[][] value) {
        Function<Object, String> toString = Object::toString;
    }
}

Note that you can also open a pull request reproducing the issue and reference that, instead of writing additional information here.

Operating system, JDK and Spoon version used

• OS: Windows 10
• JDK: Temurin 17.0.1
• Spoon version: master branch as of https://github.com/INRIA/spoon/commit/12cc1a530e5801829ec4e5f674135f704040f419

[slarse]

Ouch, that output is pretty crazy.

I don't have any more information about this off the top of my head, but what I can say immediately is that working around JDT bugs (as is the case for point 1.) is something we only do for really critical stuff. This seems to not be quite worth the effort.

The other two seem like problems in Spoon, but I don't know why. For arrays, I'd wager it has something to do with how they're modeled as nested types in Spoon.

[SirYwell]

As the JDT bug was fixed, I started looking into this again. I'm currently working on tests for all the places where types are used (JLS 4.11). I already found some issues with the model and with printing (the worst are code that doesn't compile anymore), and I'd start fixing them one by one at some point. But I think it would make sense to add all the tests regardless of whether they cover current bug. WDYT? (See here for my current progress)

On another note, I think the DJPP should always print the type annotations directly at the type and always without line breaks. The printed code is very hard to read sometimes currently. What do you think about that?

[slarse]

But I think it would make sense to add all the tests regardless of whether they cover current bug. WDYT? (See here for my current progress)

Yes.

On another note, I think the DJPP should always print the type annotations directly at the type and always without line breaks. The printed code is very hard to read sometimes currently. What do you think about that?

Could you provide a concrete before (what it looks like right now) and after (how you propose for it to look) example?

[SirYwell]

Could you provide a concrete before (what it looks like right now) and after (how you propose for it to look) example?

I didn't check the code much, but currently the DJPP prints annotations for methods and fields in front of modifiers. Example:

@Nullable
private String value = ...;

@Override
@NotNull
public String doSomething(@NotNull final String s) { ... }

JLS 9.7.4 mentions:

It is customary, though not required, to write declaration annotations before all other modifiers, and type annotations immediately before the type to which they apply.

This would rather look like

private @Nullable String value = ...;

@Override
public @NotNull String doSomething(final @NotNull String s) { ... }

but with the current printer, it always adds a line break after writing an annotation. In situation other than methods and fields, where the annotation is always directly at the type, this might cause weird output like

class A extends @Anno
B<@Anno
C> {

	public @Anno
	String m(@Anno
	int a) { ... }
}

which is hard to read.

With qualified names this becomes even more relevant, as type annotations are between the package name and the type name (see the JLS section linked above, it mentions

For example, assume an annotation interface TA which is meta-annotated with just @Target(ElementType.TYPE_USE) . The terms @TA java.lang.Object and java.@TA lang.Object are illegal because the simple name to which @TA is closest is classified as a package name. On the other hand, java.lang.@TA Object is legal.

), producing things like

<java.lang.@spoon.test.annotation.testclasses.typeannotations.TypeUseA
String>

in my tests.

Fun fact (I discovered this while writing this text, making this proposal even more reasonable), this is just another example where the printer currently produces wrong code when printing annotations on methods and fields, as the code does not compile anymore with something like

@NotNull
public java.lang.String doSomething() { ... }

if the annotation is only applicable to TYPE_USE. Guess I need to write more tests...

[SirYwell]

@slarse did you have time to look at this already?

[slarse]

@SirYwell Nope, thanks for the reminder.

java.lang.@TA Object

Hah. I had no idea you could do that.

But yes, I agree that it would be better to write annotations on one line directly preceding the concerned type.

If you have test cases that you want to merge ahead of time that's also good.