NPE when using JSR223 implementation under some conditions

I would like to work on it, but I need one clarification:

NPE can easily be avoided by a check that if GroovyClassLoader.parseClass() returned null, then GroovyScriptEngineImpl can throw javax.script.ScriptException("script class is null") or with a more appropriate message.

My question is whether GroovyClassLoader should fail to generate a class for the script string

import java.lang.*

in the first place. I thought that it would generate a script class even in this case

So, unrelated to JSR223, the underlying issue here is that, the following code does not go through

assert new GroovyClassLoader().parseClass("import java.lang.*")

If this behavior of GroovyClassLoader is considered correct, then the issue reported can just be solved by putting a null check.

But if this behavior of GroovyClassLoader is considered incorrect, then there are 2 issues 2 solve here - one with GroovyClassLoader and other the null check in GroovyScriptEngineImpl (for some additional safety).

Roshan,

Thanks for looking into this. I was also very surprised that the following would not work and I don't understand why:

assert new GroovyClassLoader().parseClass("import java.lang.*")

My take is that this should work just fine but then I'm not a Groovy expert (far from it).

I pinged Guillaume Laforge to see what he says about it but he's currently on holiday I believe.

Thanks

Yes, my guess too would have been that groovy compiler would generate a script class in this case.

I am sure Jochen will soon clarify whether this behavior of GroovyClassLoader is correct or not .

In the meanwhile I am debugging the groovy compiler to understand the current behavior and see at which point, it is deciding to not generate any class in this scenario.

Just for my curiosity - even if groovy starts generating a class here, how will it help you? Trying to understand your usage scenario a bit.

This is for the XWiki groovy macro. The user can put anything in it, including stuff that don't do anything (like just an import for ex). I'd rather that it doesn't do anything instead of generating an error which will be hard to understand for the user.

I discovered the pb when trying to debug a groovy script I had in xwiki and I was commenting out code in order to find where the pb was coming from, till for some reason I had only some import left and I got the pb.

I also have the same problem if there's only a class definition: The following fails;

class MyClass
{
}

But this works:

class MyClass
{
}

def foo

The error in this case is different:

...
Caused by: javax.script.ScriptException: No signature of method: MyClass.main() is applicable for argument types: ([Ljava.lang.String;) values: [[]]
	at org.codehaus.groovy.jsr223.GroovyScriptEngineImpl.eval(GroovyScriptEngineImpl.java:306)
	at org.codehaus.groovy.jsr223.GroovyScriptEngineImpl.eval(GroovyScriptEngineImpl.java:113)
	... 92 more
Caused by: org.codehaus.groovy.runtime.metaclass.MissingMethodExceptionNoStack: No signature of method: MyClass.main() is applicable for argument types: ([Ljava.lang.String;) values: [[]]

Thanks

To understand why

class MyClass {}

fails to run and

class MyClass {}
def foo

goes through, you should try to run them separately with groovy first. The error you will get in 1st case will make it much clearer why it failing.

GroovyScriptEngineImpl().eval() - compiles the script, creates an object of it and tries to run it.
To run the script, it remains consistent with groovy behavior - it invokes the script object's run(), which can do 2 things:

• If script is just a class - as in your 1st case - run method tries to invoke the main() method of the class - this can be seen in your case 1 exception.
• If script is a just a script(some script statements/methods) or (some classes + some script statements/methods) -
  then script statements/methods become part of the script class, which gets the run() method, which gets invoked.
  Any additional class {} result in their own classes but they are not executed by groovy engine.

So, your first case is:

class MyClass {
// no main() here, so groovy engine's run() fails to invoke it.
}

and your 2nd case is:

class MyClass {
}
class Groovy_Generate_Script_Class {
....
def run() { // not groovy engine is fine because it gets this run() on script object.
def foo // this comes from the statements you have outside your class {} blocks in the script you pass
}
....
}

Incidentally, GROOVY-3707 is discussing such things currently. But the discussion there has gone in the loop of how groovy engine "should" behave.

I am trying to cover here what is happening "currently".

Attaching a patch that allows creation of a class even if just import statements / import package statements are provided in the script.

The idea here to to find out whether we want to go ahead and allow the generation of classes in such scenarios or not. If we are going ahead with it, static imports (of classes, fields, methods) can also be added.

I mean, if "import java.lang.*" generates a class, then so should "import static foo.Bar" / "import static foo.Bar.field1" / import static "foo.method1", right?

I think a script with just imports should return null. But I think the patch goes into a wrong direction. I have not looked into it yet, but what if APP would add a "null" as statement if there had been no class and the script body is empty. The advantage of this approach is, that the code in ModuleNode needs no change. und you still get a script. If you don't do that here, other parts may get a problem with this overspecialized rules in ModuleNode.

I can try doing it through APP too - first attempt was to quickly try to fix it where it was deciding not to generate a class with just imports.

So, what is the call? A script with just imports should generate a class or not?

Or, do we need a dev-list discussion on it?

Not sure if it can be done in APP as there it's more like streaming of tokens and our special "null" statement add needs to be done after whole of AST has been formed and still no statements / methods / classes have been found in the passed script. I didn't see anything in APP that would mark the end of processing of all tokens - I could have done it there if it was there.

So, here is a revised patch, that is, hopefully, not too over specialized:

Index: src/main/org/codehaus/groovy/ast/ModuleNode.java
===================================================================
--- src/main/org/codehaus/groovy/ast/ModuleNode.java	(revision 17476)
+++ src/main/org/codehaus/groovy/ast/ModuleNode.java	(working copy)
@@ -21,7 +21,9 @@
+import org.codehaus.groovy.ast.stmt.EmptyStatement;
@@ -73,6 +75,9 @@
     }
 
     public List<ClassNode> getClasses() {
+    	if(statementBlock.isEmpty() && methods.isEmpty() && classes.isEmpty()) {
+    		statementBlock.addStatement(EmptyStatement.INSTANCE);
+    	}
         if (createClassForStatements && (!statementBlock.isEmpty() || !methods.isEmpty() || isPackageInfo())) {
             ClassNode mainClass = createStatementsClass();
             createClassForStatements = false;

what I did mean is that a script

import x.y.*

should be equal to a script

import x.y.*
null

So that means that if no classes are generated and the statementBlock is empty AÜÜ will add the ConstantExpression into the StatementBlock. At last that is my idea. ModuleNode#getClasses will then not need a change, because the statementBlock won't be empty then.

I know what change you asked for in "import java.lang.*" script. Even ModuleNode#getClasses() change was achieving the same thing. First call to get classes from the module would have effected that script change.

Although, now that I have made that very change in APP, I can see that it is better place for it.

Attaching the APP based patch. 2 tests also needed to be fixed, because compilation of "import static foo.Bar.mooky" / "import static foo.Bar.*" now results in a valid script class node and the GCL should be able to resolve foo.Bar to parse this new class node.

good that we agree now I think the patch is ok... maybe the static import tests should be moved to the gls package instead, but that is not urgent.

Fixed.

Thanks a lot Roshan

My pleasure.