RFR: 8210943: Hiding of inner classes not resolved properly

Discussion:

Hannes Wallnöfer

2018-11-29 17:01:25 UTC

Please review:

Bug: https://bugs.openjdk.java.net/browse/JDK-8210943
Webrev: http://cr.openjdk.java.net/~hannesw/8210943/webrev.00/

AccessibleMembersLookup#lookupAccessibleMembers adds all nested classes returned by Class.getClasses(), but these may contain inherited classes that are shadowed and thus not visible from the current class. The solution is to make sure we use the first inner class with any given name.

Thanks,
Hannes

Attila Szegedi

2018-11-30 11:14:52 UTC

Permalink

+1. Thanks for fixing this.

Post by Hannes WallnÃ¶fer
Bug: https://bugs.openjdk.java.net/browse/JDK-8210943
Webrev: http://cr.openjdk.java.net/~hannesw/8210943/webrev.00/
AccessibleMembersLookup#lookupAccessibleMembers adds all nested classes returned by Class.getClasses(), but these may contain inherited classes that are shadowed and thus not visible from the current class. The solution is to make sure we use the first inner class with any given name.
Thanks,
Hannes

Sundararajan Athijegannathan

2018-12-01 06:13:20 UTC

Permalink

Class.getClasses() javadoc does not mention anything about order of
classes returned.

https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()

Do we need a check using Class.getDeclaringClass() or do I something here?

Thanks,
-Sundar

Post by Attila Szegedi
+1. Thanks for fixing this.

Sundararajan Athijegannathan

2018-12-01 06:44:51 UTC

Permalink

That should have been "do I miss something here?" :)

-Sundar

Post by Sundararajan Athijegannathan
Class.getClasses() javadoc does not mention anything about order of
classes returned.
https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()
Do we need a check using Class.getDeclaringClass() or do I something here?
Thanks,
-Sundar

Post by Attila Szegedi
+1. Thanks for fixing this.

On 2018. Nov 29., at 18:01, Hannes
Bug: https://bugs.openjdk.java.net/browse/JDK-8210943
Webrev: http://cr.openjdk.java.net/~hannesw/8210943/webrev.00/
AccessibleMembersLookup#lookupAccessibleMembers adds all nested
classes returned by Class.getClasses(), but these may contain
inherited classes that are shadowed and thus not visible from the
current class. The solution is to make sure we use the first inner
class with any given name.
Thanks,
Hannes

Attila Szegedi

2018-12-01 12:05:31 UTC

Permalink

The relevant Dynalink algorithm processes the class before moving on to superclass, so Hannes fix is correct in that we won’t stomp over a subclass’ inner class (inserted into the map earlier) with the superclass’ inner class (encountered later by the algorithm). So yeah, getClasses() doesn’t specify an order, but the Dynalink code has a subclass-towards-superclass traversal order.

Attila.

Class.getClasses() javadoc does not mention anything about order of classes returned.
https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()
Do we need a check using Class.getDeclaringClass() or do I something here?
Thanks,
-Sundar

Post by Attila Szegedi
+1. Thanks for fixing this.

Hannes Wallnöfer

2018-12-01 18:12:32 UTC

Permalink

Attila, the subclass-to-superclass traversal is actually not done in dynalink but directly in java.lang.Class.getClasses(). So Sundar has a point in that my code relies on implementation rather than specified behaviour of Class.getClasses().

Now I do think it is highly unlikely that the order of classes returned by Classes.getClasses() would change in a future release. That would certainly break a lot of other code. Furthermore, if the order was reversed (superclasses to subclasses) that change would be caught by the test. The only change that could go unnoticed would be classes returned in random order, which I don’t think is a real concern. But of course we could choose to be defensive and add code that guards against it.

Hannes

Post by Attila Szegedi
The relevant Dynalink algorithm processes the class before moving on to superclass, so Hannes fix is correct in that we won’t stomp over a subclass’ inner class (inserted into the map earlier) with the superclass’ inner class (encountered later by the algorithm). So yeah, getClasses() doesn’t specify an order, but the Dynalink code has a subclass-towards-superclass traversal order.
Attila.

Class.getClasses() javadoc does not mention anything about order of classes returned.
https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()
Do we need a check using Class.getDeclaringClass() or do I something here?
Thanks,
-Sundar

Post by Attila Szegedi
+1. Thanks for fixing this.

Attila Szegedi

2018-12-01 18:37:37 UTC

Permalink

Huh… so even within the single array returned from a single call to Classes.getClasses() we can have two classes of the same short name? I foolishly presumed this wouldn’t be the case. I guess in this case the full solution would indeed to provide an ordering on the classes, first on the name, then on whether one’s declaring class is a subclass of the other (I guess the number of superclasses is also a good proxy for that.)

Attila.

Post by Hannes WallnÃ¶fer
Attila, the subclass-to-superclass traversal is actually not done in dynalink but directly in java.lang.Class.getClasses(). So Sundar has a point in that my code relies on implementation rather than specified behaviour of Class.getClasses().
Now I do think it is highly unlikely that the order of classes returned by Classes.getClasses() would change in a future release. That would certainly break a lot of other code. Furthermore, if the order was reversed (superclasses to subclasses) that change would be caught by the test. The only change that could go unnoticed would be classes returned in random order, which I don’t think is a real concern. But of course we could choose to be defensive and add code that guards against it.
Hannes

Class.getClasses() javadoc does not mention anything about order of classes returned.
https://docs.oracle.com/javase/10/docs/api/java/lang/Class.html#getClasses()
Do we need a check using Class.getDeclaringClass() or do I something here?
Thanks,
-Sundar

Post by Attila Szegedi
+1. Thanks for fixing this.

Jim Laskey

2018-11-30 13:03:13 UTC

Permalink