Problem with latest 2.0-beta-3 snapshot and Spock

[Peter Niederwieser]

The latest snapshot (Feb 17th) causes lots of failing Spock tests. It's always the same exception and always occurs when a closure containing an assert statement gets called:

groovy.lang.MissingPropertyException: No such property: owner for class: org.spockframework.smoke.condition.EqualityComparisonRendering
        at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:50)
        at org.codehaus.groovy.runtime.callsite.PogoGetPropertySite.getProperty(PogoGetPropertySite.java:49)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callGroovyObjectGetProperty(AbstractCallSite.java:231)
        at org.spockframework.smoke.condition.EqualityComparisonRendering$_$spock_feature_1_0_closure1.doCall(EqualityComparisonRendering.groovy:29)
...

This appears to be a bad interaction with Spock's AST transform which rewrites all assert statements that it can find. Any ideas?

Cheers,
Peter

[Peter Niederwieser]

More information: The problem is related to inserting direct method calls. Spock replaces AssertStatement's with power assert-like expressions which contain several new method calls. Some of these method calls set MethodCallExpression.setMethodTarget(). When I prevent that from happening (i.e. don't generate _direct_ method calls), everything is back to normal. Note that the problem only exists for direct method calls made within closures. Any ideas?

Cheers,
Peter

[Jochen Theodorou]

Am 19.02.2012 06:01, schrieb Peter Niederwieser:
> More information: The problem is related to inserting direct method calls.
> Spock replaces AssertStatement's with power assert-like expressions which
> contain several new method calls. Some of these method calls set
> MethodCallExpression.setMethodTarget(). When I prevent that from happening
> (i.e. don't generate _direct_ method calls), everything is back to normal.
> Note that the problem only exists for direct method calls made within
> closures. Any ideas?

and this happens not with beta2?

bye blackdrag

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Peter Niederwieser]

Jochen Theodorou wrote

and this happens not with beta2?

It definitely doesn't happen with beta-2. The last Spock CI build that downloaded a new beta-3-SNAPSHOT and completed successfully was on Feb 16th. So the change must have happened after that.

Cheers,
Peter

[Peter Niederwieser]

Peter Niederwieser wrote

The last Spock CI build that downloaded a new beta-3-SNAPSHOT and completed successfully was on Feb 16th. So the change must have happened after that.

To be more precise, it must have happened _around_ Feb 16th/17th, because I can't exactly tell when the good/bad Groovy snapshots were published. (If Groovy published unique snapshots, it would be easier to pinpoint such problems.)

Cheers,
Peter

[Cédric Champeau]

Hi!

Peter, last friday I merged some changes which may have caused this. Do
you have any small reproducible example which worked under beta 2 and
now fails ? It would greatly help debugging.

Thanks!

Le 19/02/2012 23:42, Peter Niederwieser a écrit :

> Peter Niederwieser wrote
>> The last Spock CI build that downloaded a new beta-3-SNAPSHOT and
>> completed successfully was on Feb 16th. So the change must have happened
>> after that.
>>
> To be more precise, it must have happened _around_ Feb 16th/17th, because I
> can't exactly tell when the good/bad Groovy snapshots were published. (If
> Groovy published unique snapshots, it would be easier to pinpoint such
> problems.)
>
> Cheers,
> Peter
>
> --
> View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5497719.html
> Sent from the groovy - user mailing list archive at Nabble.com.
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>      http://xircles.codehaus.org/manage_email
>
>

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Peter Niederwieser]

Cédric Champeau wrote

Peter, last friday I merged some changes which may have caused this. Do
you have any small reproducible example which worked under beta 2 and
now fails ? It would greatly help debugging.

What I have is a Spock snapshot and a simple spec that consistently fails. Would that help?

Cheers,
Peter

[Cédric Champeau]

This is not as small as I would like it to be, but that will do :) If
you can reduce the specification to the minimal thing that makes it
fail, it would be perfect.

Thanks!

Le 20/02/2012 23:34, Peter Niederwieser a écrit :

> Cédric Champeau wrote
>> Peter, last friday I merged some changes which may have caused this. Do
>> you have any small reproducible example which worked under beta 2 and
>> now fails ? It would greatly help debugging.
>>
> What I have is a Spock snapshot and a simple spec that consistently fails.
> Would that help?
>
> Cheers,
> Peter
>
> --
> View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5500485.html
> Sent from the groovy - user mailing list archive at Nabble.com.
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>      http://xircles.codehaus.org/manage_email
>
>
>

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Cédric Champeau]

btw, I tried building spock from sources, but it seems that there's no
groovy-2.0 branch in the svn repo.

Le 20/02/2012 23:34, Peter Niederwieser a écrit :

> Cédric Champeau wrote
>> Peter, last friday I merged some changes which may have caused this. Do
>> you have any small reproducible example which worked under beta 2 and
>> now fails ? It would greatly help debugging.
>>
> What I have is a Spock snapshot and a simple spec that consistently fails.
> Would that help?
>
> Cheers,
> Peter
>
> --
> View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5500485.html
> Sent from the groovy - user mailing list archive at Nabble.com.
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>      http://xircles.codehaus.org/manage_email
>
>
>

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Peter Niederwieser]

Cédric,

you want https://github.spockframework.org, and the branch is 'groovy-1.9'. In order to reproduce, uncomment the line in org.spockframework.compiler.AstUtil.createDirectMethodCall(), build with `./gradlew test`, and observe the many failing tests. The simplest spec that fails is:

// place this under spock-specs/src/test/groovy
class MySpec extends spock.lang.Specification {
  def foo() {
    setup:
    // org.spockframework.compiler.ConditionRewriter will insert some direct methods calls within the closure
    def cl = { assert true }
    cl()
  }
}

You can generate a working IDEA/Eclipse project with `./gradlew idea` or `./gradlew eclipse`. It's important to rebuild the whole project after making the change to AstUtil.

Let me know if I can help in any other way.

Cheers,
Peter

[Peter Niederwieser]

Correction: It's http://github.spockframework.org

[Peter Niederwieser]

Another try: http://github.spockframework.org/spock

[Cédric Champeau]

Thanks!

Le 21/02/2012 15:26, Peter Niederwieser a écrit :

> Cédric,
>
> you want https://github.spockframework.org, and the branch is 'groovy-1.9'.
> In order to reproduce, uncomment the line in
> org.spockframework.compiler.AstUtil.createDirectMethodCall(), build with
> `./gradlew test`, and observe the many failing tests. The simplest spec that
> fails is:
>
> // place this under spock-specs/src/test/groovy
> class MySpec extends spock.lang.Specification {
>    def foo() {
>      setup:
>      // org.spockframework.compiler.ConditionRewriter will insert some direct
> methods calls within the closure
>      def cl = { assert true }
>      cl()
>    }
> }
>
> You can generate a working IDEA/Eclipse project with `./gradlew idea` or
> `./gradlew eclipse`. It's important to rebuild the whole project after
> making the change to AstUtil.
>
> Let me know if I can help in any other way.
>
> Cheers,
> Peter
>
> --
> View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5502332.html
> Sent from the groovy - user mailing list archive at Nabble.com.
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>      http://xircles.codehaus.org/manage_email
>
>
>

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Cédric Champeau]

Hi Peter!

The good news is that I can reproduce the problem. The bad one is that I cannot outside of Spock :( Basically, the code in Groovy core which is causing problem in your case is this one:

ClassNode declaringClass = target.getDeclaringClass();
ClassNode classNode = controller.getClassNode();
if (implicitThis
    && !classNode.isDerivedFrom(declaringClass)
    && !classNode.implementsInterface(declaringClass)
    && classNode instanceof InnerClassNode) {
  // we are calling an outer class method
  compileStack.pushImplicitThis(false);
  if (controller.isInClosure()) {
    Expression expr = new PropertyExpression(
                                VariableExpression.THIS_EXPRESSION, "owner"
                        );
    expr.visit(controller.getAcg());
  } else {
    Expression expr = new PropertyExpression(new ClassExpression(declaringClass), "this");
    expr.visit(controller.getAcg());
  }
} else {
  compileStack.pushImplicitThis(implicitThis);
  receiver.visit(controller.getAcg());
}

This says, to be short, that if we are using a direct method call with an implicit this and that the declaring class of the target method is not the class the ASM generator is currently visiting, nor a method from a superclass, then we must check if we are inside a closure or if we are calling an outer class method. In this case, we are in a closure, so we're loading the receiver using "this.owner". If I replace the PropertyExpression with a VariableExpression ("owner"), then spock fails with:

 Cannot cast object 'MySpec@49dc423f' with class 'MySpec' to class 'org.spockframework.runtime.ValueRecorder'

However, using an assert in classic groovy code like this one, it doesn't fail:

void foo() {
   def cl = { assert true }
   cl()
}
foo()

Can you give me a hint of what "generated code" with spock from your example would look like? Last, commenting on this issue makes me think the algorithm to load the receiver in that case is not perfect, because we could also have a closure in which we call a method from an outer owner class, or a method from another receiver (delegate, ...). Direct method call support is still incomplete...


Le 21/02/2012 15:29, Peter Niederwieser a écrit :

Another try: http://github.spockframework.org/spock

--
View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5502344.html
Sent from the groovy - user mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Peter Niederwieser]

Cédric,

the generated code (which replaces the assert statement) is along the lines of:

def $spock_valueRecorder = new org.spockframework.runtime.ValueRecorder()
// all the following method invocations are direct (one static and two instance method invocations)
org.spockframework.runtime.SpockRuntime.verify($spock_valueRecorder.reset(), $spock_valueRecorder.record(true))

A breakpoint in org.spockframework.compiler.ConditionRewriter.rewriteCondition(Statement, Expression, Expression, boolean) should turn up the exact generated code (without the variable declaration).

Hope this helps.

Cheers,
Peter

[Cédric Champeau]

Ok, the problem is fixed by adding, in ASTUtil,
result.setImplicitThis(false); after setting the target method. The
"implicit this" thing is complex to me, as it is not very obvious when
it should be set to false or not. It defaults to true, even if you
specify a receiver, which doesn't seem natural. Maybe Jochen can explain
to us when it should be used or not, because it's quite obvious that
making errors on that precise point is easy.

Le 21/02/2012 17:53, Peter Niederwieser a écrit :

> Cédric,
>
> the generated code (which replaces the assert statement) is along the lines
> of:
>
> def $spock_valueRecorder = new org.spockframework.runtime.ValueRecorder()
> // all the following method invocations are direct (one static and two
> instance method invocations)
> org.spockframework.runtime.SpockRuntime.verify($spock_valueRecorder.reset(),
> $spock_valueRecorder.record(true))
>
> A breakpoint in
> org.spockframework.compiler.ConditionRewriter.rewriteCondition(Statement,
> Expression, Expression, boolean) should turn up the exact generated code
> (without the variable declaration).
>
> Hope this helps.
>
> Cheers,
> Peter
>
> --
> View this message in context: http://groovy.329449.n5.nabble.com/Problem-with-latest-2-0-beta-3-snapshot-and-Spock-tp5496353p5502764.html
> Sent from the groovy - user mailing list archive at Nabble.com.
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>      http://xircles.codehaus.org/manage_email
>
>
>

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jochen Theodorou]

Am 22.02.2012 09:45, schrieb Cédric Champeau:
> Ok, the problem is fixed by adding, in ASTUtil,
> result.setImplicitThis(false); after setting the target method. The
> "implicit this" thing is complex to me, as it is not very obvious when
> it should be set to false or not. It defaults to true, even if you
> specify a receiver, which doesn't seem natural. Maybe Jochen can explain
> to us when it should be used or not, because it's quite obvious that
> making errors on that precise point is easy.

in this Closure

def cl = { this.foo(); bar() }

you have a call foo() and a call bar(). he call bar() has an implicit
this and will take part in the MOP of the Closure. In this.foo() we make
a call on the surrounding class, thus this will bypass the MOP of the
Closure and will be only part in the MOP of the surrounding class. If
the object part of a MethodCallExpression is not "this", then the use of
implicitThis undefined. There might be code that assumes if implicitThis
is false, that the objectExpression contains a "this" though, but I
would say that is wrong.

bye blackdrag

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Cédric Champeau]

Le 22/02/2012 11:02, Jochen Theodorou a écrit :

> Am 22.02.2012 09:45, schrieb Cédric Champeau:
>> Ok, the problem is fixed by adding, in ASTUtil,
>> result.setImplicitThis(false); after setting the target method. The
>> "implicit this" thing is complex to me, as it is not very obvious when
>> it should be set to false or not. It defaults to true, even if you
>> specify a receiver, which doesn't seem natural. Maybe Jochen can explain
>> to us when it should be used or not, because it's quite obvious that
>> making errors on that precise point is easy.
>
> in this Closure
>
> def cl = { this.foo(); bar() }
>
> you have a call foo() and a call bar(). he call bar() has an implicit
> this and will take part in the MOP of the Closure.

Ok, so, say we create a MethodCallExpression like this:

MCE call = new MCE(receiver, "message", args)

Then, the constructor assumes that whatever "receiver" is, be it "this"
or not, implicitThis is set to true. From what I understand, we should
only set implicitThis to true iff receiver==null.

> In this.foo() we make a call on the surrounding class, thus this will
> bypass the MOP of the Closure and will be only part in the MOP of the
> surrounding class.
Don't we use "thisObject" in that case ? Normally, owner=thisObject, so
this.bar() should be equivalent to bar().
> If the object part of a MethodCallExpression is not "this", then the
> use of implicitThis undefined.
Which is still unclear. For "this.foo()", we have receiver == "this" &&
implicitThis=false. For "foo()", we have receiver==null? &&
implicitThis=true ?
> There might be code that assumes if implicitThis is false, that the
> objectExpression contains a "this" though, but I would say that is wrong.
If implicitThis is false, we cannot make any assumption, correct? We can
still have "this" as a receiver.
>
> bye blackdrag
>


--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jochen Theodorou]

Am 22.02.2012 13:50, schrieb Cédric Champeau:
[...]
> Then, the constructor assumes that whatever "receiver" is, be it "this"
> or not, implicitThis is set to true. From what I understand, we should
> only set implicitThis to true iff receiver==null.

The current logic assumes there is no null receiver at many places.

bye blackdrag

--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Cédric Champeau]

Le 22/02/2012 14:08, Jochen Theodorou a écrit :
> Am 22.02.2012 13:50, schrieb Cédric Champeau:
> [...]
>> Then, the constructor assumes that whatever "receiver" is, be it "this"
>> or not, implicitThis is set to true. From what I understand, we should
>> only set implicitThis to true iff receiver==null.
>
> The current logic assumes there is no null receiver at many places.
>
Ok, so what does "true if no object expression was specified otherwise
if some expression was specified for the object on which to evaluate the
method then return false" really mean, in isImplicitThis() method?
Actually, I'm thinking there are many MethodCallExpressions created with
the implicitThis flag set to true, where it shouldn't. In that case, if
implicitThis is true, is there any check that we can make on the
receiver, to verify that actually "no object expression was specified"?

> bye blackdrag
>

Thanks!

--
Cédric Champeau
SpringSource - A Division Of VMware
http://www.springsource.com/
http://twitter.com/CedricChampeau


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email