Mailing-List: contact dev-help@groovy.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@groovy.apache.org
Subject: Re: Is FieldsAndPropertiesStaticCompileTest#testUseGetterFieldAccess
 really correct?
To: dev@groovy.apache.org
References: <ffa98e67-2f96-63ac-2f77-debf78677081@gmx.org>
 <e8611ae1-d7fd-275b-5f17-89b095fae925@tiscali.it>
From: Jochen Theodorou <blackdrag@gmx.org>
Message-ID: <510cf0eb-8d97-d3f4-d46f-e4771f570656@gmx.org>
Date: Fri, 24 Nov 2017 10:16:33 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <e8611ae1-d7fd-275b-5f17-89b095fae925@tiscali.it>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
archived-at: Fri, 24 Nov 2017 09:16:39 -0000


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