8202471: (ann) Cannot read type annotations on generic receiver type's type variables #851

raphw · 2020-10-24T21:44:22Z

A method's or constructor's owner type might carry annotations on its potential type parameters but is never represented as parameterized type what makes these parameters inaccessible at runtime, despite the presence of parameter type annotations.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Testing

	Linux aarch64	Linux arm	Linux ppc64le	Linux s390x	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)					✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8202471: (ann) Cannot read type annotations on generic receiver type's type variables

Reviewers

Joel Borggrén-Franck (@jbf - Reviewer) ⚠️ Review applies to 811afcba94dd1c8b8a87e19dd63bdfc47c4aecc7

Download

$ git fetch https://git.openjdk.java.net/jdk pull/851/head:pull/851
$ git checkout pull/851

bridgekeeper · 2020-10-24T21:44:30Z

👋 Welcome back winterhalter! 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 · 2020-10-24T21:46:22Z

@raphw The following label will be automatically applied to this pull request:

core-libs

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 · 2020-10-24T21:50:47Z

Webrevs

07: Full (91c518c)
06: Full - Incremental (811afcba94dd1c8b8a87e19dd63bdfc47c4aecc7)
05: Full - Incremental (f043f685539cd4c990697168aabfe614385dda39)
04: Full - Incremental (9e4c20378ec8272d70632471cbea4a55b40ccf9d)
03: Full - Incremental (ac306162bb8906b1c14bbb8aba9ccfd223f72d03)
02: Full - Incremental (fa2bca376ea4f5ff26474648e4e1508d3213f6e8)
01: Full - Incremental (7721c77bb6146be608f74877ee0ca33d1ce0d7a2)
00: Full (864971eabd7b31b452f0ec38a16a25684c9ed1c8)

shipilev · 2020-10-27T19:09:33Z

Hint: if you merge from master, you would get the updated test workflows, which would include the artifact with *.jtr (jtreg output) files in "testsupport". You can then use them to diagnose the testing failures you are getting.

raphw · 2020-11-01T01:06:52Z

Thanks, I had those working on Mercurial and kind of winged the transfer Should work now!

jbf

Hi Rafael,

Thanks for your patience and for the patch, I left a few comments.

jbf · 2020-11-15T19:25:42Z

src/java.base/share/classes/java/lang/reflect/Constructor.java

+        Type o = resolveOwner(enclosingClass);
+        Type t;
+        if (o != null || v.length > 0) {
+            t = ParameterizedTypeImpl.make(enclosingClass, v, o);


I think these should be instantiated by a GenericsFactory. Ideally you shouldn't have to import the reflective object directly.

I added it to the existing generics factory but as a static method since the instance values are not really required.

jbf · 2020-11-15T19:27:50Z

src/java.base/share/classes/java/lang/reflect/Constructor.java

@@ -656,13 +657,22 @@ public AnnotatedType getAnnotatedReceiverType() {
            return null;
        }

+        TypeVariable<?>[] v = enclosingClass.getTypeParameters();


Can you push down most or all of this to resolveOwner? It can then either return a Class instance or an appropriate Type instance.

Pushed it down to the factory.

jbf · 2020-11-15T19:28:39Z

src/java.base/share/classes/java/lang/reflect/Executable.java

            return null;
+        }
+        Class<?> c = getDeclaringClass();
+        TypeVariable<?>[] v = c.getTypeParameters();


Same here, can this be pushed down to a common method that either returns a Class or resolves the appropriate Type instance?

I moved it to an annotation factory class and avoided the import.

jbf · 2020-11-15T19:49:57Z

test/jdk/java/lang/annotation/typeAnnotations/TestReceiverTypeParameterizedConstructor.java

+    class Inner {
+        Inner(TestReceiverTypeParameterizedConstructor<@TypeAnnotation T> TestReceiverTypeParameterizedConstructor.this) { }
+    }
+


It might be good to add a case where the annotated type variable isn't on the immediate outer type like:

class Inner2 { class TwiceInner { TwiceInner(TestReceiverTypeParameterizedConstructor<@TypeAnnotation T>.Inner2 TestReceiverTypeParameterizedConstructor.Inner2.this) { } } }

Same for the method test.

I added two tests for it.

jbf · 2020-11-16T10:23:23Z

src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java

@@ -89,6 +90,26 @@ public static CoreReflectionFactory make(GenericDeclaration d, Scope s) {
        return new CoreReflectionFactory(d, s);
    }

+    public static Type ownerType(Class<?> c) {


This feels somewhat odd. How about, moving these back to Executable and rename ownerType to resolveToType ? I realise we can't get use getFactory() in executable but as you write, it isn't needed, it should be fine in this case to create it directly as in your v1. The reason is the semantics of this is somewhat unintuitive and is only used in the annotated receiver case.

I moved the code back and it's fairly compact now. I added "owner" to the method name since it does not make sense otherwise. Let me know what you think!

jbf · 2020-11-16T10:29:48Z

...jdk/java/lang/annotation/typeAnnotations/TestReceiverTypeParameterizedConstructorNested.java

+import java.lang.reflect.AnnotatedType;
+import java.lang.reflect.Constructor;
+
+public class TestReceiverTypeParameterizedConstructorNested {


Suggestion, what I would like to test is that we can walk up the chain and find type parameters on the outermost owner. Same for Method.

I adjusted the test to check for an inner and outer variable for both method and constructor.

jbf

Looks good. I'll run some additional testing on the patch.

openjdk · 2020-11-16T11:24:33Z

@raphw This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8202471: (ann) Cannot read type annotations on generic receiver type's type variables

Reviewed-by: jfranck

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check
4553fa0: 8256258: some missing NULL checks or asserts after CodeCache::find_blob_unsafe
1228517: 8256274: C2: Optimize copying of the shared type dictionary
537b40e: 8252588: HotSpot Style Guide should permit uniform initialization
c35e1a2: 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument
30a2ad5: 8254872: Optimize Rotate on AArch64
eabf3ba: 8256037: [TESTBUG] com/sun/jndi/dns/ConfigTests/PortUnreachable.java fails due to the hard coded threshold is small
36dbe6f: 8256376: The javax/swing/JSpinner/SerializationTest.java fails on headful windows
bf84dac: 8247781: Day periods support
... and 9 more: https://git.openjdk.java.net/jdk/compare/8eeb36f14a9121b6cb1ed3228f78021d5da9e81b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jbf) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

jbf · 2020-11-16T11:28:59Z

I think it would be good to rebase on top of a recent jdk upstream before merging.

…ns on its potential type parameters but is never represented as parameterized type what makes these parameters inaccessible at runtime, despite the presence of parameter type annotations.

raphw · 2020-11-16T11:51:32Z

Rebased on HEAD/master.

jbf · 2020-11-17T10:01:20Z

Tier 1-3 looks good, go ahead and / integrate and I'll sponsor

raphw · 2020-11-17T10:25:04Z

/integrate

openjdk · 2020-11-17T10:25:36Z

@raphw
Your change (at version 91c518c) is now ready to be sponsored by a Committer.

jbf · 2020-11-17T11:23:07Z

/sponsor

openjdk · 2020-11-17T11:24:02Z

@jbf @raphw Since your change was applied there have been 21 commits pushed to the master branch:

adb8561: 8253228: [JVMCI] provide more info on fatal JVMCI errors
a7422ac: 8255150: Add utility methods to check long indexes and ranges
6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check
4553fa0: 8256258: some missing NULL checks or asserts after CodeCache::find_blob_unsafe
1228517: 8256274: C2: Optimize copying of the shared type dictionary
537b40e: 8252588: HotSpot Style Guide should permit uniform initialization
c35e1a2: 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument
30a2ad5: 8254872: Optimize Rotate on AArch64
eabf3ba: 8256037: [TESTBUG] com/sun/jndi/dns/ConfigTests/PortUnreachable.java fails due to the hard coded threshold is small
... and 11 more: https://git.openjdk.java.net/jdk/compare/8eeb36f14a9121b6cb1ed3228f78021d5da9e81b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 53a3188.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

…s type variables Reviewed-by: jfranck

openjdk bot added the rfr Pull request is ready for review label Oct 24, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 24, 2020

raphw force-pushed the 8202471 branch from 864971e to 7721c77 Compare October 25, 2020 08:29

raphw force-pushed the 8202471 branch 3 times, most recently from 49cd2c4 to ac30616 Compare October 31, 2020 21:06

jbf reviewed Nov 15, 2020

View reviewed changes

raphw force-pushed the 8202471 branch from ac30616 to 9e4c203 Compare November 15, 2020 23:06

raphw changed the title ~~8202471: Make type annotations on owner type parameters available~~ 8202471: (ann) Cannot read type annotations on generic receiver type's type variables Nov 15, 2020

raphw force-pushed the 8202471 branch from 9e4c203 to f043f68 Compare November 15, 2020 23:17

jbf reviewed Nov 16, 2020

View reviewed changes

raphw force-pushed the 8202471 branch 2 times, most recently from cc60c45 to 811afcb Compare November 16, 2020 11:06

jbf approved these changes Nov 16, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 16, 2020

8202471: A method's or constructor's owner type might carry annotatio…

91c518c

…ns on its potential type parameters but is never represented as parameterized type what makes these parameters inaccessible at runtime, despite the presence of parameter type annotations.

raphw force-pushed the 8202471 branch from 811afcb to 91c518c Compare November 16, 2020 11:49

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 17, 2020

openjdk bot closed this Nov 17, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 17, 2020

openjdk-notifier bot referenced this pull request Nov 17, 2020

8202471: (ann) Cannot read type annotations on generic receiver type'…

53a3188

…s type variables Reviewed-by: jfranck

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8202471: (ann) Cannot read type annotations on generic receiver type's type variables #851

8202471: (ann) Cannot read type annotations on generic receiver type's type variables #851

raphw commented Oct 24, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 24, 2020

openjdk bot commented Oct 24, 2020

mlbridge bot commented Oct 24, 2020 •

edited

Loading

shipilev commented Oct 27, 2020

raphw commented Nov 1, 2020

jbf left a comment

jbf Nov 15, 2020

raphw Nov 15, 2020

jbf Nov 15, 2020

raphw Nov 15, 2020

jbf Nov 15, 2020

raphw Nov 15, 2020

jbf Nov 15, 2020

raphw Nov 15, 2020

jbf Nov 16, 2020

raphw Nov 16, 2020

jbf Nov 16, 2020

raphw Nov 16, 2020

jbf left a comment

openjdk bot commented Nov 16, 2020 •

edited

Loading

jbf commented Nov 16, 2020

raphw commented Nov 16, 2020

jbf commented Nov 17, 2020

raphw commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

jbf commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

8202471: (ann) Cannot read type annotations on generic receiver type's type variables #851

8202471: (ann) Cannot read type annotations on generic receiver type's type variables #851

Conversation

raphw commented Oct 24, 2020 • edited by openjdk bot Loading

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 24, 2020

openjdk bot commented Oct 24, 2020

mlbridge bot commented Oct 24, 2020 • edited Loading

Webrevs

shipilev commented Oct 27, 2020

raphw commented Nov 1, 2020

jbf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbf left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 16, 2020 • edited Loading

jbf commented Nov 16, 2020

raphw commented Nov 16, 2020

jbf commented Nov 17, 2020

raphw commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

jbf commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

raphw commented Oct 24, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 24, 2020 •

edited

Loading

openjdk bot commented Nov 16, 2020 •

edited

Loading