The Wayback Machine - https://web.archive.org/web/20220709162130/https://github.com/apache/groovy/pull/502
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GROOVY-8096 setScriptBaseClass with Java class breaks @Field Binding init due to call to super() instead of super(Binding) #502

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kreiger
Copy link

@kreiger kreiger commented Feb 22, 2017

This test fails because ClassNode.getSuperClass().getDeclaredConstructor(SCRIPT_CONTEXT_CTOR) doesn't see the constructors in the Java base class.

ModuleNode.setScriptBaseClassFromConfig(ClassNode)
calls .setSuperClass(ClassHelper.make(baseClassName)) on the scriptDummy ClassNode.

The ClassNode created for this script's base class has .lazyInitDone = true and .constructors = null so when .getSuperClass().getDeclaredConstructor(SCRIPT_CONTEXT_CTOR) is called by ModuleNode.createStatementsClass(), then ClassNode.constructors is set to an empty ArrayList in ClassNode.getDeclaredConstructors()

The script constructor is then generated as

     Constructor(Binding context) {
         super();                   // Fields are initialized after the call to super()
                                    // Fields are initialized here
         setBinding(context);       // Fields are initialized before the call to setBinding(context)
     }

instead of

     Constructor(Binding context) {
         super(context);            // Fields are initialized after the call to super(context)
     }

Fields are initialized between the call to super() and the setBinding(context)
which means Field initializers don't have access to the Binding context.

This leads to MissingPropertyException because we're trying to look up variables from the new Binding() created in the default constructor, instead of the binding we passed in.

…ass with a Java class causes ModuleNode to generate a constructor that prevents Field initialization from Binding context.
@kreiger kreiger changed the title CompilerConfiguration.setScriptBaseClass with Java class calls super() instead of super(Binding) CompilerConfiguration.setScriptBaseClass with Java class breaks @Field Binding init with call to super() instead of super(Binding) Feb 22, 2017
* A Script which requires a Binding passed in the constructor and disallows calling the default constructor.
*/
public abstract class BindingScript extends Script {
protected BindingScript() {
Copy link
Contributor

@daniellansun daniellansun Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making the default constructor private? Developers can get compilation error other than runtime error, then the program will be more robust IMO.

Copy link
Author

@kreiger kreiger Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the exact same super() call is generated, because classNode.getSuperClass().getDeclaredConstructor(SCRIPT_CONTEXT_CTOR) incorrectly returns null in ModuleNode.createStatementsClass() even though that constructor exists.

If i make the default constructor private i get IllegalAccessError:

java.lang.reflect.InvocationTargetException
    at TestBindingsInFieldInitializersWithConfigJavaBaseScript.main(TestBindingsInFieldInitializersWithConfigJavaBaseScript.groovy)
Caused by: java.lang.IllegalAccessError: tried to access method groovy.lang.BindingScript.<init>()V from class TestBindingsInFieldInitializersWithConfigJavaBaseScript
    at TestBindingsInFieldInitializersWithConfigJavaBaseScript.<init>(TestBindingsInFieldInitializersWithConfigJavaBaseScript.groovy)

If i remove the default constructor i get NoSuchMethodError:

java.lang.reflect.InvocationTargetException
    at TestBindingsInFieldInitializersWithConfigJavaBaseScript.main(TestBindingsInFieldInitializersWithConfigJavaBaseScript.groovy)
Caused by: java.lang.NoSuchMethodError: groovy.lang.BindingScript: method <init>()V not found
    at TestBindingsInFieldInitializersWithConfigJavaBaseScript.<init>(TestBindingsInFieldInitializersWithConfigJavaBaseScript.groovy)
Copy link
Author

@kreiger kreiger Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i remove both constructors the super() call just goes to the default constructor in groovy.lang.Script, giving MissingPropertyException on trying to access the wrong Binding context on field initalization:

groovy.lang.MissingPropertyException: No such property: args for class: TestBindingsInFieldInitializersWithConfigJavaBaseScript
    at TestBindingsInFieldInitializersWithConfigJavaBaseScript.<init>(TestBindingsInFieldInitializersWithConfigJavaBaseScript.groovy:2)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at groovy.lang.GroovyShell.runScriptOrMainOrTestOrRunnable(GroovyShell.java:271)
@kreiger kreiger changed the title CompilerConfiguration.setScriptBaseClass with Java class breaks @Field Binding init with call to super() instead of super(Binding) GROOVY-8096 setScriptBaseClass with Java class breaks @Field Binding init due to call to super() instead of super(Binding) Feb 22, 2017
@kreiger
Copy link
Author

@kreiger kreiger commented Feb 22, 2017

@paulk-asert
Copy link
Contributor

@paulk-asert paulk-asert commented Feb 28, 2017

Just to clarify - this isn't a PR designed to fix a problem (although BindingScript might prove useful in a solution) but to illustrate a problem. Correct?

@kreiger
Copy link
Author

@kreiger kreiger commented Mar 3, 2017

@paulk-asert Correct, this just adds a couple of unit tests to illustrate the problem. I hope you agree that it's better than just describing it. :)

I was able to make a patch myself where i use Class.forName() to look up the class, but i highly doubt it is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants