ArC: decorators fixes #51166
ArC: decorators fixes #51166
Conversation
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| if (Modifier.isAbstract(decoratorClass.flags())) { | ||
| // TODO this check is not precise: we check that decorators do not declare _any_ abstract methods, |
There was a problem hiding this comment.
Do we have a follow-up issue for this TODO?
manovotn
left a comment
manovotn
left a comment
There was a problem hiding this comment.
LGTM
Particularly the solution in the second commit is really nice 👍
Originally, we only looked at methods declared directly in the decorator class, but that's not enough. Decorators may inherit implementations of decorated methods from superclasses and superinterfaces. Note we make sure we don't include non-`public` methods. It is possible to inherit `protected` and package-private methods in the same package, but since all decorated types are interfaces, and interfaces may only declare `public` and `private` methods, and `private` methods are never inherited, inherited decorated methods must be `public`. Attempting to inherit a `protected` decorated method or a package-private decorated method in the same package would fail compilation.
In case multiple decorated types of a single decorator declare the same method (it may have the same signature, but in presence of generic types, it doesn't have to), we used to remember all of them, which later lead to an error when generating the decorating subclass. This commit fixes that: we really only need to remember _one_ method from each decorator, and it doesn't really matter which exact one it is, because all decorated methods are virtual and we invoke them as such.
Ladicek
force-pushed
the
arc-fix-decorators
branch
from
5d5d0a7 to
46e725d
Compare
|
Rebased in order to rerun, and added a fix for #50950. |
This comment has been minimized.
This comment has been minimized.
|
🎊 PR Preview d2bb1ce has been successfully built and deployed to https://quarkus-pr-main-51166-preview.surge.sh/version/main/guides/
|
manovotn
left a comment
manovotn
left a comment
There was a problem hiding this comment.
Initially I thought the third commit will be problematic for a non-normal scoped non-app bean decorated with an app bean decorator but that seems to somehow just work, so I think we're good :)
I'll add a test for this case soon. |
A while ago, we turned generated `_Bean`, `_Subclass` and `_ClientProxy` classes pertaining to non-app beans into app classes to allow decoration by app decorators. This in turn made injection into non-`public` IPs impossible. We fix it in this commit by a bytecode transformation that makes these IPs `public`. However, this might break subclasses (albeit in relatively rare cases), so in case the bean class has subclasses, we error out. Also, this commit turns the generated `_ClientProxy` classes back into non-app, because that allows invoking non-`public` methods. Turning the generated `_ClientProxy` class into app class was arguably a mistake. Finally, this commit removes the `<scope>test</scope>` on 2 dependencies in the `extensions/arc/deployment` module, because when test-scoped dependencies are used in `QuarkusUnitTest.setForcedDependencies()`, they are treated as application archives. If they have the default scope, they are treated as non-app archives, which is exactly what we need in all tests in this module that use `setForcedDependencies()`. To make sure we don't regress again, a comment is added in the POM and all tests that use this feature also assert that the classes from the external archives are indeed loaded by the base runtime CL (and not runtime CL).
Ladicek
force-pushed
the
arc-fix-decorators
branch
from
46e725d to
709c1d6
Compare
|
Added the test to the top commit. |
Status for workflow
|
Status for workflow
|
3 participants
Fixes #50947
Fixes #50948
Fixes #50950