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

ArC fixes for spec compatibility, round 9 #33447

Merged
merged 7 commits into from
May 19, 2023
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 17, 2023

Related to #28558

  • support Bean.getInjectionPoints() in the strict mode
    • strict-mode only to avoid increasing generated code size, as we consider Bean.getInjectionPoints() virtually useless
  • fix treatment of the "current" injection point
    • includes a small cleanup of InstanceImpl constructors and static factories
  • fix too strict circular dependency detection
    • this is mainly for static producers and normal-scoped beans
    • includes a small cleanup of a few things (add BeanInfo.isProducer() and isStaticProducer(), bean dependency handling in ComponentsProviderGenerator)
  • fix generated Bean.destroy() methods to catch exceptions
  • treat additional scope annotations correctly, even if there's no corresponding context
    • this is possibly a little controversial, so I can be persuaded to move the scope scanning code from ArC to embedders, but the code that obtains a ScopeInfo just needs to be there
  • fix searching for inherited scopes
    • a non-@Inherited scope annotation on a superclass is not inherited, but it prevents inheriting an @Inherited scope annotation from a further superclass
  • add links to TCK challenges to some test exclusions

@Ladicek Ladicek requested review from mkouba and manovotn May 17, 2023 13:00
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label May 17, 2023
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks!

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 17, 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.

Awesome number of fixes for TCKs! LGTM

@quarkus-bot

This comment has been minimized.

@Ladicek
Copy link
Contributor Author

Ladicek commented May 18, 2023

The GraphQLOpenTelemetryTest.nestedCdiBeanInsideQueryTraceTest test has been failing quite a lot recently.

Ladicek added 7 commits May 19, 2023 10:33
This mainly fixes a bug with `BeanManager.getReference()`, but it also
attempts to cleanup the `InstanceImpl` implementation and adds some
more tests.

This commit would also remove a TCK exclusion for `InjectionPointTest`,
except the test uses stereotype annotations on injection points, which
we consider an error. There's a challenge for it.
ArC used to treat producers as depending on their declaring beans, so if a bean
also has a dependency on one of the producers it declares, circular dependency
exists. This is too strict for `static` producers: they in fact don't depend
on their declaring beans, so no circularity exists in such case. Circular
dependency only exists if the producer is not `static`. This commit fixes that.

When creating the `Bean` instance of a bean that depends on another bean, ArC
either supplies the `Bean` instance of the dependency directly, in case it already
exists, or it allows deferred instantiation in most cases. There used to be one
case that didn't allow deferred instantiation of `Bean` objects: a declaring bean
of a producer. There is no reason for that, deferred instantiation works correctly
even in this case, so this commit allows it.
The `InstanceHandle.destroy()` method used to catch exceptions thrown by
the call to `Bean.destroy()`, but that isn't enough. Per the specification,
the generated `Bean.destroy()` method itself must catch exceptions. This
commit does that; catching exceptions in `InstanceHandle.destroy()` becomes
superfluous, so is removed.
… corresponding context

It makes no sense to register an additional scope annotation without
registering a corresponding context, but the CDI TCK uses that and
it isn't terribly hard to support.

With this commit, ArC treats additional bean defining annotations
that are meta-annotated `@Scope` or `@NormalScope` as scope annotations,
even if there is no corresponding context.

This commit also fixes the type discovery implementations to also discover
scope annotations and register them as additional bean defining annotations.
The CDI specification mandates that if a superclass of a bean declares
a scope annotation, scope annotations from further superclasses are
not inherited. This holds even if the annotation is not `@Inherited`.
This commit fixes searching for inherited scopes to honor this.
@Ladicek
Copy link
Contributor Author

Ladicek commented May 19, 2023

Rebased.

@quarkus-bot
Copy link

quarkus-bot bot commented May 19, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@Ladicek Ladicek merged commit 7bbc002 into quarkusio:main May 19, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 19, 2023
@Ladicek Ladicek deleted the arc-fixes branch May 19, 2023 15:09
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants