8274617: constructor and parameter annotations are not copied to the anonymous class constructor #6548

vicente-romero-oracle · 2021-11-24T22:34:29Z

Please review this PR which is about propagating constructor and parameter annotations to the anonymous class constructor. Currently we are propagating them to bridge methods which are synthetic. Propagating them to the anonymous class constructor seems sensible given that they are mandated by the spec. Please review also the related CSR,

TIA

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-8274617

Issues

JDK-8274617: annotations in the super class constructor are not propagated to the anonymous class constructor ⚠️ Title mismatch between PR and JBS. ⚠️ Issue is not open.
JDK-8277679: annotations in the super class constructor are not propagated to the anonymous class constructor (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6548/head:pull/6548
$ git checkout pull/6548

Update a local copy of the PR:
$ git checkout pull/6548
$ git pull https://git.openjdk.java.net/jdk pull/6548/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6548

View PR using the GUI difftool:
$ git pr show -t 6548

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6548.diff

…anonymous class constructor

bridgekeeper · 2021-11-24T22:36:05Z

👋 Welcome back vromero! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-11-24T22:39:08Z

@vicente-romero-oracle The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-11-24T22:44:31Z

Webrevs

vicente-romero-oracle · 2021-11-29T18:44:28Z

/csr

openjdk · 2021-11-29T18:45:45Z

@vicente-romero-oracle this pull request will not be integrated until the CSR request JDK-8277679 for issue JDK-8274617 has been approved.

lahodaj · 2021-11-29T19:47:56Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java

@@ -1337,9 +1337,17 @@ public Type constructorType() {

        @Override
        public MethodSymbol constructorSymbol() {
+            // we should do this only once
+            boolean copyConstAnnos = constructorSymbol == null;


Question: can we avoid repeating this method completely if constructorSymbol != null? Seems a bit weird to not copy the annotation again (which I understand), but redo all the rest again.

yep good point

lahodaj · 2021-11-29T19:48:34Z

test/langtools/tools/javac/annos_not_copied_anonymous_ctr/AnnosNotCopiedToAnonymousCtrTest.java

+    public class Test {
+        @VisibleCtrAnnotation
+        @InvisibleCtrAnnotation
+        public Test(String firstParam, @VisibleParamAnnotation String secondParam, @InvisibleParamAnnotation String thirdParam) {}


How about type annotations of the parameter types? Should these be copied? (And tested?)

yep I should add a test for type annotations, good point

lahodaj · 2021-11-29T19:51:35Z

test/langtools/tools/jdeps/listdeps/ListModuleDeps.java

@@ -95,6 +95,7 @@ public void compileAll() throws Exception {
                                "java.base/jdk.internal.javac",
                                "java.base/jdk.internal.jmod",
                                "java.base/jdk.internal.misc",
+                                "java.base/jdk.internal.vm.annotation",


This looks a bit suspicious to me - why the new entry? Do we know why it was necessary? (I would expect that any change here would be caused by a change in dependencies among modules, but I don't seem to see that change.)

tbh I'm not sure why this new dependency appeared there is no new import, I will do some research

lahodaj · 2021-12-07T18:07:17Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java

+                         * given parameter
+                         */
+                        if (!param.getRawTypeAttributes().isEmpty()) {
+                            params.append(make.VarDef(param, make.Ident(param.type.tsym), null));


I guess I would only use this branch, whether or not there is a type annotation. (Would be much better tested, among other things.)

lahodaj · 2021-12-07T18:12:06Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TypeEnter.java

+
+            private List<JCTypeParameter> typeParams(List<Type> typarams) {
+                ListBuffer<JCTypeParameter> tparams = new ListBuffer<>();
+                final AtomicInteger paramIndex = new AtomicInteger(0);


The AtomicInteger is a big weird (it is the first time it is used in javac, believe). I wonder if using some alternative (like an array) would not fit better.

liach · 2021-12-19T01:53:53Z

Can these annotations be detected properly by reflection at runtime (when, say, compiled with -parameters flag to avoid a few other existing issues)?

vicente-romero-oracle · 2021-12-20T14:29:23Z

Can these annotations be detected properly by reflection at runtime (when, say, compiled with -parameters flag to avoid a few other existing issues)?

sorry I'm not sure I'm following you, the -parameters option deals with the parameter names it has nothing to do with annotations

liach · 2021-12-22T00:04:08Z

Hmm, since both annotation and method signature attributes' recorded signature/annotations don't exactly correspond to the parameters in the descriptor, I thought they probably would share the same mechanism of determining the exact parameter mapping. Reflection uses MethodParameters attribute's modifiers to determine how signatures apply to parameters, while it handles annotation in a separate way.

This indeed doesn't matter as the extra local variables passed are in the end of parameter list, which doesn't impact the current way annotations are written to class files and read at runtime. Sorry for digressing here.

bridgekeeper · 2022-01-19T05:41:51Z

@vicente-romero-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-02-16T10:44:23Z

@vicente-romero-oracle This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8274617: constructor and parameter annotations are not copied to the …

5fa6a07

…anonymous class constructor

removing dead code

7d95951

openjdk bot added the rfr Pull request is ready for review label Nov 24, 2021

openjdk bot added the compiler compiler-dev@openjdk.org label Nov 24, 2021

openjdk bot added the csr Pull request needs approved CSR before integration label Nov 29, 2021

lahodaj reviewed Nov 29, 2021

View reviewed changes

vicente-romero-oracle added 2 commits November 30, 2021 12:52

addressing review comments

69aecf9

adding support for type annotations

2ed4c22

lahodaj reviewed Dec 7, 2021

View reviewed changes

vicente-romero-oracle added 3 commits December 8, 2021 12:44

Merge branch 'master' into JDK-8274617

15f1417

addressing review comments

2e90dd1

additional tests

c779913

bridgekeeper bot closed this Feb 16, 2022

8274617: constructor and parameter annotations are not copied to the anonymous class constructor #6548

8274617: constructor and parameter annotations are not copied to the anonymous class constructor #6548

Uh oh!

Conversation

vicente-romero-oracle commented Nov 24, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Integration blocker

Issues

Reviewing

Uh oh!

bridgekeeper bot commented Nov 24, 2021

Uh oh!

openjdk bot commented Nov 24, 2021

Uh oh!

mlbridge bot commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vicente-romero-oracle commented Nov 29, 2021

Uh oh!

openjdk bot commented Nov 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Dec 19, 2021

Uh oh!

vicente-romero-oracle commented Dec 20, 2021

Uh oh!

liach commented Dec 22, 2021

Uh oh!

bridgekeeper bot commented Jan 19, 2022

Uh oh!

bridgekeeper bot commented Feb 16, 2022

Uh oh!

Uh oh!

vicente-romero-oracle commented Nov 24, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 24, 2021 •

edited

Loading