Discussion:
RFR: 8210943: Hiding of inner classes not resolved properly
(too old to reply)
Hannes Wallnöfer
2018-11-29 17:01:25 UTC
Permalink
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.
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: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.
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
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.
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
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
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.
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
Jim Laskey
2018-11-30 13:03:13 UTC
Permalink
+1
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
Loading...