Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

org.jboss.cdi.tck.tests.definition.bean.BeanDefinitionTest #429

Closed
jeanouii opened this issue Feb 14, 2023 · 97 comments · Fixed by #433
Closed

org.jboss.cdi.tck.tests.definition.bean.BeanDefinitionTest #429

jeanouii opened this issue Feb 14, 2023 · 97 comments · Fixed by #433
Labels
accepted Issue/challenge is accepted challenge TCK test challenge

Comments

@jeanouii
Copy link
Contributor

Per the current TCK Process documentation, please find bellow a challenge related to CDI TCK. Can someone please add the challenge label which is required by the TCK Process but not available to everyone to add.

The relevant specification version and section number(s)

https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#bean_types
https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#managed_bean_types

The coordinates of the challenged test(s)

Only following test in the class are related to the challenge

  • testRawBeanTypes

The exact TCK version

CDI TCK version 4.0.7

The implementation being tested, including name and company

Apache OpenWebBeans 4.0.0-SNAPSHOT from the Apache Software Foundation

A full description of why the test is invalid and what the correct behavior is believed to be

The specification defines the following

A bean type defines a client-visible type of the bean. A bean may have multiple bean types.

And then provides a list of valid and invalid type. But I'm facing a case where it's ambiguous, at least I can find different behaviors so it's worth either updating a bit the TCK or improving the specification.

The test may fail with the assert on MySuperInterface.

    @Test
    @SpecAssertions({ @SpecAssertion(section = MANAGED_BEAN_TYPES, id = "a"), @SpecAssertion(section = BEAN_TYPES, id = "a"),
            @SpecAssertion(section = BEAN_TYPES, id = "l"), @SpecAssertion(section = BEAN, id = "ba") })
    public void testRawBeanTypes() {
        assertEquals(getBeans(MyRawBean.class).size(), 1);
        Bean<MyRawBean> bean = getBeans(MyRawBean.class).iterator().next();
        assertEquals(bean.getTypes().size(), 5);
        assertTrue(bean.getTypes().contains(MyRawBean.class));
        assertTrue(bean.getTypes().contains(MyBean.class));
        assertTrue(bean.getTypes().contains(MyInterface.class));
        assertTrue(bean.getTypes().contains(MySuperInterface.class));
        assertTrue(bean.getTypes().contains(Object.class));
    }
interface MyInterface extends MySuperInterface<Number> {
}

MySuperInterface can be seen from a specification point of view as an interface or as a parametrized type. The 2 are valid per https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#managed_bean_types unless my reading is wrong.

Any supporting material; debug logs, test output, test logs, run scripts, etc.

Here are 2 screenshots to show 2 different behaviors for the same specification

One option
image

Second option
image

@manovotn
Copy link
Contributor

manovotn commented Feb 14, 2023

We recently encountered this in Quarkus as well but in the end concluded that the test is actually correct in that it follows the JLS. IIRC the reasoning was that once you inherit a raw type, then the types you discover and inherit from hierarchy will also be raw types. I can try to look it up tomorrow.
@Ladicek this^^ should sound very familiar, do you remember the details?

@manovotn manovotn added the challenge TCK test challenge label Feb 14, 2023
@manovotn
Copy link
Contributor

@jeanouii Did some digging and here is the issue we had for it at the time - quarkusio/quarkus#30571
And here is a PR (just to show that it mimics the TCK scenario; we were investigating it exactly because of this TCK test).

The JLS quote is here and goes like this:

The superclass types (respectively, superinterface types) of a raw type are the erasures of the superclass types (superinterface types) of the named class or interface.

Which IMO implicates that the TCK test is correct.
Thoughts?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 15, 2023

Yeah, I also filed an issue to clarify this in the specification: jakartaee/cdi#645 But I'm pretty convinced the TCK is correct. It's just that at the point this part of the CDI spec was written, even the JLS didn't make a very good distinction between superclasses and superclass types (and interfaces / interface types).

@jeanouii
Copy link
Contributor Author

Thanks for the feedback, it helps.

I think there are 2 main things here

  • is the TCK test valid according to the spec?
  • is the TCK test ambiguous or not?
  • is the Spec clear enough so there is no possible ambiguity?

And then, there is the question, is this what we want or do we want to change the spec in one way or the other?

As part of this challenge, only the first part matters in my opinion. And in that area, I think the test is at least ambiguous or too restrictive compared to the spec.

If we look at the spec again, it says

Almost any Java type may be a bean type of a bean:
- A bean type may be an interface, a concrete class or an abstract class, and may be declared final or have final methods.
- A bean type may be a parameterized type with actual type parameters and type variables.
- A bean type may be an array type. Two array types are considered identical only if the element type is identical.
- A bean type may be a primitive type. Primitive types are considered to be identical to their corresponding wrapper types in java.lang.
- A bean type may be a raw type.

However, some Java types are not legal bean types :
- A type variable is not a legal bean type.
- A parameterized type that contains a wildcard type parameter is not a legal bean type.
- An array type whose component type is not a legal bean type.

Per spec, both interface and parametrized type are valid. There is not additional information on when we pick one or the other if we have the choice. So either we update the test to accept both in the assert, or we update the test such as the super interface is not a parametrized type or we update the spec to be more clear on what we expect.

What do you think?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 15, 2023

There is not additional information on when we pick one or the other if we have the choice.

There's no choice though. The relevant section of the CDI specification says:

The unrestricted set of bean types for a managed bean contains the bean class, every superclass and all interfaces it implements directly or indirectly.

If the bean has a raw type in its supertype hierarchy, then all supertypes of that raw type are erasures. This is specified directly in the Java Language Specification, chapter 4.8.

Here's a quote from JLS 17:

The superclass types (respectively, superinterface types) of a raw type are the erasures of the superclass types (superinterface types) of the named class or interface.

Here's a quote from JLS 8:

The superclasses (respectively, superinterfaces) of a raw type are the erasures of the superclasses (superinterfaces) of any of the parameterizations of the generic type.

The JLS 8 quote is closer in time to when the relevant part of the CDI specification was written, so naturally the CDI specification uses similar wording. In my opinion, the JLS 17 wording is more precise (because it makes a distinction between supreclasses and superclass types), but the meaning is the same.

@jeanouii
Copy link
Contributor Author

Lemme discuss this with the team please. There is something I can't buy but it's probably a lack of understanding of the specification. My apologies for that.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 15, 2023

No worries -- it tooks us a while to figure this out as well.

@manovotn
Copy link
Contributor

Per spec, both interface and parametrized type are valid

True, and there are tests for both - this test is specifically looking at what should happen if your bean extends a raw type that itself has a type hierarchy with parameterized typed in it.
But you certainly cannot choose freely between that, that would then break the assignability rules. Based on your choice, you'd either be or not be able to inject that bean into parameterized injection point (spec rules for assignability of param. types are here)

I am with Ladislav in that you need to follow JLS to determine what types you discover (and hence accept as bean types).

@jeanouii
Copy link
Contributor Author

jeanouii commented Feb 16, 2023

I've discuss this with the team which does understand the choice but does not fully agree with it because it has some side effects (for example 2 decorators on parametrized types).

I can't argue against the spec, so I'll maintain the challenge because the TCK is more restrictive than the specification which allows both. And it implies 3 possible choices

1/ MySuperInterface<Number> and MySuperInterface
2/ MySuperInterface<Number> only
3/ MySuperInterface only

The TCK forces 3/ by choice.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

I disagree that the specification allows both. What is a supertype of a raw type is clearly defined by the JLS.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

Actually, let me expand on that a little.

Clearly a parameterized type is a legal bean type, that is, it may be present in the set of bean types. However, the set of bean types may not contain arbitrary types drawn out of thin air. The set of types that eventually become bean types is defined in the CDI specification here: https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#managed_bean_types (for managed beans; similar definitions exist for producer beans).

The unrestricted set of bean types for a managed bean contains the bean class, every superclass and all interfaces it implements directly or indirectly.

Now, the managed bean class in question is MyRawBean. That class, and all its supertypes, are declared like this:

@Dependent
class MyRawBean extends MyBean {
}
@Dependent
class MyBean<T> implements MyInterface {
}
interface MyInterface extends MySuperInterface<Number> {
}
interface MySuperInterface<T> {
}

Now, clearly MyRawBean extends MyBean and clearly MyBean is a generic class. Therefore, MyRawBean extends a raw type of MyBean.

From there, it follows that all supertypes of MyBean in the set of supertypes of MyRawBean are erasures of the respective types. This is the crucial point.

From that, it directly follows that MyRawBean doesn't implement MySuperInterface<Number>; it implements the raw type MySuperInterface.

And per the CDI specification, the set of bean types of MyRawBean cannot possibly include MySuperInterface<Number> -- simply because that is not an interface it implements.

Hope that's a bit more clear now.

@struberg
Copy link

@Ladicek where exactly in the JLS did you think your argument is supported? Because e.g. 8.1.4 rather supports Jean-Louis argumentation. Note that whole JLS 8.1.4 does NOT mention |T|.C ! txs

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

I quoted the precise sentence from the JLS above. See chapter 4.8:

The superclass types (respectively, superinterface types) of a raw type are the erasures of the superclass types (superinterface types) of the named class or interface.

EDIT: chapter 4.10.2 says the same:

Given a generic class or interface C with type parameters F1,...,Fn (n > 0), the direct supertypes of the raw type C (§4.8) are all of the following:

  • The erasure (§4.6) of the direct superclass type of C, if C is a class.
  • The erasure of the direct superinterface types of C.
  • The type Object, if C is an interface with no direct superinterface types.

@manovotn
Copy link
Contributor

Ladislav's explanation is correct.
@jeanouii also note that spec cannot just "let you choose" - as an example, imagine having an injection point for such a bean. In this case that would be (if I am not mistaken) @Inject MySuperInterface<Number>. If the spec were to let you choose according to your options, this injection point might or might not be satisfied. That'd be pretty useless behavior and non-portable on top of that.

@struberg
Copy link

You missed my point. nowhere in 8.1.4 https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.1.4 is mentioned that |T|.C (type erasure that is) gets triggered! Nowhere in the CDI spec is it defined neither.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

The very first sentence of chapter 8.1.4 says:

The optional extends clause in a normal class declaration specifies the direct superclass type of the class being declared.

What is a direct superclass type is defined by chapter 4.10.2, which I linked to above.

@struberg
Copy link

Ladislav's explanation is correct. @jeanouii also note that spec cannot just "let you choose" - as an example, imagine having an injection point for such a bean. In this case that would be (if I am not mistaken) @Inject MySuperInterface<Number>. If the spec were to let you choose according to your options, this injection point might or might not be satisfied. That'd be pretty useless behavior and non-portable on top of that.

The point is that the bean type resolution and the matching to the injection points DEPENDS on whether it is a bean injection point, a decorator or a producer bean. Thus whether type erasure is required or not resolved in the bean type calculation but at the point when it gets applied.

@manovotn
Copy link
Contributor

Ladislav's explanation is correct. @jeanouii also note that spec cannot just "let you choose" - as an example, imagine having an injection point for such a bean. In this case that would be (if I am not mistaken) @Inject MySuperInterface<Number>. If the spec were to let you choose according to your options, this injection point might or might not be satisfied. That'd be pretty useless behavior and non-portable on top of that.

The point is that the bean type resolution and the matching to the injection points DEPENDS on whether it is a bean injection point, a decorator or a producer bean. Thus whether type erasure is required or not resolved in the bean type calculation but at the point when it gets applied.

I am not sure I can see how is existence of slightly different assignability rules for various scenarios related to my example?
The point still stands in that giving the spec an option to choose types breaks injectability expectations - to be extra accurate, for the purpose of this example, we can consider the injection to be the plain field injection into a class based bean.

@jeanouii
Copy link
Contributor Author

jeanouii commented Feb 16, 2023

Thanks for your patience and references. I think I can fully understand the use case we are talking about and what this specific test is trying to do.

This ONLY applies to MyRawBean because it is considered as a raw type (it extends MyBean without providing an explicit parameter for the parametrized Class MyBean<T>). And therefore if as you mention we should follow JLS rules, then MySuperInterface must be used with its erasure and not with its parameter. Correct?

Do you agree it would be different if MyRawBean would extend MyBean<Foo> then we would keep MySuperInterface<Number> and not its erasure ?

The direct impact of this use case as defined in https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#assignable_parameters is that MyRawBean is only assignable to MySuperInterface<?> or MySuperInterface<Object> but not MySuperInterface<Number>. Correct?

Edit: Sorry, I realized @Ladicek rephrased and gave an example. I was reading and playing around and click send without noticing your replies. I rephrased it this way, with some examples to make sure I understood correctly.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 16, 2023

This ONLY applies to MyRawBean because it is considered as a raw type (it extends MyBean without providing an explicit parameter for the parametrized Class MyBean<T>). And therefore if as you mention we should follow JLS rules, then MySuperInterface must be used with its erasure and not with its parameter. Correct?

Yes.

Do you agree it would be different if MyRawBean would extend MyBean<Foo> then we would keep MySuperInterface<Number> and not its erasure ?

Yes.

The direct impact of this use case as defined in https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#assignable_parameters is that MyRawBean is only assignable to MySuperInterface<?> or MySuperInterface<Object> but not MySuperInterface<Number>. Correct?

The MyRawBean bean has the raw MySuperInterface type among its bean types. Therefore, the bean would be assignable to an injection point if the required type is:

  1. raw MySuperInterface, because

    Parameterized and raw types are considered to match if they are identical

  2. MySuperInterface<Object> and MySuperInterface<T> (where T is an unbounded type variable), because

    Parameterized and raw types are considered to match [...] if the bean type is assignable to the required type
    ...
    A raw bean type is considered assignable to a parameterized required type if the raw types are identical and all type parameters of the required type are either unbounded type variables or java.lang.Object.

I believe the current CDI specification wording doesn't consider raw MySuperInterface assignable to MySuperInterface<?>, though I don't see a reason why. I believe this could be added.

@jeanouii
Copy link
Contributor Author

Awesome. Thanks, then I see a couple of actions for non expert eyes like mine

  • add the injection part of matching to make sure we can inject it anywhere because it's a raw type
  • consider adding a small concrete example in 2.4.2.4 at the end when raw type is mentioned
  • add in the CDI spec either "in 2.1.2.1. Legal bean types" or "2.2.1.2. Bean types of a managed bean" a note such as this edge case is covered. Something with this idea "as an exception and in order to comply with JLS definition of raw types, all parametrized type (superclasses or superinterface implement directly or indirectly) must be represented by their erasure".

@rmannibucau
Copy link

Hi guys,

Read everything and think several statements are against CDI and Java SE as well (JLS), concretely I think the core issue comes from

The MyRawBean bean has the raw MySuperInterface type among its bean types.

The erasure happens and must be respected, this is right and written everywhere - not requoting what is already done - BUT you must take into account if the erasure happens on each type of the hierarchy and this is where the error comes from IMHO.
Typically raw bean looses mybean generic parameter so T is lost but mysuper interface has number as parameter and this is 100% unrelated to that so all good this must be kept to comply to java assignment rules otherwise it means a number is a string (guess we all agree it is not the case).

If we try to phrase it easier and without the mathematical notations to really make it obvious: types must respect reflection, this is what java does at the end in terms of runtime (keep in mind CDI is 100% targetting a runtime whatever it is). I know the compiler got enhanced a bit and can solve more cases these days - due to lambdas mainly - so it can be the most restrictive source java has as of today and it is fully aligned on the CDI requirement to provide type assignment to extensions in all events - otherwise extensions must reimplement the resolution each time so I guess we are good and the test got a shortcut we should fix.

@dblevins
Copy link
Contributor

dblevins commented Feb 20, 2023

It appears as though the term "raw type" is a term that describes a reference to a generic class, not the generic class itself.

If that's the case, we're all technically wrong as we've all been using it to describe the class/interface/bean and argue if a bean is or isn't a raw type when in reality a generic class that has any unbound parameters can be referred to either way (raw or parameterized). Depending on what decision the referring side makes, it will get different compiler behavior (erasure) enforced on its use of the reference.

What makes this ambiguous in the context of CDI is that we're using JLS semantics that 1) support either raw or parameterized, and 2) apply to referring side to argue CDI should support only one or the other, using just the bean definition.

I think we should definitely work this out and update the spec. It would seem the behavior that would most match the JLS is that we are able to support injection points that use beans either as raw types or parameterized types. It's unclear if any implementation supports that at this point, however.

Spec text

More precisely, a raw type is defined to be one of:

  • The reference type that is formed by taking the name of a generic class or interface declaration without an accompanying type argument list.

The key part is where it says the raw type is defined as "the reference type." Again, we should support both raw and parameterized types, but the JLS does actively steer people away from raw types.

The use of raw types is allowed only as a concession to compatibility of legacy code. The use of raw types in code written after the introduction of generics into the Java programming language is strongly discouraged. It is possible that future versions of the Java programming language will disallow the use of raw types.

That last sentence really helps me understand as if "raw type" was meant to apply to how the class/bean was defined, it'd basically be saying it's bad to leave undefined generic types in your class. That's obviously the point of generics, so it's more understandably saying "don't reference generic classes without supplying parameters."

Experimentations

No unbound types

I created a class that has 3 generic types, but doesn't leave any of the types undefined (unbound).

public static class EveryFlavorBean implements
        Comparator<File>,
        Predicate<String>,
        Consumer<URI> {
    // ...
}

No decisions were left to the referring code (i.e. it can't be a raw type, nor can parameters be supplied) and the compiler will enforce things like this:

image

Mixed bound and unbound types

I updated that class that has 3 bound generic types so there is a 4th generic type that is unbound (Supplier<T>)

public static class EveryFlavorBean<T> implements
        Supplier<T>,
        Comparator<File>,
        Predicate<String>,
        Consumer<URI> {
    // ...
}

The value of T was left to the referring code to decide.

referenced as a raw type

If the referring code does not supply parameters (uses it as a raw type), the compiler does not seem to enforce any of the generics, even the other 3 that are defined.

image

referenced as a parameterized type

If the referring code does supply parameters, even if that is ?, the compiler will enforce all the generics it can and we once again get our compile errors back (i.e. different erasure)

image

Final thoughts

Seems from a spec perspective we should ensure it requires us to support both: allowing generic classes that have unbound types to be referred to as raw or parameterized types.

I'm not entirely sure how this would be done in the guts of a CDI implementation or how we would test what types would be exposed during deploy or build-time events.

I'd need to think about it further, but the first instinct would be an approach where the bean definition had as much information it as possible -- all the generic types are visible in the bean definitions even if some are still unbound variables. Extensions can then see all the type information available (know what we know) and act on it or potentially modify it.

For resolution, however, we would need to be prepared to permit both raw and parameterized injection points. This becomes incredibly tricky as CDI flags ambiguous references and this could create a lot of them. The JLS doesn't have that concept, so this would be our problem to solve.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 21, 2023

I find it incredibly important to make a distinction between declarations and types. On many places, the CDI specification doesn't, and so didn't the Java Language Specification many years ago (I'm looking at JLS 8 specifically).

The TCK test being challenged here checks the set of bean types. For managed beans, that set is defined in the CDI specification like so:

The unrestricted set of bean types for a managed bean contains the bean class, every superclass and all interfaces it implements directly or indirectly.

The resulting set of bean types for a managed bean consists only of legal bean types, all other types are removed from the set of bean types.

Similar wording exists for producer beans.

The very first sentence is inaccurate: classes in Java do not extend classes and implement interfaces -- they extend class types and implement interface types. As I said, even JLS 8 wasn't 100% precise about this. Compare chapter 8.1.4 from JLS 8:

The optional extends clause in a normal class declaration specifies the direct superclass of the current class.

with chapter 8.1.4 from JLS 17:

The optional extends clause in a normal class declaration specifies the direct superclass type of the class being declared.

(Emphasis not mine.)

But even JLS 8 admits that the extends clause (as well as the implements clause) denotes a type when talking about generic classes and parameterized types. The grammar production also reads ClassExtends: extends ClassType.

As illustrated above, it is relatively common to refer to declarations when talking about types, which leads to shorter sentences in case the meaning is obvious. There's an inherent ambiguity in that, though. I will further use the terms as JLS 17 uses them and will try to avoid the ambiguity.

In addition to a direct superclass type, JLS also defines what a direct superclass is: the class named by the direct superclass type. Further, the superclass relation is a transitive closure of the direct superclass relation. This is all chapter 8.1.4 (similar definitions exist for interfaces).

Now, chapter 4.10 defines the supertype (and subtype) relation: it is a transitive closure over the direct supertype relation. The direct supertype relation is defined in the same chapter. For a type of a non-generic class, direct supertype = direct superclass type + direct superinterface types; for raw type of a generic class, direct supertype = erasure of direct superclass type + erasure of direct superinterface types; for a parameterized type of a generic class, direct supertype = application of type arguments to the direct superclass type + application of type arguments to the direct superinterface types (note that there's a little bit more to it, I'm shortening here, hopefully without harm).

Finally, chapter 4.8 defines what a raw type is; among others, it defines a raw type of a generic class. It also says:

The superclass types (respectively, superinterface types) of a raw type are the erasures of the superclass types (superinterface types) of the named class or interface.

Unlike direct superclass type, the "superclass types" are not defined anywhere, which is a little confusing. I take it the implied meaning is "the types of superclasses, as defined by the supertype relation", or something like that. (The term is used on several other places outside of chapter 4.8, and I believe it is used in accordance with my interpretation here.)

In that sense, MyRawBean implements MySuperInterface (which is a raw type) and does not implement MySuperInterface<Number>.

I'd love to hear a different interpretation of the JLS that leads to different results, but so far, I haven't.

@rmannibucau
Copy link

I can agree it is maybe not explicit but it is the case both ways and ultimately the intent is to be aligned on java usage while it matches CDI usage and here there is no ambiguity that MyRawBean is a MySuperInterface<Number> even if the wording is maybe not as clear as desired so CDI should just comply to not lead to cases where it goes against java (if not then means MyRawBean is a MySuperInterface<String> which is obviously wrong and will not compile nor run).

@manovotn
Copy link
Contributor

I can agree it is maybe not explicit but it is the case both ways and ultimately the intent is to be aligned on java usage while it matches CDI usage and here there is no ambiguity that MyRawBean is a MySuperInterface<Number> even if the wording is maybe not as clear as desired so CDI should just comply to not lead to cases where it goes against java (if not then means MyRawBean is a MySuperInterface<String> which is obviously wrong and will not compile nor run).

I don't see how this fits the JLS interpretation or proves Ladislav's earlier in-depth explanation wrong?
I'd rather stick with mathematical notations to stay precise because statements such as

If we try to phrase it easier and without the mathematical notations to really make it obvious: types must respect reflection

are very broad, vague and easily lead to inaccuracies.

@rmannibucau
Copy link

Ok so let's do the exercise:

  • JLS 8.1.4 is about inheritance (classes) so NOT MyInterface part except in terms of dependency (which has no ambiguity in this issue which is 100% about generics)
  • 8.1.5 clarifies the point

Given a generic class declaration C<F1,...,Fn> (n > 0), the direct superinterfaces of the parameterized class type C<T1,...,Tn>, where Ti (1 ≤ i ≤ n) is a type, are all types I<U1 θ,...,Uk θ>, where I<U1,...,Uk> is a direct superinterface of C<F1,...,Fn> and θ is the substitution [F1:=T1,...,Fn:=Tn].

In this ticket example there is NO link between the class generics and the interface so substitutions are only done in MyInterface parent hierarchy independently of the classes implementing it.

So there is no erasue per JLS.

This is compliant with the compiler and runtime behaviors of Java.

If we apply it we get that:

Data:

class MyRawBean extends MyBean
interface MyBean<T> implements MyInterface
interface MyInterface extends MySuperInterface<Number>

Logic:

direct_types(MyRawBean) = { MyRawBean, MyBean }
direct_types(MyBean<T>) = { MyBean<T>, MyInterface }
direct_types(MyBean) = { MyBean, MyInterface } # substitutions applied, there i no substitution to do there so it is mainly a noop
direct_types(MyInterface) = { MyInterface, MySuperInterface<Number> }

Then using 8.1.4 and 8.1.5+9.1.3 you get the transitive nature of the inheritance:

types(MyRawBean)
= { MyRawBean, types(MyBean) }
= { MyRawBean, MyBean, types(MyInterface)  }
= { MyRawBean, MyBean, MyInterface, MySuperInterface<Number>  }

So at the end we comply to JLS, to java compilers, to java runtimes and CDI event and extension requirements so all good, no?
Just the test to fix.

QED?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 21, 2023

Okay, I'll repeat the classes, as you got them wrong:

class MyRawBean extends MyBean {}
class MyBean<T> implements MyInterface {}
interface MyInterface extends MySuperInterface<Number> {}
interface MySuperInterface<T> {}

It is very obvious that MyRawBean extends the raw type of MyBean.

Quoting JLS 8, as you did, we have

The superclasses (respectively, superinterfaces) of a raw type are the erasures of the superclasses (superinterfaces) of any of the parameterizations of the generic type.

So if we have

superclasses_and_superinterfaces(MyBean<X>)
    = { MyBean<X>, MyInterface, MySuperInterface<Number>, Object }

// doesn't really matter what `X` is concretely; per above, "any of the parameterizations" will do

We also have

superclasses_and_superinterfaces(MyBean)
    = { erasure(MyBean<X>), erasure(MyInterface), erasure(MySuperInterface<Number>), erasure(Object) }
    = { MyBean, MyInterface, MySuperInterface, Object }

From there, we see how the following works:

superclasses_and_superinterfaces(MyRawBean)
    = { MyRawBean, superclasses_and_superinterfaces(MyBean) }

So: any reason why the provisions of chapter 4.8 do not apply here?

@rmannibucau
Copy link

@Ladicek my point is that chapter 4.8 applies but does not involve MySuperInterface erasure.

The superclass types (respectively, superinterface types) of a raw type are the erasures of the superclass types (superinterface types) of the named class or interface.

Start from MyRawBean, its super class is the MyBean (with erasure) then MyInterface (no erasure since no generic) then you do the same but the 4.8 only deals with super[class|interface] since the rest is done recursively propagating visited types - as in plain Java - so you do it on MyInterface which does not have any erasure so you keep MySuperInterface<Number>.

4.8 does not mean "if there is an erasure everything is erased" but only "if there is an erasure the the related types are erased recursively". It is better explained in parts 8.

Gues the error is

= { erasure(MyBean), erasure(MyInterface), erasure(MySuperInterface), erasure(Object) }

This is not the case. I agree erasure(MyInterface) which is MyInterface and implies MySuperInterface<Number>.

@manovotn
Copy link
Contributor

Without resolution to JDK-8044366, you IMO shouldn't jump to the conclusion that one of those behaviors is correct as you can then end up violating JLS with CDI requirement.
Either way, this discussion is out of scope of this ticket and should have its own under cdi GH repo.

@jeanouii
Copy link
Contributor Author

@Ladicek I can send a PR of course

jeanouii added a commit to jeanouii/cdi-tck that referenced this issue Feb 24, 2023
@jeanouii
Copy link
Contributor Author

@Ladicek #433 should be enough for this challenge

@dblevins
Copy link
Contributor

From process perspective, the only resolutions to a test challenge are approved or invalid.

Fair enough, guess we'll need to take the approved route here to get it in.

@manovotn @Ladicek if you're both ok with the approved path, can you apply the approved label?

Implementation teams should really not be approving their own challenges, so that rules out @struberg @rmannibucau @jeanouii and myself.

@manovotn
Copy link
Contributor

Oh, I thought I already added that yesterday, sorry - will fix it now

@manovotn manovotn added the accepted Issue/challenge is accepted label Feb 24, 2023
@manovotn manovotn linked a pull request Feb 24, 2023 that will close this issue
@dblevins
Copy link
Contributor

@manovotn no worries and thank you everyone for the considerable amount of time on this.

@struberg
Copy link

+1 Thank you @manovotn !

@struberg
Copy link

struberg commented Feb 25, 2023

Guess plan is to get an approval on this challenge, make the test tolerant and fix the spec either by breaking the raw type support or by enforcing the generic usage when present in the runtime

There is actually not a real raw type support in the Spec for this specific case as of yet. I remember we've been ambiguous in CDI-1.0, but this got pinned down a long time ago.

The following cases are valid and do work:

    public static class InjectionBean {
        @Inject MySuperInterface<Number> superInterface;
    }

and

    public static class InjectionBean {
        @Inject MySuperInterface<?> superInterface;
    }

But the following code must result in an UnsatisfiedResolutionException

    public static class InjectionBean {
        @Inject MySuperInterface<String> superInterface;
    }

This is also not allowed afaict:

    public static class InjectionBean {
        @Inject MySuperInterface superInterface;
    }

@manovotn
Copy link
Contributor

Typesafe resolution is based on bean types and in this issue, we've agreed that bean types in this scenario aren't clear.
As such, you cannot arrive at the implications you stated and hold them true for all implementations @struberg.

I understand that that's how it works for OWB because those are the types you end up getting. which is why I asked about it in my earlier comment (see #429 (comment)).
However, as Weld ends up having MySuperInterface as bean type but not MySuperInterface<Number> - literally what the TCK expects before we merge #433 - the injection points you stated will (or won't) be satisfied slightly differently.

@rmannibucau
Copy link

@manovotn not sure I get your last comment, you mean with weld what would be the output of:

public static void main(final String... args) {
    try (final var container = SeContainerInitializer.newInstance().initialize()) {
        final var nbr = container.select(new TypeLiteral<MySuperInterface<Number>>() {
        }).get();
        System.out.println(nbr);

        final var str = container.select(new TypeLiteral<MySuperInterface<String>>() {
        }).get();
        System.out.println(str);
    }
}

It is the same than openwebbeans so the bean types must (as must in the spec) be aligned on the resolution so the behavior you explained before is wrong (I guess weld uses a bean type + resolution pass to make it behaving properly but as we saw it is against the spec which must have a consistent list of types used for resolution without another filtering pass on this aspect for this case), no?

@dblevins
Copy link
Contributor

As this challenge is now accepted, we should likely move the conversation of what to do next somewhere else. I've filed an issue jakartaee/cdi#653 to hopefully provide a place we can continue to discuss. I recommend we close this issue.

@rmannibucau
Copy link

rmannibucau commented Feb 26, 2023

It is more about the fix for the challenge. Shouldnt be relaxed but really refined.

@manovotn
Copy link
Contributor

@manovotn not sure I get your last comment, you mean with weld what would be the output of:

final var nbr = container.select(new TypeLiteral<MySuperInterface<Number>>() {}).get();

MyRawBean bean in Weld does not have the type MySuperInterface<Number> so it won't resolve this particular bean.
Instead, we assign it the raw type MySuperInterface, so what would work in Weld but (from what I understood) not in OWB is:

CDI.current().getBeanContainer().createInstance().select(MySuperInterface.class).get()

The other example:

final var str = container.select(new TypeLiteral<MySuperInterface<String>>() {}).get();

This is unsatisfied and I don't think there is any dispute about that in this issue whatsoever; this follows assignability rules for beans with type params and there is no way you can assign Foo<Number> to Foo<String>.

It is more about the fix for the challenge. Shouldnt be relaxed but really refined.

If that's what you think then I am not sure we've reached an agreement on how to fix it.
The test actually defines how it should behave as is - but due to the JDK issue, there is a good point in that it can be interpreted in two ways. I am all in for allowing both (doesn't break anything that wasn't already broken for as long as this test exists anyway), but if you instead aim to turn it around and enforce the other interpretation exclusively, that's something I wouldn't agree with :)

@rmannibucau
Copy link

Well, from weld types the string resolution should work - was even explained as intended in this thread. It is obviously not the case so weld and owb behave the same on resolution, spec does align resolution and types so means weld should be aligned on that in its type list. Then once done raw type of super interface has nothing to do there due to cdi parameterized type resolution (so types) rules where param match exactly.

@manovotn
Copy link
Contributor

Well, from weld types the string resolution should work - was even explained as intended in this thread. It is obviously not the case so weld and owb behave the same on resolution, spec does align resolution and types so means weld should be aligned on that in its type list. Then once done raw type of super interface has nothing to do there due to cdi parameterized type resolution (so types) rules where param match exactly.

Sorry but I can't see where you're headed with this.
There are two different things we are mixing now.

Firstly, you need to determine types of any given bean. This is where Weld and OWB differs and we've gone over why and that none of the approaches is right or wrong and that both should be accepted due to the JDK bug and possibly aligned after that JDK bug is resolved. Agreed?

Secondly, you then use previously determined set of types to perform typesafe resolution. In this, both impls perform the same (correct) job as far as I can tell.
However, since in this specific case the entry set of bean types differs for both impls, you therefore cannot expect the same result of the typesafe resolution WRT to that one conflicting type (MySuperInterface versus MySuperInterface<Number>).

@rmannibucau
Copy link

@manovotn agree we differ in 1st and behave the same in 2nd but CDI spec requires 1 and 2 to be aligned so 2 is the behavior and types you must (as must in the spec) see in types.

@manovotn
Copy link
Contributor

but CDI spec requires 1 and 2 to be aligned so 2 is the behavior and types you must (as must in the spec) see in types.

Type resolution says nothing about what types you should see there, it only defines matching rules that work on predetermined types.

@rmannibucau
Copy link

The bean types and qualifiers of a bean determine where its instances will be injected by the container, as defined in Dependency injection and lookup.

(part 2.1)

The bean types of a bean are used by the rules of typesafe resolution defined in Typesafe resolution.

part 2.1.2 and if you follow the part 2.4.2 there is no magic there enabling weld behavior.

Concretely type resolution defines what must be in types.

@manovotn
Copy link
Contributor

part 2.1.2 and if you follow the part 2.4.2 there is no magic there enabling weld behavior.

More like nothing disabling weld behavior?
2.1.2 is just how you get types - there is nothing preventing you from having a raw type. In fact, the spec literally says:

A bean type may be a raw type.

So yes, we arrive at different types in this one particular case due to the JDK bug.

Which is exactly why 2.4.2 works for Weld just as well as for owb 🤷

@rmannibucau
Copy link

@manovotn what you state is right but you don't put things together. Indeed you can get a raw type but you have to use the types present in the bean to resolve the beans at runtime, weld obviously does something more there since it does not have the parameterized type but resolves it properly. Doing this additional step is againsrt the spec (once again the rational being to present the same view to extensions and runtime).

@Ladicek
Copy link
Contributor

Ladicek commented Feb 27, 2023

I'd just like to point out that I believe there is nothing ambiguous in the CDI specification itself. We're "just" hitting an ambiguity in the JLS.

My understanding of that JLS ambiguity is that it allows raw MySuperInterface and parameterized MySuperInterface<Number> to be superinterface types of MyRawBean, so that is what the TCK test should allow.

The comments above that MySuperInterface<String> can also be a superinterface type of MyRawBean are misinterpretations. The fact that new MyRawBean() can be assigned to MySuperInterface<String> only proves that new MyRawBean() can have a type of raw MySuperInterface and that an unchecked conversions occur as they should -- it does not mean that MySuperInterface<String> is a superinterface type of MyRawBean (if it was, there would be no need for the unchecked conversion).

@rmannibucau
Copy link

@Ladicek thing is there is nothing ambiguous on the resolution - and it works well for all impl - and spec states resolution and types are aligned so agree there is nothing ambiguous but proves test is wrong.

@manovotn
Copy link
Contributor

weld obviously does something more there...

I don't know how else to say that, there is no extra step. We identify set of types and we use them and nothing else to perform typesafe resolution.

...since it does not have the parameterized type but resolves it properly.

What parameterized type? MySuperInterface<Number>? Maybe you just misread the earlier comment? We do not have this type in the set of types for MyRawBean and therefore do not match MyRawBean for injection points of MySuperInterface<Number>.

@rmannibucau
Copy link

Ok, got it, my error was that weld resolves MyBean and not MyRawBean, mea culpa. Fine to stick on the generic resolution definition, if it is recursive (OWB) or absolute (Weld) at spec level. Thanks for that last rephrasing @manovotn .

manovotn pushed a commit that referenced this issue Feb 27, 2023
* Issue #429 Tolerate both MySuperInterface and MySuperInterface<Number> for raw-type types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue/challenge is accepted challenge TCK test challenge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants