Re: Synthetic GroovyObject methods

[Daniel Sun]

Reverted:

https://github.com/apache/groovy/commit/8754145385577dff72db48a78c7de2eb027232a1


Many errors occurred after commit 7afe485:
https://travis-ci.org/apache/groovy/jobs/352622463



--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html

Re: Synthetic GroovyObject methods

[Jochen Theodorou]

agreed

Am 12.03.2018 um 13:07 schrieb Paul King:
I think we probably have to. It's an integration point that we haven't 
really seen a lot of call for in the past but with @CompileStatic 
becoming more popular, I can see more use of this style of integration.


Keen to hear if anyone can see an ongoing problem if we remove it. Do we 
know of any other tools which might be relying on that aspect?


Paul.

On Mon, Mar 12, 2018 at 4:38 PM, Daniel.Sun > wrote:


Hi Jochen,

      Should we remove ACC_SYNTHETIC?

Cheers,
Daniel.Sun





--
Sent from:
http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html

Re: Synthetic GroovyObject methods

[Paul King]

I think we probably have to. It's an integration point that we haven't
really seen a lot of call for in the past but with @CompileStatic becoming
more popular, I can see more use of this style of integration.

Keen to hear if anyone can see an ongoing problem if we remove it. Do we
know of any other tools which might be relying on that aspect?

Paul.

On Mon, Mar 12, 2018 at 4:38 PM, Daniel.Sun  wrote:

> Hi Jochen,
>
>  Should we remove ACC_SYNTHETIC?
>
> Cheers,
> Daniel.Sun
>
>
>
>
>
> --
> Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
>

Re: Synthetic GroovyObject methods

[Daniel.Sun]

Hi Jochen,

 Should we remove ACC_SYNTHETIC?

Cheers,
Daniel.Sun





--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html

Re: Synthetic GroovyObject methods

[Daniil Ovchinnikov]

https://issues.apache.org/jira/browse/GROOVY-7362 

https://issues.apache.org/jira/browse/GROOVY-8496 

https://issues.apache.org/jira/browse/GROOVY-8497 


And https://youtrack.jetbrains.com/issue/IDEA-173360 
 which is basically 
GROOVY-7362.

—

Daniil Ovchinnikov
Software Developer
JetBrains
jetbrains.com
“Drive to develop”


> On 10 Mar 2018, at 16:24, Jochen Theodorou  wrote:
> 
> On 10.03.2018 10:36, Daniil Ovchinnikov wrote:
>> Thank you for looking into this.
>>> In an IDE, if you have x. you would then be 
>>> presented with getProperty, setProperty and get/setMetaClass. Do we 
>>> actually want that?
>> These methods are public and non-synthetic in GroovyObject interface so they 
>> will be suggested independently of their implementations.
> 
> good point. So what is actually the downside of them being synthetic right 
> now?
> 
> 
> bye Jochen
> 

Re: Synthetic GroovyObject methods

[Daniil Ovchinnikov]

Thank you for looking into this.

> In an IDE, if you have x. you would then be 
> presented with getProperty, setProperty and get/setMetaClass. Do we actually 
> want that?

These methods are public and non-synthetic in GroovyObject interface so they 
will be suggested independently of their implementations.

—

Daniil Ovchinnikov
Software Developer
JetBrains
jetbrains.com
“Drive to develop”


> On 10 Mar 2018, at 03:48, Paul King  wrote:
> 
> 
> We have recently also started adding @Generated to such methods. Originally 
> this was to assist with better results when doing coverage. One option would 
> be to remove the synthetic now with the expectation that tools could look for 
> the annotation.
> 
> But I understand Jochen's point that this has been there a while so it might 
> be hard to understand all the implications for all language users.
> 
> Cheers, Paul.
> 
> On Sat, Mar 10, 2018 at 9:10 AM, Jochen Theodorou  > wrote:
> On 09.03.2018 17:19, Daniel.Sun wrote:
> Hi Daniil,
> 
>Maybe Jochen can tell us the reason.
> 
>Ping Jochen ;-)
> 
> 
> Checking Verifier I see:
> 
> if (!node.hasMethod("getProperty", GET_PROPERTY_PARAMS)) {
> MethodNode methodNode = addMethod(node, 
> !isAbstract(node.getModifiers()),
> "getProperty",
> ACC_PUBLIC,
> ClassHelper.OBJECT_TYPE,
> GET_PROPERTY_PARAMS,
> ClassNode.EMPTY_ARRAY,
> new BytecodeSequence(new BytecodeInstruction() {
> public void visit(MethodVisitor mv) {
> ...
> }
> })
> );
> if (shouldAnnotate) methodNode.addAnnotation(generatedAnnotation);
> }
> 
> This is having only ACC_PUBLIC as modifier, no ACC_SYNTHETIC.
> 
> Using Groovy 2.4.14 to compile class X{} shows this:
> 
>   // access flags 0x1001
>   public synthetic getProperty(Ljava/lang/String;)Ljava/lang/Object;
> 
> Investigating further shows, addMethod is there as well investigating 
> more into the history points me to
> 
> fix for GROOVY-3877. To ensure abstract classes can always be extended in 
> Java, even if precompiled, all Groovyobject methods must not have the 
> synthetic flag being set. In normal classes this is no problem.
> 
> but that is only to lift the restriction for abstract classes and then it 
> goes to way before... 891ad59d074990a38d7ba0dca65890e80061158a from Jan 29 
> 2004, a change made by James explicitly to add synthetic
> 
> I think the idea was that everything the compiler adds as a helper method is 
> supposed to be synthetic. Most likely back then getProperty and friends have 
> been seen as internal methods, not to be called directly. And from Java code 
> you rarely do. In most cases you go through GroovyObject instead. Now given 
> that this is there for like 14 years I think it is worth spending a minute on 
> the implications.
> 
> In an IDE, if you have x. you would then be 
> presented with getProperty, setProperty and get/setMetaClass. Do we actually 
> want that?
> 
> bye Jochen
> 

Re: Synthetic GroovyObject methods

[Paul King]

We have recently also started adding @Generated to such methods. Originally
this was to assist with better results when doing coverage. One option
would be to remove the synthetic now with the expectation that tools could
look for the annotation.

But I understand Jochen's point that this has been there a while so it
might be hard to understand all the implications for all language users.

Cheers, Paul.

On Sat, Mar 10, 2018 at 9:10 AM, Jochen Theodorou  wrote:

> On 09.03.2018 17:19, Daniel.Sun wrote:
>
>> Hi Daniil,
>>
>>Maybe Jochen can tell us the reason.
>>
>>Ping Jochen ;-)
>>
>
>
> Checking Verifier I see:
>
> if (!node.hasMethod("getProperty", GET_PROPERTY_PARAMS)) {
>> MethodNode methodNode = addMethod(node,
>> !isAbstract(node.getModifiers()),
>> "getProperty",
>> ACC_PUBLIC,
>> ClassHelper.OBJECT_TYPE,
>> GET_PROPERTY_PARAMS,
>> ClassNode.EMPTY_ARRAY,
>> new BytecodeSequence(new BytecodeInstruction() {
>> public void visit(MethodVisitor mv) {
>>
> ...
>
>> }
>> })
>> );
>> if (shouldAnnotate) methodNode.addAnnotation(gener
>> atedAnnotation);
>> }
>>
>
> This is having only ACC_PUBLIC as modifier, no ACC_SYNTHETIC.
>
> Using Groovy 2.4.14 to compile class X{} shows this:
>
>   // access flags 0x1001
>>   public synthetic getProperty(Ljava/lang/String;)Ljava/lang/Object;
>>
>
> Investigating further shows, addMethod is there as well investigating
> more into the history points me to
>
> fix for GROOVY-3877. To ensure abstract classes can always be extended in
>> Java, even if precompiled, all Groovyobject methods must not have the
>> synthetic flag being set. In normal classes this is no problem.
>>
>
> but that is only to lift the restriction for abstract classes and then
> it goes to way before... 891ad59d074990a38d7ba0dca65890e80061158a from
> Jan 29 2004, a change made by James explicitly to add synthetic
>
> I think the idea was that everything the compiler adds as a helper method
> is supposed to be synthetic. Most likely back then getProperty and friends
> have been seen as internal methods, not to be called directly. And from
> Java code you rarely do. In most cases you go through GroovyObject instead.
> Now given that this is there for like 14 years I think it is worth spending
> a minute on the implications.
>
> In an IDE, if you have x. you would then be
> presented with getProperty, setProperty and get/setMetaClass. Do we
> actually want that?
>
> bye Jochen
>

Re: Synthetic GroovyObject methods

[Jochen Theodorou]

On 09.03.2018 17:19, Daniel.Sun wrote:

Hi Daniil,

   Maybe Jochen can tell us the reason.

   Ping Jochen ;-)



Checking Verifier I see:


if (!node.hasMethod("getProperty", GET_PROPERTY_PARAMS)) {
MethodNode methodNode = addMethod(node, 
!isAbstract(node.getModifiers()),
"getProperty",
ACC_PUBLIC,
ClassHelper.OBJECT_TYPE,
GET_PROPERTY_PARAMS,
ClassNode.EMPTY_ARRAY,
new BytecodeSequence(new BytecodeInstruction() {
public void visit(MethodVisitor mv) {

...

}
})
);
if (shouldAnnotate) methodNode.addAnnotation(generatedAnnotation);
}


This is having only ACC_PUBLIC as modifier, no ACC_SYNTHETIC.

Using Groovy 2.4.14 to compile class X{} shows this:


  // access flags 0x1001
  public synthetic getProperty(Ljava/lang/String;)Ljava/lang/Object;


Investigating further shows, addMethod is there as well 
investigating more into the history points me to



fix for GROOVY-3877. To ensure abstract classes can always be extended in Java, 
even if precompiled, all Groovyobject methods must not have the synthetic flag 
being set. In normal classes this is no problem.


but that is only to lift the restriction for abstract classes and 
then it goes to way before... 891ad59d074990a38d7ba0dca65890e80061158a 
from Jan 29 2004, a change made by James explicitly to add synthetic


I think the idea was that everything the compiler adds as a helper 
method is supposed to be synthetic. Most likely back then getProperty 
and friends have been seen as internal methods, not to be called 
directly. And from Java code you rarely do. In most cases you go through 
GroovyObject instead. Now given that this is there for like 14 years I 
think it is worth spending a minute on the implications.


In an IDE, if you have x. you would then be 
presented with getProperty, setProperty and get/setMetaClass. Do we 
actually want that?


bye Jochen

Re: Synthetic GroovyObject methods

[Daniel.Sun]

Hi Daniil,

  Maybe Jochen can tell us the reason. 

  Ping Jochen ;-)

Cheers,
Daniel.Sun



--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html

Re: Synthetic GroovyObject methods

[Daniil Ovchinnikov]

I’ve created https://issues.apache.org/jira/browse/GROOVY-8495 
 to track this.

—

Daniil Ovchinnikov
Software Developer
JetBrains
jetbrains.com
“Drive to develop”



> On 6 Mar 2018, at 18:39, Daniil Ovchinnikov 
>  wrote:
> 
> Is there any reason why implementations of GroovyObject methods which are 
> injected into Groovy classes are marked with ACC_SYNTHETIC? 
> 
> This causes all sorts of confusion:
> 
> class GroovyClass {}
> 
> class JavaClass extends GroovyClass {} // compiles ok
> 
> class JavaClass extends GroovyClass implements GroovyObject {} // error: 
> JavaClass is not abstract and does not override abstract method 
> setMetaClass(MetaClass) in GroovyObject
> 
> class JavaClass extends GroovyClass { 
>@Override
>public Object getProperty(String propertyName) {
>return super.getProperty(propertyName); // error: cannot find symbol: 
> method getProperty(String)
>}
> }
> 
> class JavaClass {
>void usage() {
>new GroovyClass().getProperty("a”); // error: cannot find symbol: 
> method getProperty(String)
>}
> }
> 
> This happens because javac ignores ACC_SYNTHETIC members as if they don’t 
> even exist.
> 
> —
> 
> Daniil Ovchinnikov
> Software Developer
> JetBrains
> jetbrains.com
> “Drive to develop”
> 

Synthetic GroovyObject methods

[Daniil Ovchinnikov]

Is there any reason why implementations of GroovyObject methods which are 
injected into Groovy classes are marked with ACC_SYNTHETIC? 

This causes all sorts of confusion:

class GroovyClass {}

class JavaClass extends GroovyClass {} // compiles ok

class JavaClass extends GroovyClass implements GroovyObject {} // error: 
JavaClass is not abstract and does not override abstract method 
setMetaClass(MetaClass) in GroovyObject

class JavaClass extends GroovyClass { 
@Override
public Object getProperty(String propertyName) {
return super.getProperty(propertyName); // error: cannot find symbol: 
method getProperty(String)
}
}

class JavaClass {
void usage() {
new GroovyClass().getProperty("a”); // error: cannot find symbol: 
method getProperty(String)
}
}

This happens because javac ignores ACC_SYNTHETIC members as if they don’t even 
exist.

—

Daniil Ovchinnikov
Software Developer
JetBrains
jetbrains.com
“Drive to develop”