Assignability of raw and parameterized types for type variables with multiple bounds (CDI-440)

[ljnelson]

I happened to notice that CDI-440 does not seem to have any resolution in the current version of the CDI specification.

Following the rules in the Jira issue, which read:

The CDI project is part ofJakarta and uses GitHub issues as it's issue tracking system.
Therefore, all issues in CDI JIRA project are being bulk-closed as described in this GitHub issue.
If you feel like this particular issue deserves ongoing discussion, investigation or fixes in CDI/CDI TCK, please create a new issue under GitHub repository and include a link to this JIRA.
For specification related question/issues, please use - https://github.com/eclipse-ee4j/cdi/issues
For CDI TCK related questions/issues, please use - https://github.com/eclipse-ee4j/cdi-tck/issues

…I am filing this issue to resurrect CDI-440.

The original issue reads:

Rules defined in section '5.2.4. [2.4.2.4] Assignability of raw and parameterized types' do not consider the case when a type variable has multiple bounds.

For example, the bullet

the required type parameter and the bean type parameter are both type variables and the upper bound of the required type parameter is assignable to the upper bound, if any, of the bean type parameter.

should have meaning similar to:

... for each upper bound T of the bean type parameter, there is an (at least one) upper bound of the required type which is assignable to T.

Consider the following example:

• interfaces Bar, Baz, Foo

class BarImpl implements Bar { ... }
interface MyInterface<T> { ... }

• bean:

class MyBean<T extends Bar & Baz & Foo> implements MyInterface<T> { ... }

• injection point:

class TestClass<U extends BarImpl & Baz> {
@Inject
MyInterface<U> bean;
}

MyBean should not be assignable to this IP as U does not have the bound Foo. However, if e.g. BarImpl implemented also Foo, it should be OK, i.e. MyBean could be injected.

Please, correct me if I am wrong.

A comment indicates that this interpretation would seem to be correct, but it was never adopted.

There is a related issue that indicates that Weld adopted and implemented this unofficial interpretation.

The TCK appears to perhaps enforce this unofficial interpretation as well.

I'm not sure the suggested language is exactly correct, but the spirit of it seems correct.

[manovotn]

You're right that the TCK test seems to cover exactly this and has been part of the testsuite since around the time of the CDI JIRA issue (May/June 2014).

I also agree that this functionality makes sense. My guess would be that the wording change was simply forgotten back then and injections like these are probably pretty rare so it didn't really surface again. I'd consider it a clarification and add it to the next release unless there are objections OFC.

The only issue I have with this is that I am not sure how to word this so that this especially hard-to-read-and-understand section of the spec doesn't get even worse.
@ljnelson are you willing to send a PR?

[ljnelson]

I can try but it will take a while because I am not immediately smart enough to understand the replacement. Additionally, there are many problems with the terminology in that section and I don't want to make the terminology misuse even worse. So should I:

1. Keep the incorrect terminology in any wording I add to address this case
   • Pros: adds the least amount of text possible
   • Cons: perpetuates confusing terminology usage
2. Correct the terminology in the section overall while adding text to address this case
   • Pros: Fixes broken windows
   • Cons: the terminology misuse is widespread throughout the spec; addressing it here only might not be a good idea
3. Write a giant PR that tries to correct terminology everywhere
   • Pros: sure needs doing
   • Cons: huge, probably not practical

…? For arbitrary examples, the whole section uses "parameter" when it probably means "argument"; "raw type" appears to actually mean "non-generic or raw type", terms like "actual type" are undefined except in comments in closed bugs, "match" is undefined, and so on.

Maybe a larger PR is in order that attempts to preserve the exact current meaning in the spec but that uses proper terminology? Maybe it would be huge? Maybe it wouldn't be so bad? I don't know.

[manovotn]

Personally I don't have that much of an issue with the incorrect terminology but I guess I might just be used to it over the years :)
I am afraid that with option 3 it will be difficult to make sure it preserves the functionality (basically what is asserted by TCKs as I don't expect we want to change the scope of that). That being said, if you are willing to give it a go, I am not against it. Though it might be smarter to start with what you described in 2. and if that's agreed upon, move on to 3.?

Let's see what others think. Cc @Ladicek

FTR as far as huge PRs go, that's not an issue as we are now talking 4.1 which is next bigger release (EE 11), so changes like that are not blocking in any way.

[Ladicek]

I have noticed some of these inaccuracies myself (type parameter vs type argument is the most obvious one), but haven't been brave enough to do something about it :-) In my opinion, one good way to address this is incremental. Address this issue in a single PR. Address the confusion between type parameters and type arguments in another PR. Address the term "actual type" in yet another PR. And so on.

[ljnelson]

So (1)?

[manovotn]

I guess what Ladislav says is he isn't against correcting this throughout the spec (nor am I) but we should do that incrementally.
Whether that means separate PRs or separate commits in a single PR is of little importance to me :)

For instance, I'd start with a PR that fixes just (1) - that way we have bad terminology but full scope of functionality covered.
Once that's in the code, propose a PR that does (2) as a example of how to improve the terminology in a single section and from there incrementally add commits into that PR to fix that in other various places in the spec.
Does that seem sensible to you @ljnelson @Ladicek?

[Ladicek]

I probably wasn't entirely clear, so let me rephrase. I'm suggesting to fix one thing at a time, but that one thing that is being fixed or improved should ideally be fixed or improved throughout the spec.

[Ladicek]

Also, for this particular issue, here's a proposal: Ladicek@7ca846c (I couldn't resist and improved the wording for class types and array types too.)

To be fair, I don't really understand the case when the required type parameter is a wildcard and the bean type parameter is a type variable. Specifically, why should the bound(s) of the type variable be assignable to or assignable from the upper bound? That seems pretty weird to me.

Considering an injection point of

@Inject
List<? extends CharSequence> list;

then all these beans would be assignable:

@Dependent
class MyBean1<T extends CharSequence> implements List<T> {
}

@Dependent
class MyBean2<T extends String> implements List<T> {
}

@Dependent
class MyBean3<T extends Object> implements List<T> {
}

I'm not trying to change that right now, but I'd like to hear anyone's opinion on that.

[manovotn]

...be assignable to or assignable from the upper bound? T

Yea, I agree this seems weird. The lower bound bit of text doesn't contain this wording either.
I don't really see why MyBean3 should be valid - might be worth checking to what extent do we test this in the TCKs too

[ljnelson]

My perhaps incorrect understanding is that for better or for worse the bean type argument T extends Object is not "CDI assignable" to the required type argument ? extends CharSequence because normal Java/covariant assignment semantics are not actually in play here at all, so CharSequence does not "match" Object and Object does not "match" CharSequence.

(The fact that "match" is only partially defined I'll leave untouched for now.)

From looking at Weld code, the only time covariant assignability does come into play is with multiple type parameter bounds (which is not applicable in your example). So that's here (https://github.com/weld/core/blob/c538ec4f6a1a37a89bc3afa5c59babe6597c01da/impl/src/main/java/org/jboss/weld/resolution/BeanTypeAssignabilityRules.java#L178), but the method addressing your example is here: https://github.com/weld/core/blob/c538ec4f6a1a37a89bc3afa5c59babe6597c01da/impl/src/main/java/org/jboss/weld/resolution/BeanTypeAssignabilityRules.java#L162.

Again, it's hard to understand all this so I might be making a mistake.

[manovotn]

Hm, I think you might be mixing up a few things. The spec wording operating on the bounds of the wildcard and bean type variable and allowing you to compare assignability both, from and to the upper bound effectively means that T extends Object will be a valid candidate (which to me seems weird at best).

Indeed, if you try Ladislav's example in Weld, you'll see that all three beans are considered valid.

I've taken a look at the TCKs and found this test.
If I am reading it correctly, it is the same case as we're discussing here for T extends Object:

• required type is Result<? extends RuntimeException, ? super RuntimeException>
• bean type is ResultImpl<T1 extends Exception, T2 extends Exception> implements Result<T1, T2>

Which means you will be checking assignability of T1 extends Exception to or from the wildcard upper bound ? extends RuntimeException.
All of the above sadly doesn't get us any closer as to why is this specced but it seems fairly intentional because it is also explicitly tested 🤔

[Ladicek]

I actually think the word "match" is defined fairly well in the CDI specification, specifically in the paragraph that starts with "The bean has a bean type that matches the required type". The rest of the paragraph describes a relation match(type, type) by enumerating all the pairs of types that match. Implicitly, all other pairs of types don't match. It is imprecise (incorrect, I dare say) for class types (not mentioned at all) and for array types (defined in terms of element type, while it should be defined either as identity, or in terms of component type), but other than that, it seems fine to me.

On the other hand, it is very true that the CDI specification has its own definition of types being "assignable to" one another. It seems to me that this definition sometimes refers to itself, recursively, while other times, it refers to the Java definition. That isn't detailed anywhere, unfortunately, but CDI for example never defines what "assignable from" means and yet the term is used on several places -- it seems clear to me that that is the Java notion of assignability.

[Ladicek]

Ah and one more thing I wanted to say: it is unfortunate that the Weld codebase uses the term "match" on so many places, but that shouldn't be used to infer any conclusions about the CDI specification.

[Azquelt]

Specifically, why should the bound(s) of the type variable be assignable to or assignable from the upper bound? That seems pretty weird to me.

Thought about this a bit and had an idea:

Only Dependent beans are allowed to have parameterized types. So when you're trying to evaluate whether a bean with type parameters is valid for a required type, you're asking "is there any value I could use for the type variables that would allow it to be valid here".

So with your MyBean3 example, the injection should be valid because it would be valid to write this:

List<? extends CharSequence> list = new MyBean3<String>;

Does that sound reasonable?

[Ladicek]

Only Dependent beans are allowed to have parameterized types.

That is not exactly true (@ApplicationScoped class MyBean implements List<String> { ... } would be valid and would have a bean type of List<String>), the constraint is a little different (generic classes must be dependent, and producer methods with a parameterized return type that contains type variables must be dependent), but the intuition seems headed in the right direction.

Indeed the rule likely only applies to dependent beans, which means that from the type system perspective, it is as if the type variable was substituted for an actual type independently for each injection point. I need to think more about it, but this might be a correct explanation, thanks!

[manovotn]

Does that sound reasonable?

Hmm, that also opens up door for virtually unlimited set of incorrect types that the list can contain - then again, when dealing with generic types on bean (and hence dependent beans) there is plenty of ways to break the type safety if you want to.
So yes, I think you might be right. Thanks for chiming in! :)

[ljnelson]

Ladicek:

I actually think the word "match" is defined fairly well in the CDI specification, specifically in the paragraph that starts with "The bean has a bean type that matches the required type". The rest of the paragraph describes a relation match(type, type) by enumerating all the pairs of types that match. Implicitly, all other pairs of types don't match. It is imprecise (incorrect, I dare say) for class types (not mentioned at all) and for array types (defined in terms of element type, while it should be defined either as identity, or in terms of component type), but other than that, it seems fine to me.

So partially defined.

manovotn:

Hm, I think you might be mixing up a few things. The spec wording operating on the bounds of the wildcard and bean type variable and allowing you to compare assignability both, from and to the upper bound effectively means that T extends Object will be a valid candidate (which to me seems weird at best).

I was indeed. Assignability of bounds is indeed covariant, always.

[manovotn]

@ljnelson feel free to propose a PR with changes and we can continue the discussion there. After all, it's easier to talk code/text than hypothetical changes :)

[manovotn]

@ljnelson are you willing to propose a PR for this change?
We're nearing an M1 milestone for next CDI release (bound to happen late Nov), so I am trying to get a measure of which issues to keep in the milestone and which won't be realized.

[ljnelson]

Probably not in time for the milestone :)