ceylon icon indicating copy to clipboard operation
ceylon copied to clipboard

Java {PARAMETER,FIELD} annotation applied to a parameter gets placed on field

Open dlkw opened this issue 8 years ago • 7 comments

Don't know if it qualifies as bug, but it look like it is one:

shared class A(inject B b)
{
    //...
}

If this class is inspected in Java with Reflection, it has a one-parameter constructor with parameter type B and a zero-parameter constructor. The one-parameter constructor does not have the @Inject annotation listed in the getParameterAnnotations() call. I think that's a bug: the annotation should be there.

(In my case, inject is an annotation written in Java, not sure if that makes a difference.)

On the other hand, when writing

shared class A
{
    shared new(inject B b){}
}

Again the class has two constructors visible in Java, but this time the one-parameter constructor is provided with @Injectas parameterAnnotation.

dlkw avatar Oct 14 '17 15:10 dlkw

Isn't this a misuse of the inject annotation?

I'm surprised the typechecker even accepts this, since @Inject is declared @Target(value={METHOD,CONSTRUCTOR,FIELD}).

gavinking avatar Oct 14 '17 16:10 gavinking

Oh, right, it sticks it on the field.

The one-parameter constructor does not have the @Inject annotation listed in the getParameterAnnotations() call.

Yeah, it, quite correctly, it seems to me, gets placed on the field declaration:

@javax.inject.Inject
@dlkw com.redhat.ceylon.compiler.java.metadata.Ignore
@com.redhat.ceylon.common.NonNull
private final .goodbye.B b;

gavinking avatar Oct 14 '17 16:10 gavinking

On the other hand, when writing ... the class has two constructors visible in Java, but this time the one-parameter constructor is provided with @Injectas parameterAnnotation.

I get the following error from the typechecker in this case:

annotated program element does not satisfy annotation constraint: the annotation is declared 'target {FIELD, METHOD, CONSTRUCTOR}'

That looks perfectly correct to me. There is no reasonable target for the @Inject annotation in that case.

gavinking avatar Oct 14 '17 16:10 gavinking

So, really the correct usage of inject is either:

shared inject class A(B b) {
}

Or:

shared class A {
    B b;
    shared inject new (B b) {
        this.b = b;
    }
}

And these produce approximately the same code.

Stuff like:

shared class A(inject B b) {
}

Results in field injection, which is also reasonable, though not what I would recommend.

I'm going to close this, because I think it's functioning correctly.

gavinking avatar Oct 14 '17 16:10 gavinking

@gavinking well, it's not the standard java @Inject annotation, I should have noted. This one is

@Retention(RUNTIME)
@Target( {
        FIELD, PARAMETER
})
public @interface Inject

(comes from apache Cayenne's DI). Sorry for the confusion.

dlkw avatar Oct 14 '17 18:10 dlkw

I agree that the FIELD-targetted annotations should be placed on the field declaration. The question here is if it should also be placed on the parameters (which I would deem correct).

On the other hand, it's unknown with what semantics the annotations are evaluated at run time, so maybe it's sometimes harmful to put in in both occasions. For Ceylon, PARAMETER would always also mean FIELD in the sense that initializator parameters are always fields. I still think it'd be more correct to also put them on the parameters.

dlkw avatar Oct 14 '17 18:10 dlkw

Ah, vale, that’s a pattern we haven’t seen before. (It’s not used by any of the EE annotations, as far as I’ve seen.)

And yeah, I’m inclined to say that if you have a PARAMETER annotation on a parameter, then that’s where it should stay.

gavinking avatar Oct 14 '17 19:10 gavinking