You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@groovy.apache.org by Jochen Theodorou <bl...@gmx.org> on 2017/11/24 00:46:01 UTC
Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Hi all,
the test is defined as this:
> void testUseGetterFieldAccess() {
> assertScript '''
> class A {
> boolean getterCalled = false
>
> protected int x
> public int getX() {
> getterCalled = true
> x
> }
> }
> class B extends A {
> void usingGetter() {
> this.x
> }
> }
> B b = new B()
> b.usingGetter()
> assert b.isGetterCalled() == true
> '''
> assert astTrees['B'][1].contains('INVOKEVIRTUAL B.getX')
> }
The test was written as a reaction to GROOVY-5619, which is to ensure
that we do not call a setter in this case... only there is no setter
here. Instead this test fixates the behaviour of the static compiler to
call a getter instead of directly accessing the field. Direct access to
the field is what is required for GROOVY-8385
In my opinion the test is wrong, but I'd like to hear others about this.
And another point. We seem to have no similar test for dynamic Groovy.
Groovy does use direct field access if the field is available on "this".
But the question here is if that extends to all super classes. In my
opinion it should.
If I get no vetos I will push a fix for this for all current groovy versions
bye Jochen
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by Jochen Theodorou <bl...@gmx.org>.
Am 24.11.2017 um 13:09 schrieb Mauro Molinari:
> I think this is a quite "grey" area of Groovy, at least it took a while
> for me to understand getter/setter vs direct field access in different
> cases when I faced it and some fixes to the Groovy plugin for Eclipse
> were also necessary to properly support code highlighting and navigation.
> Now you're going further in the analysis by adding inheritance and
> member visibility to the picture. IMHO Groovy should define a coherent
> and consistent policy for this and proper documentation should be provided.
agreed, right now that is only halfway specified
> Just one question: when you talk about "this.x and super.x", you also
> mean the case in which "this." is implicit, don't you?
I mean the explicit this. The implicit this is the same except for
closure bodies and possibly inner classes
> In any case, IMHO the @CompileStatic case should behave exactly like
> @CompileDynamic, absolutely!
Here we always have to consider how Java behaves in in how far the
dynamic case prevents a fully static case.
bye Jochen
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by Mauro Molinari <ma...@tiscali.it>.
I think this is a quite "grey" area of Groovy, at least it took a while
for me to understand getter/setter vs direct field access in different
cases when I faced it and some fixes to the Groovy plugin for Eclipse
were also necessary to properly support code highlighting and navigation.
Now you're going further in the analysis by adding inheritance and
member visibility to the picture. IMHO Groovy should define a coherent
and consistent policy for this and proper documentation should be provided.
Just one question: when you talk about "this.x and super.x", you also
mean the case in which "this." is implicit, don't you?
In any case, IMHO the @CompileStatic case should behave exactly like
@CompileDynamic, absolutely!
Mauro
Il 24/11/2017 10:16, Jochen Theodorou ha scritto:
>
>
> Am 24.11.2017 um 09:09 schrieb Mauro Molinari:
>> Il 24/11/2017 01:46, Jochen Theodorou ha scritto:
>>> In my opinion the test is wrong, but I'd like to hear others about
>>> this.
>>>
>>> And another point. We seem to have no similar test for dynamic
>>> Groovy. Groovy does use direct field access if the field is
>>> available on "this". But the question here is if that extends to all
>>> super classes. In my opinion it should.
>>>
>>> If I get no vetos I will push a fix for this for all current groovy
>>> versions
>>
>> I don't know if I understood it right, but are you saying that you
>> think that invoking b.usingGetter() in your example should not call
>> A.getX(), but rather access A.x directly? And that, I guess, the same
>> policy should apply for the corresponding setter?
>
> yes, just that we do not have a test for this.
>
>> I guess you think so because A.x is declared as protected, don't you?
>> Because if A.x had no modifier, I think that such a change may break
>> a lot of code that assumes getter/setter are called, in particular
>> I'm thinking of the write access case for @Bindable fields
> I am thinking so because in
>
>> class X {
>> def x
>> def foo(){x}
>> }
>
> we will access the field directly. It will access the field directly,
> because the field is accessible. In
>
>> class X {
>> def x
>> }
>> class Y extends X {
>> def foo(){x}
>> }
>
> the field itself is not accessible and the getter/setter must be used.
> But in
>
>> class X {
>> public x
>> def getX(){x}
>> }
>> class Y extends X {
>> def foo(){x}
>> }
>
> the field is accessible, thus I think the access should be done directly.
>
> And then there is the following to consider as well:
>
>> class X {
>> public x
>> def getX(){x}
>> }
>> class Y extends X {
>> def foo(){super.x}
>> }
>
> This will access the field directly in todays implementation. And this
> here
>
>> class X { public x=1
>> def getX(){x+10}
>> }
>> class Y extends X {}
>> class Z extends Y {
>> def foo(){super.x}
>> }
>
> will not access the field directly in todays implementation. And if
> the field is private or does not exist even super.x would always use
> the getter, as would this.x
>
> I think the rules should be the following:
>
> this.x and super.x should access the field if the field is accessible.
> If the field is not accessible, then x has to be accessed as property
> (which would prefer the getter/setter over the field)
>
> Accessible here means for me first and for all, if the field is
> defined in the same class, then for this.x the field is always
> accessible, regardless of any modifier. If the field is from a super
> class, the field must be public or protected. Inaccessible fields are
> to be ignored. Example:
>
>> class X { public x
>> def getX(){x}
>> }
>> class Y extends X {
>> private x
>> }
>> class Z extends Y {
>> def foo(){super.x}
>> }
>
> should also access the field in X, same for this.x instead of super.x
>
>
> The current implementation feels inconsistent to me. Will this break
> code? that may very well be. But I think the chance is not that big.
>
> And then finally there is the question of if we do not want this, do
> we want it different for @CompileStatic?
>
> bye Jochen
>
>
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by Jochen Theodorou <bl...@gmx.org>.
Am 24.11.2017 um 09:09 schrieb Mauro Molinari:
> Il 24/11/2017 01:46, Jochen Theodorou ha scritto:
>> In my opinion the test is wrong, but I'd like to hear others about this.
>>
>> And another point. We seem to have no similar test for dynamic Groovy.
>> Groovy does use direct field access if the field is available on
>> "this". But the question here is if that extends to all super classes.
>> In my opinion it should.
>>
>> If I get no vetos I will push a fix for this for all current groovy
>> versions
>
> I don't know if I understood it right, but are you saying that you think
> that invoking b.usingGetter() in your example should not call A.getX(),
> but rather access A.x directly? And that, I guess, the same policy
> should apply for the corresponding setter?
yes, just that we do not have a test for this.
> I guess you think so because A.x is declared as protected, don't you?
> Because if A.x had no modifier, I think that such a change may break a
> lot of code that assumes getter/setter are called, in particular I'm
> thinking of the write access case for @Bindable fields
I am thinking so because in
> class X {
> def x
> def foo(){x}
> }
we will access the field directly. It will access the field directly,
because the field is accessible. In
> class X {
> def x
> }
> class Y extends X {
> def foo(){x}
> }
the field itself is not accessible and the getter/setter must be used.
But in
> class X {
> public x
> def getX(){x}
> }
> class Y extends X {
> def foo(){x}
> }
the field is accessible, thus I think the access should be done directly.
And then there is the following to consider as well:
> class X {
> public x
> def getX(){x}
> }
> class Y extends X {
> def foo(){super.x}
> }
This will access the field directly in todays implementation. And this here
> class X {
> public x=1
> def getX(){x+10}
> }
> class Y extends X {}
> class Z extends Y {
> def foo(){super.x}
> }
will not access the field directly in todays implementation. And if the
field is private or does not exist even super.x would always use the
getter, as would this.x
I think the rules should be the following:
this.x and super.x should access the field if the field is accessible.
If the field is not accessible, then x has to be accessed as property
(which would prefer the getter/setter over the field)
Accessible here means for me first and for all, if the field is defined
in the same class, then for this.x the field is always accessible,
regardless of any modifier. If the field is from a super class, the
field must be public or protected. Inaccessible fields are to be
ignored. Example:
> class X {
> public x
> def getX(){x}
> }
> class Y extends X {
> private x
> }
> class Z extends Y {
> def foo(){super.x}
> }
should also access the field in X, same for this.x instead of super.x
The current implementation feels inconsistent to me. Will this break
code? that may very well be. But I think the chance is not that big.
And then finally there is the question of if we do not want this, do we
want it different for @CompileStatic?
bye Jochen
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by Mauro Molinari <ma...@tiscali.it>.
Il 24/11/2017 01:46, Jochen Theodorou ha scritto:
> In my opinion the test is wrong, but I'd like to hear others about this.
>
> And another point. We seem to have no similar test for dynamic Groovy.
> Groovy does use direct field access if the field is available on
> "this". But the question here is if that extends to all super classes.
> In my opinion it should.
>
> If I get no vetos I will push a fix for this for all current groovy
> versions
I don't know if I understood it right, but are you saying that you think
that invoking b.usingGetter() in your example should not call A.getX(),
but rather access A.x directly? And that, I guess, the same policy
should apply for the corresponding setter?
I guess you think so because A.x is declared as protected, don't you?
Because if A.x had no modifier, I think that such a change may break a
lot of code that assumes getter/setter are called, in particular I'm
thinking of the write access case for @Bindable fields.
Mauro
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by MG <mg...@arscreat.com>.
Hi Jochen,
thanks for the quick reply. Using direct field access (y.@x) leads to
the same problem (see Jira).
Bye,
mg
On 09.12.2017 21:23, Jochen Theodorou wrote:
> On 09.12.2017 17:45, MG wrote:
>> Hi Jochen,
>>
>> did you ever get around to pushing the fix you mentioned ?
>> I just checked if the current GROOVY_2_4_X snapshot works for
>> Minecraft modding, but alas it still throws:
>> https://issues.apache.org/jira/browse/GROOVY-8385?focusedCommentId=16284807&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16284807
>
>
> as a work around you should try out this.&x and super.&x for accessing
> fields directly. I have not found the time to complete that fix yet, no
>
> bye Jochen
>
>
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by Jochen Theodorou <bl...@gmx.org>.
On 09.12.2017 17:45, MG wrote:
> Hi Jochen,
>
> did you ever get around to pushing the fix you mentioned ?
> I just checked if the current GROOVY_2_4_X snapshot works for Minecraft
> modding, but alas it still throws:
> https://issues.apache.org/jira/browse/GROOVY-8385?focusedCommentId=16284807&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16284807
as a work around you should try out this.&x and super.&x for accessing
fields directly. I have not found the time to complete that fix yet, no
bye Jochen
Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
really correct?
Posted by MG <mg...@arscreat.com>.
Hi Jochen,
did you ever get around to pushing the fix you mentioned ?
I just checked if the current GROOVY_2_4_X snapshot works for Minecraft
modding, but alas it still throws:
https://issues.apache.org/jira/browse/GROOVY-8385?focusedCommentId=16284807&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16284807
Cheers,
mg
On 24.11.2017 01:46, Jochen Theodorou wrote:
> Hi all,
>
> the test is defined as this:
>
>> void testUseGetterFieldAccess() {
>> assertScript '''
>> class A {
>> boolean getterCalled = false
>>
>> protected int x
>> public int getX() {
>> getterCalled = true
>> x
>> }
>> }
>> class B extends A {
>> void usingGetter() {
>> this.x
>> }
>> }
>> B b = new B()
>> b.usingGetter()
>> assert b.isGetterCalled() == true
>> '''
>> assert astTrees['B'][1].contains('INVOKEVIRTUAL B.getX')
>> }
>
> The test was written as a reaction to GROOVY-5619, which is to ensure
> that we do not call a setter in this case... only there is no setter
> here. Instead this test fixates the behaviour of the static compiler
> to call a getter instead of directly accessing the field. Direct
> access to the field is what is required for GROOVY-8385
>
> In my opinion the test is wrong, but I'd like to hear others about this.
>
> And another point. We seem to have no similar test for dynamic Groovy.
> Groovy does use direct field access if the field is available on
> "this". But the question here is if that extends to all super classes.
> In my opinion it should.
>
> If I get no vetos I will push a fix for this for all current groovy
> versions
>
> bye Jochen
>