On 20.10.2016 16:14, Alan Bateman wrote:
On 20/10/2016 14:48, Jochen Theodorou wrote:
:
I still wish you had at least not included protected members. Sure,
protected is not public and protected is always a pain in the butt, I
still fail to comprehend the logic that a class can extend a class
from another module and then use the protected methods of the super
class, but setAccessible is not allowed to be used to gain access to
it from anywhere but that same class.
This might appear to be an anomaly but remember you don't have the
receiver at this point. You could have setAccessible silently fail so
that the access check happens at newInstance/invoke/get/put but I don't
see anyone being happy with that. That said, if you are using core
reflection to access a protected member in the same package or super
type then you don't need setAccessible in the first place as the access
check should succeed (there have been issues with protected members that
Peter has recently fixed in jdk9/dev, I don't know if you've run into
that).
I wrote "same class" and thought same class and subclasses... my bad.
Anyway, I am aware of how it works, the problem is that shi... caller
sensitivity. If you work with classic reflection and have a central
space for these kind of actions (for example because that had not really
been caller sensitive before) you get into trouble now.
And another example...
Module A:
package a
class X {
protected void foo(){}
}
unnamed Module:
package u
class Y extends X{
public void bar(){ foo() }
}
How am I supposed to realize the call to foo using a runtime based on
classic reflection? I have only one chance, and that is to have the
actual Method.invokeMethod call at the callsite of foo(). If I do not do
that, I have other frames in between and then @CallerSensitive will see
the "wrong" intermediate frames and assume the call is not made from a
class that actually has access. So I have about 5 frames of my runtime
that will be in the way to realize this call.
Even with MethodHandle I can fall into this trap, if frames of my
runtime get in the way. For example to realize dynamic typing I will not
be able to do the actual method selection from bootstrap. I will do it
by calling a method from my runtime, which then sets and calls the
target. Subsequent calls will be without a frame of my runtime, but
since the first call will fail, this does not matter. Also I would have
to investigate but I think if you do a exception catching handle you may
still get a frame of yours in. The only reason we can work around this
with handles is, because Y will give a "I sell my soul to you" lookup
object to the runtime.
And if you thought that is the worst, nope... Mix Closure in there and
it gets a lot worse:
unnamed Module:
package u
class Y extends X{
public void bar(){ def cl = {foo()}; cl() }
}
Now there will be 5 frames to Closure#call, one to doCall and another 7
to the actual foo. And even though the exact class sunclassing Closure
that is used to realize the call will be an inner class of Y, that does
not mean I will automatically have a bridge method available. Such a
method was simply not required before. And believe me, that is still not
the worst. I have not started on Groovy builders yet....
To say it blunt: #AwkwardStrongEncapsulation prevents classic Groovy
from calling protected methods properly... even if we did the java rules
for protected.
It means there will be older Groovy programs, that will simply no longer
run even with an updated version of the Groovy runtime. And I am not
sure if we even *can* fix this. #AwkwardStrongEncapsulation is simply
not including the concept of a runtime doing calls to protected methods.
And just to be clear: trySetAccessible wouldn't help here either.
could be Groovy 2.4.7, that kind of error is fixed in 2.4.8... once
it is released, which we are currently working on. Because 2.4.7
assumes either all of the class can be made accessible or none. And
that is not true with #AwkwardStrongEncapsulation anymore. Which also
means meta class creation will take a lot longer, since we now cannot
use the array-taking setAccessible method in all cases anymore. Would
have been nice to have a version of setAccessible that just makes
accessible what is allowed and maybe reports back for those that are
not allowed.
A trySetAccessible is possible although a @CS variant of accessCheck
(like in MH.Lookup) might be more general. I think we need to be
cautious about adding APIs here and so would be interesting to get some
performance data here (I don't think I've seen the bulk setAccessible
used very often, it's almost always the instance method).
well, we changed it exactly because the bulk variant is faster. It might
be not much difference. Bunt if you have to create a few thousand meta
classes, it makes a difference. And as stated above, a @CS variant will
get us nowhere. I don't know why all the JDK people seem to think that
@CS saves the world...
bye Jochen
