respecify wildcard type arguments for Event and Instance#982
Conversation
1899887 to
79d50b2
Compare
79d50b2 to
1b5a7aa
Compare
Event and Instance
|
Updated. |
1b5a7aa to
bfc0f2a
Compare
|
Updated once again. I also reverted the change for CDI-232, because that change made the specification of the built-in bean for |
|
I would approve the rest (despite not agreeing with it as it is IMO useless in practice but we've wasted enough time on it already).
Though this I don't like - I understand the current wording, as-is, won't work but I would rather tweak it then revert to the original one. (I also know @mkouba wasn't fond of this when he found out) Original CDI issue link for reference - https://redhat.atlassian.net/browse/CDI-232 |
As much as I hate wasting time on this, I have to disagree. The CDI 2.0 wording does not define a bean properly, because it doesn't say what the set of bean types is. I understand that an infinite set is inconvenient, and it is in fact impossible to implement |
Bean typesThe CDI 1.0 wording for the bean types of the I would expect that the set of bean types for the built-in
I'm not sure what you want here. I would argue that we should not specify the set of bean types in absolute terms. For example, if an implementation had some kind of QualifiersMy main objection to the CDI 2.0 wording is the bullet point The CDI 1.0 wording is not much better. If the intention here is to allow the Looking at this example in the spec, I think that is indeed the intention: @Inject @PayBy(CHEQUE) Instance<PaymentProcessor> chequePaymentProcessor;Conclusion
I would change the wording like this: The container must provide a built-in bean that:
* is eligible for injection to any injection point with required type `Instance<X>` or `Provider<X>`, for any legal bean type `X`, with any qualifiers,
* has scope `@Dependent`,
* has no bean name, and
* has an implementation provided automatically by the container.This is largely the current wording, but with the change that it's the injection point that's allowed to have any qualifiers, not the bean. Technically, stating that the injection point can have any qualifiers doesn't actually change anything (it's implied by omission), but I think it's helpful to explicitly state that requirement because it's weird and can't be fulfilled by a regular bean. In the same way, I guess in the CDI 2.0 wording the requirement "has any qualifiers" doesn't change anything either. The bean can have any qualifiers you like, as long as it fulfils the first requirement that you can inject it into any |
| For an injected `Instance`: | ||
|
|
||
| * the _required type_ is the type parameter specified at the injection point, and | ||
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound (or an unbounded wildcard), in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and |
There was a problem hiding this comment.
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound (or an unbounded wildcard), in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and | |
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard, in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and |
or
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound (or an unbounded wildcard), in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and | |
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard without a lower bound, in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and |
or
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound (or an unbounded wildcard), in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and | |
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound or an unbounded wildcard, in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and |
It's odd to put the unbounded wildcard part in brackets, it's just as important as the wildcard with an upper bound.
However, what we really mean is "a wildcard without a lower bound", so we could just say that instead.
However, we could also leave that part out altogether to simplify this bullet point since we forbid a wildcard with a lower bound later.
There was a problem hiding this comment.
I would just say:
the type argument of
Instanceas specified at the injection point, or its upper bound if the type argument is a wildcard
much shorter and means the same thing.
|
I've left my comments above, everything else I think is fine. |
OK, but why do you think that's not correct? You can't just assert this with no argumentation. Infinite sets are perfectly normal and mathematically well-defined, and there's no particular reason why the CDI spec needs to avoid them when defining a bean in an abstract sense. But the wording of the 1.0 spec does not even imply that the set is infinite, because the CDI bean container lives in a closed world where the complete set of bean types known, so it is in fact a finite set. It's perfectly acceptable for an implementation to interpret "every bean type", and "every qualifier type" to mean "every bean type known to the container" and "every qualifier type known to the container". |
| * type `Event<?>`, or | ||
| * type `Event<? extends X>`, for any type `X`, |
There was a problem hiding this comment.
These two bullet points could be collapsed, since Event<?> is the same type as Event<? extends Object>.
gavinking
left a comment
There was a problem hiding this comment.
Thank you for this change, I very much appreciate it.
The change that was made to this language in CDI 2.0 really messed up the original intention, and I'm very happy to see the original language restored.
| * `Instance<X>` and `Provider<X>` for every legal bean type `X` in its set of bean types, | ||
| * every qualifier type in its set of qualifier types, |
There was a problem hiding this comment.
The following suggestion is in my opinion completely unnecessary, but I do get that people with CS degrees tend to be quite uncomfortable with the notion of infinity in general. This should resolve such discomforts.
| * `Instance<X>` and `Provider<X>` for every legal bean type `X` in its set of bean types, | |
| * every qualifier type in its set of qualifier types, | |
| * `Instance<X>` and `Provider<X>` for every legal bean type `X` in its set of bean types,footnote:[An implementation is permitted to consider the universe of legal bean types to encompass only those bean types known to the bean manager at initialization time.] | |
| * every qualifier type in its set of qualifier types,footnote:[An implementation is permitted to consider the universe of qualifiers to encompass only those qualifiers known to the bean manager at initialization time.] |
@gavinking No, they're not, because then you wouldn't be able to perform the typesafe resolution in a finite time interval. "The bean has a bean type that matches the required type." implies iteration over the set of bean types.
You're right, it's not infinite. It's just a very large set which is often not easy/possible to collect.
Not really, the change merely reflected the current status of implementations because it would be very impractical to follow the spec in this regard, i.e. @Ladicek @manovotn @Azquelt In any case, I think that this discussion is quite useless, waste of time and resources, and it does not bring any real benefits to CDI users. We should devote time to more important topics. |
That makes sense to me.
I want a well defined set of bean types, because that's one of the core properties every bean has. Your suggestion above does that in an elegant way which I'd be fine with. It is technically somewhat different than the original intent, but I think we can just have a well-defined and small set of bean types and qualifiers and use a special provision to override the rules of bean to IP assignability.
This is an extra-specification behavior that doesn't break the spec in any way, so I think it should be fine without the spec going an extra mile to allow this?
Good point.
Correct. On the other hand, I'm certainly not married to it. I think we can just say that the built-in
As mentioned above, I would personally be fine with saying:
And we could do something similar for the built-in
The issue I see is that
This is unfortunately not true, because even if there's no bean of type e.g.
Not really. For typesafe resolution, the set of bean types is used merely as a predicate of "it this required type present in this set of bean types?". If the set of bean types is representable as a finite predicate (which it is in this case), it doesn't matter that the set itself is infinite.
Well, it's one of the corner cases we find, and if we can close them, we're improving the spec. Granted, it's not as fun as adding new features, but it still feels relatively important. |
I'm not sure how this would work, but it does not really matter since we already agreed that the set is actually finite in this case.
In theory, yes. In practice, I don't think so. |
You know very well how this could work: say you have a set of all even numbers -- it is clearly infinite, but the predicate behind is
There are 2 ways to interpret "finite" here: only bean types of all beans, or only types loaded in the JVM. Both are finite, but while the first is straightforward to enumerate, the second is not. You might have thought it's the first, but it is in fact the second -- you can inject |
|
Actually I might be wrong about |
Sorry, reading back what I wrote, it's not very clear. I will try again to explain what I'm getting at. The original wording implies that the set of bean types must be an infinite set, but I think it's quite possible to fulfil all of the other requirements for Therefore, this part of the spec is unnecessarily requiring a specific implementation when I think there are other valid options.
I agree, in an abstract sense, but here we're defining a bean in very concrete sense. The bean must have a In an abstract sense, I think it's fine to describe a bean that behaves during type resolution as if it has an infinite set of bean types and qualifiers, as long as the spec is worded so that I agree that you could implement it that way, as long as all you don't need to call |
|
I think the biggest issue with infinite sets of bean types and qualifiers is that we also expose these sets through the It seems obvious that these sets should be immutable, so all mutating methods can freely throw an exception. Implementing I see 2 possible solutions:
I personally don't mind which way we go, but we should choose one path for both |
|
I added one new commit that implements solution 2 from my previous comment. It feels a lot less intrusive to the core part of the specification and only affects an API that noone actually calls on the I believe this is now ready for merge. |
gavinking
left a comment
There was a problem hiding this comment.
LGTM now, I left a note about one minor grammatical issue.
| The specified type must not be a wildcard type. | ||
| If an injected `Event` has a specified type that is a wildcard type, the container treats it as a definition error. | ||
| If a programmatically obtained `Event` has a specified type that is a wildcard type, non-portable behavior results. | ||
| If programmatically obtaining an `Event` whose specified type is a wildcard without lower bound is attempted, an `IllegalArgumentException` is thrown. |
There was a problem hiding this comment.
There's a grammar issue here: "If programmatically obtaining ...".
There was a problem hiding this comment.
You mean this should read "If programmatically obtaining an Event is attempted whose ..." instead of "If programmatically obtaining an Event whose ... is attempted"? I'm not a native speaker, so I'll gladly be corrected here -- I know the sentence is hard to read.
There was a problem hiding this comment.
Well, first, the sentence is missing a subject, which makes it hard to tell who or what is doing the "programmatic obtaining". But also "If programmatically obtaining an Event is attempted" is very difficult to parse because the passive voice often feels very awkward in English.
You could say:
If the application attempts to programmatically obtain an
Eventwhose specified type is a wildcard without a lower bound, anIllegalArgumentExceptionmust be thrown.
or something like that.
But I would prefer something much more specific:
If a
TypeLiteralrepresenting a wildcard instantiation ofEventwith no lower bound on the specified type is passed to an operation which performs programmatic bean resolution, the operation must throwIllegalArgumentException.
There was a problem hiding this comment.
You could say:
If the application attempts to programmatically obtain an
Eventwhose specified type is a wildcard without a lower bound, anIllegalArgumentExceptionmust be thrown.
Oh this sounds just about perfect! Much better than what I had.
But I would prefer something much more specific:
If a
TypeLiteralrepresenting a wildcard instantiation ofEventwith no lower bound on the specified type is passed to an operation which performs programmatic bean resolution, the operation must throwIllegalArgumentException.
This seems like the same thing, only more detailed? Unless you disagree, I'll use your first proposal.
There was a problem hiding this comment.
Either is fine; yes, they say the same thing.
I liked the second one because I thought the reader might not know how to programmatically request a type with a wildcard. But it's not a big deal.
| /** | ||
| * Obtains the {@linkplain jakarta.enterprise.inject bean types} of the bean. | ||
| * <p> | ||
| * If the set of bean types of the bean is infinite, only the {@link Set#contains(Object)} | ||
| * and {@link Set#containsAll(Collection)} methods may be called on the result; | ||
| * calling the other {@code Set} methods leads to non-portable behavior. |
There was a problem hiding this comment.
So really we're saying you can only safely call contains or containsAll on the result, since you don't know whether the set is infinite.
That might break existing debugging utilities that dump out information about a bean if the non-portable behaviour is to throw a runtime exception, or return an Iterator that does attempt to iterate over the types.
Seems unfortunate.
There was a problem hiding this comment.
I disagree. I explicitly mentioned that all user-defined beans have a finite set of bean types and qualifiers, so all (read-only) methods are usable virtually always.
| * `Instance<X>` and `Provider<X>` for every legal bean type `X` in its set of bean types, | ||
| * all possible qualifiers in its set of qualifiers, |
There was a problem hiding this comment.
I remain opposed to this requirement because:
- it requires every implementation to create a badly behaved implementation of
Setto represent an infinite set - it prevents other implementations of the
Instancebean which would work fine and meet the other requirements ofInstance
For example, I think you could have a bean with:
- bean types
Instance<X>andProvider<X>whereXis a type variable - a special qualifier
@IgnoreQualifiersthat your implementation interprets as "Ignore qualifiers when resolving comparing this bean during typesafe resolution"
I think you could also just have a special bean that your implementation treats differently during typesafe resolution.
I think such an implementation could follow all of the other the rules for Instance, but would not be allowed due to this requirement. I don't understand why we feel it necessary to forbid these implementations.
As a final suggestion, we could put this:
The container must provide a built-in bean that:
* is assignable to an injection point during typesafe resolution as if it has
* `Instance<X>` and `Provider<X>` for every legal bean type `X` in its set of bean types,
* all possible qualifiers in its set of qualifiers,
* has scope `@Dependent`,
* has no bean name, and
* has an implementation provided automatically by the container.However, this argument has gone on long enough, so if you still want to merge it as is I won't protest any longer.
|
|
||
| * the _required type_ is the type parameter specified at the injection point, and | ||
| * the _required type_ is the type argument specified at the injection point, unless the type argument is a wildcard with upper bound (or an unbounded wildcard), in which case the required type is the wildcard's upper bound (or `java.lang.Object`), and |
There was a problem hiding this comment.
I would use Gavin's wording from https://github.com/jakartaee/cdi/pull/982/changes#r3444003069 here.
| * <li>the <em>required type</em> is the type parameter specified at the injection point, and</li> | ||
| * <li>the <em>required type</em> is the type argument specified at the injection point, unless the type argument | ||
| * is a wildcard with an upper bound (or an unbounded wildcard), in which case the specified type is the wildcard's | ||
| * upper bound (or {@code java.lang.Object}), and</li> |
There was a problem hiding this comment.
The suggested change to the spec wording could be made here too.
e62db01 to
11ed2a4
Compare
|
Hi @Azquelt, sorry for not replying sooner, I was too tired of dealing with this issue and was assigned some unexpected work too. I applied a few fixes that you pointed out, but I have no idea what @gavinking would say to just removing infinite sets of bean types / qualifiers and special-casing typesafe resolution instead :-) In any case, here's a specification proposal that uses this approach (solution 1 from my comment #982 (comment)): https://github.com/Ladicek/cdi-spec/commits/event-instance-meaningful-wildcards-alternative/ |
The type parameter of `Event` is naturally contravariant, so injecting `Event<? super FooBar>` is meaningful. Injecting `Event<?>` or `Event<? extends FooBar>` is meaningless. The type parameter of `Instance` is invariant, but all methods that require typesafe resolution have the type parameter in a covariant position, so for the purpose of typesafe resolution, `Instance<? extends FooBar>` is meaningful. For the meaningful wildcards, special cases exist in the definition of an `Event`'s specified type and `Instance`'s required type that force stripping of the wildcard and using its bound. For the meaningless wildcards, a different approach than before is used. Previously, extra rules were added. These additions are removed and instead, the existing rules that prohibit injecting raw `Event` / `Instance` are extended so that injecting these types with meaningless wildcard arguments is prohibited as well.
The change was well-meant, but made the specification of built-in `Instance` a lot worse. Each bean has a set of bean types, and this change removed that definition from the built-in bean for `Instance`.
This commit specifies that the built-in beans for `Instance` and `Event` have: - a finite set of bean types: `Instance<X>` and `Provider<X>` for `Instance` and `Event<X>` for `Event`, where `X` is a type variable without bounds - a finite set of qualifiers: `@Default` This doesn't override requirements stated elsewhere, so both the `Instance` and `Event` beans also have `Object` in their set of bean types, as well as `@Any` in their set of qualifiers. This requires altering the rules for bean assignability. Instead of the usual rules, the `Instance` bean is assignable to all injection points with required types `Instance<X>` or `Provider<X>`, where `X` is a legal bean type, and the `Event` bean is assignabile to all injection points with required type `Event<X>` where `X` is a type without type variables. In both cases, required qualifiers are ignored.
11ed2a4 to
fb9120b
Compare
|
I pushed 2 branches showing my 2 approaches:
This PR now contains the first approach. |
manovotn
left a comment
There was a problem hiding this comment.
Thank you @Ladicek.
I like the current approach (no infinite sets) a lot more, hence my approval.
Let's keep this PR open until Tue CDI meeting. Assuming no objections arise until then, I'd like to merge this so that we can move on to the actual CR/Final release.
The type parameter of
Eventis naturally contravariant, so injectingEvent<? super FooBar>is meaningful. InjectingEvent<?>orEvent<? extends FooBar>is meaningless.The type parameter of
Instanceis genuinely invariant (even though some might claim it should be treated as covariant), so injectingInstance<? extends|super FooBar>is meaningless.This commit adjusts the specification so that only meaningful wildcards are allowed. It uses a different approach than before, where extra rules were added. These additions are removed and instead, the existing rules that prohibit injecting raw
Event/Instance, are extended so that injecting these types with meaningless wildcard arguments is prohibited as well.Further, the only meaningful wildcard (
Event<? super FooBar>) gets special treatment: the specified type is the wildcard's bound. Also, looking upEvent<? extends FooBar>throwsIllegalArgumentException, whereas previously, this was just non-portable behavior.