WIP Annotation and shadow classes

[surli]

While working on #1914 I got stuck with undefined contracts regarding annotations and shadow classes.

Annotation and retention policy

All annotations, with a retention policy not defined explicitely as RUNTIME are not kept as runtime: this means that the information about the annotation is not available through reflexivity. Ego, we cannot access them through the shadow mechanism.

I'm not sure if this has been really documented in Spoon.

Annotation values with arrays

If we have an annotation with a method returning an array, we can use it without the braces if a single value is given:

public @interface MyRole {
String[] role();
}

public class MyClass {
@MyRole(role = "value");
String myField;

@MyRole(role = { "value" })
String myField;

When using reflexivity those two annotation will have the same value: an array of String.
However, when we parse this code, we produce two differents models for the annotation: for the first one, we will create a CtLiteral and for the second one, a CtNewArray. The result is that when we got the shadow class or the spooned class, we have different results.

I think we explicit used different models to be closed to the sourcecode of the user and to be able to prettyprint it correctly (it might be related to #1724). However, I think that we should align our behaviours here: the return type is an array, so I personnaly think it should always return a CtExpression corresponding to an array. And if there is a single value in the array, then we might always prettyprint it, using the first proposed expression.

WDYT?

[monperrus]

All annotations, with a retention policy not defined explicitely as |RUNTIME| are not kept as runtime: this means that the information about the annotation is not available through reflexivity. Ego, we cannot access them through the shadow mechanism. I'm not sure if this has been really documented in Spoon.

You're right, it's good to document this.

[monperrus]

think we explicit used different models to be closed to the sourcecode of the user and to be able to prettyprint it correctly (it might be related to #1724 <#1724>). However, I think that we should align our behaviours here: the return type is an array, so I personnaly think it should always return a CtExpression corresponding to an array.

I agree.

[surli]

Now do you think that in case of an array with a single element, we should remove remove the unnecessary braces when pretty printing?

@myAnnotation(value = { 1 }) -> @myAnnotation(value = 1)

Or should we keep them explicit?

[pvojtechovsky]

we should remove the unnecessary braces when pretty printing?

I like this idea. It is nearer to how most of the developers would write it - without extra brackets. So the code produced by Spoon will be nearer to origin - nice - sources.

[monperrus]

Afterthought, I would prefer to keep full backward compatibility and to simply introduce a new method getValueAsArray() for more regular usage.

[pvojtechovsky]

Then the spoon model will mirror the origin sources and the spoon java printer can generate same annotation expression like it was in origin sources.

... of course we cannot detect that in runtime, but it is acceptable.

[surli]

Afterthought, I would prefer to keep full backward compatibility and to simply introduce a new method getValueAsArray() for more regular usage.

I don't see the point of the method getValueAsArray then?... And what about the behaviour in shadow classes? If we get an array with a single element we only take the single element to have a consistent behaviour?

I really think here that it's a bug in Spoon that we should fix: the clients should expect a CtNewArray when the type of the annotation method is an array of something... It looks tricky to manage differently the result for the same type.

[pvojtechovsky]

I as a client of Spoon prefer, when generated code is as much as possible similar to origin code. This is critical Spoon feature for me.

Therefore I like current implementation, which mirrors expression, which was in source code. I have no problem that it is not exactly type matching with type of annotation value.

If somebody really needs type matching expression value, then we can make new derived method, which will add wrapping CtNewArray element when type of annotation value is an array... The method might be something like getTypedValue(String) ... but I personally do not see use case for that... So I vote to keep the annotations build from sources as they are.

The annotations made by java reflection might be handled in compatible way - if there is array with one value, then expression should be simplified to that one value. Advantage:

• it behaves similar like annotation from sources
• we do not have to modify DJPP, because it will print nice code - like human would write.
• If we do it opposite and DJPP will automatically remove {} arround single value array, then it again breaks the rule: generated code is as much as possible similar to origin code

WDYT?

[surli]

I have no problem that it is not exactly type matching with type of annotation value.

I personnaly have a problem with that: if you want to write a transformation based on an annotation that you know it will return an array, then you'll want to catch the CtNewArray not the CtLiterals.

The current implementation forces the developpers to check the type and to manage different cases which are not "natural" when you look at the type.

I personnaly consider that the requirements model should be as least astonishing as possible for developpers is more important than the produced code on the end: it's easier to write transformations if the model is as expected.

So why not adding a method and document that the returned value of getElementValue is not necessarily the expected type.

[pvojtechovsky]

getElementValue x getTypedValue

I do not care about names, but in any case I suggest to

1. the method which returns origin expression from the source code is NOT DERIVED - so clonning, scanning, equals still works
2. the method which returns expression adapted to expected type is DERIVED

WDYT?

[surli]

WDYT?

OK for me. And then we agree that in shadow classes all array with a single element will be transformed in the model to only reflect the single element, right?

[pvojtechovsky]

that in shadow classes all array with a single element will be transformed in the model to only reflect the single element, right?

I agree

[monperrus]

model should be as least astonishing as possible for developpers

+1

So why not adding a method and document that the returned value of getElementValue is not necessarily the expected type.

Good, we're on the same line