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

Fix two small wording mistakes #675

Merged
merged 2 commits into from
May 25, 2023
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 24, 2023

Revert incorrect change in ambiguity resolution

We previously [1] changed the ambiguity resolution wording from

The container eliminates all eligible beans that are not alternatives,
except for producer methods and fields of beans that are alternatives.

to

The container eliminates all eligible beans that are not alternatives
selected for the application, except for producer methods and fields
of beans that are alternatives.

At that time, we did not add a rule to the CDI Full section that would
cover alternatives selected for the bean archive.

Note that for a bean to be eligible, it must be available for injection,
and an alternative is available for injection only if it is selected (for
application in CDI Lite; for application or for bean archive in CDI Full).
Alternative beans that are not selected are hence not even considered during
ambiguity resolution, so the ambiguity resolution description does not need
to mention them at all. This is why the previous description is correct.

Now, only eliminating beans that are not alternatives selected for application,
as the new text says, would mean that in CDI Full, beans that are alternatives
selected for bean archive would also be eliminated. That would be wrong.

Therefore, this change is incorrect. Of course, no implementation actually
made that change in behavior, and the TCK wasn't adjusted either.

There are 2 ways to fix this: revert to previous wording, or add extra wording
to the CDI Full section:

The container eliminates all eligible beans that are not alternatives
selected for the bean archive or selected for the application, except
for producer methods and fields of beans that are alternatives.

This would be similar how ambiguous name resolution is specified.

This commit simply reverts to the previous wording, because that's easier,
even though it leads to slight inconsistency between ambiguous dependency
resolution and ambiguous name resolution. That inconsistency exists since
CDI 1.1, so is not an issue.

[1] 1840939

Fix the definition of availability for injection

We previously [2] changed the definition of availability for injection
from

the bean is either not an alternative, or the module is a bean archive
and the bean is a selected alternative of the bean archive, or the bean
is a selected alternative of the application

to

the bean is either not an alternative, or the module is a bean archive
and the bean is a selected alternative for the application

in the CDI Lite section of the specification. The CDI Full section has
the original wording verbatim, so it does not have an issue.

The new wording is clearly wrong though, because the it mixes the 2nd and
3rd clause of the original sentence. What we should have done is to only
remove the 2nd clause:

the bean is either not an alternative, or the bean is a selected
alternative of the application

This commit does that.

[2] 693139b

Ladicek added 2 commits May 24, 2023 15:46
We previously [1] changed the ambiguity resolution wording from

> The container eliminates all eligible beans that are not alternatives,
> except for producer methods and fields of beans that are alternatives.

to

> The container eliminates all eligible beans that are not alternatives
> selected for the application, except for producer methods and fields
> of beans that are alternatives.

At that time, we did _not_ add a rule to the CDI Full section that would
cover alternatives selected for the bean archive.

Note that for a bean to be _eligible_, it must be _available for injection_,
and an alternative is available for injection only if it is selected (for
application in CDI Lite; for application or for bean archive in CDI Full).
Alternative beans that are not selected are hence not even considered during
ambiguity resolution, so the ambiguity resolution description does not need
to mention them at all. This is why the previous description is correct.

Now, only eliminating beans that are not alternatives selected for application,
as the new text says, would mean that in CDI Full, beans that are alternatives
selected for bean archive would also be eliminated. That would be wrong.

Therefore, this change is incorrect. Of course, no implementation actually
made that change in behavior, and the TCK wasn't adjusted either.

There are 2 ways to fix this: revert to previous wording, or add extra wording
to the CDI Full section:

> The container eliminates all eligible beans that are not alternatives
> selected for the bean archive or selected for the application, except
> for producer methods and fields of beans that are alternatives.

This would be similar how ambiguous name resolution is specified.

This commit simply reverts to the previous wording, because that's easier,
even though it leads to slight inconsistency between ambiguous dependency
resolution and ambiguous name resolution. That inconsistency exists since
CDI 1.1, so is not an issue.

[1] jakartaee@1840939
We previously [1] changed the definition of _availability for injection_
from

> the bean is either not an alternative, or the module is a bean archive
> and the bean is a selected alternative of the bean archive, or the bean
> is a selected alternative of the application

to

> the bean is either not an alternative, or the module is a bean archive
> and the bean is a selected alternative for the application

in the CDI Lite section of the specification. The CDI Full section has
the original wording verbatim, so it does not have an issue.

The new wording is clearly wrong though, because the it mixes the 2nd and
3rd clause of the original sentence. What we should have done is to only
remove the 2nd clause:

> the bean is either not an alternative, or the bean is a selected
> alternative of the application

This commit does that.

[1] jakartaee@693139b
@Ladicek Ladicek added this to the CDI 4.1 milestone May 24, 2023
@Ladicek Ladicek requested a review from manovotn May 24, 2023 13:51
@Ladicek Ladicek changed the title Fix two small mistakes Fix two small wording mistakes May 24, 2023
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, thanks!

@Ladicek Ladicek merged commit 57b1c2c into jakartaee:master May 25, 2023
@Ladicek Ladicek deleted the fix-small-mistakes branch May 25, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants