`Verify` blows up with `groovy.lang.MissingPropertyException`

[boris-petrov]

Describe the bug

@Verify
private void foo(a, b) {
  a.every { it == b }
}

And call it with foo([1, 2, 3], 2). Leads to:

a.every { it == b }
| |
| groovy.lang.MissingPropertyException: No such property: $spock_errorCollector for class: com.company.SomeSpec
|   at com.company.SomeSpec.foo_closure3(SomeSpec.groovy:234)
|   at com.company.SomeSpec.foo(SomeSpec.groovy:234)
|   at com.company.SomeSpec.bar(SomeSpec.groovy:219)
[1, 2, 3]
    at app//com.company.SomeSpec.foo(SomeSpec.groovy:234)
    at com.company.SomeSpec.bar(SomeSpec.groovy:219)

    Caused by:
    groovy.lang.MissingPropertyException: No such property: $spock_errorCollector for class: com.company.SomeSpec
        at com.profuzdigital.data_server.types.SomeSpec.foo_closure3(SomeSpec.groovy:234)
        ... 2 more

To Reproduce

Check the code above.

Expected behavior

No error.

Actual behavior

The error above.

Java version

openjdk version "24.0.1" 2025-04-15

Buildtool version

Gradle 8.14-rc-2

What operating system are you using

Linux

Dependencies

Just Spock.

Additional context

No response

[Vampire]

@pshevche / @leonard84 I think this is because not only top-level conditions are rewritten, but also deeper conditions.
Is there a reason this was done?
Looking at the PR description, only top-level conditions should be rewritten like we have it everywhere else in Spock too.

There are good reasons to only rewrite top-level conditions.
In a.every { it == b } you don't want the it == b to be an implicit assertion.
In this case it might be appropriate as if any of the it == b fails, the every would also fail.
But think about for example a.none { it == b }, in that case failing on it == b is a bad idea, because the check is intended to verify that indeed all theses comparisons fail.
Or !a.any { it == b }, just the same.

It should most probably be changed, so that only what is also rewritten everywhere else is rewritten in such helper methods.
I actually would have expected that here simply the DeepBlockRewriter is used that just behaves like for any other place where we do such rewritings.

[leonard84]

I had instructed @pshevche to also inspect non-toplevel statements, as it has been a constant source of bugs when people use if-else statements with implicit assertions that do not work. And this is something I'd like to change for Spock 3.x.

I have to take a look at why this example below does not blow up, because in condition blocks like with we also instrument implicit assertions in if-statements, but the .every call does not blow up.

import spock.lang.*

class ASpec extends Specification {
  def "implicit"() {
    def a = true
    expect:
    if (a) {
      !a
    }
  }

  def "within with"() {
    def a = true
    expect:
    with("irrelevant") {
      if (a) {
        !a
        b.every { it == c }
      }
    }
  }
  

  def "within with"() {
    def a = true
    def b = [1,1,2]
    def c = 1
    expect:
    with("irrelevant") {
      b.every { it == c }
    }
  }

  def "within myCondition"() {
    def a = true
    def b = [1,1,2]
    def c = 1
    expect:
    ASpec.myCondition {
      b.every { it == c }
    }
  }

  def "within myCondition2"() {
    def a = true
    def b = [1,1,2]
    def c = 1
    expect:
    ASpec.myCondition {
      if (a) {
        !a
      }
    }
  }
  

  @org.spockframework.lang.ConditionBlock
  static void myCondition(Closure cls) {
    cls()
  }
}

Groovy Console

╷
└─ Spock ✔
   └─ ASpec ✔
      ├─ implicit ✔
      ├─ within with ✘ Condition not satisfied:
      │     
      │        !a
      │        ||
      │        |true
      │        false
      ├─ within with ✘ Condition not satisfied:
      │     
      │        b.every { it == c }
      │        | |
      │        | false
      │        [1, 1, 2]
      ├─ within myCondition ✘ Condition not satisfied:
      │     
      │        b.every { it == c }
      │        | |
      │        | false
      │        [1, 1, 2]
      └─ within myCondition2 ✘ Condition not satisfied:
            
               !a
               ||
               |true
               false

[AndreasTu]

@leonard84
In your case of "within myCondition2"() feature the closure got the following synthetic accessor:

@Generated
public ErrorCollector get$spock_errorCollector() {
    return ((Class)$spock_errorCollector.get()).cast<invokedynamic>($spock_errorCollector.get());
}

And a groovy.lang.Reference to the $spock_errorCollector is passed into the constructor.

But in the @Verify with each{} closure there is not such synthetic accessor generated, and then the MissingPropertyException is thrown.

But I do not yet know why...

[Vampire]

Overall I stay with what I said, just not with the only top-level.

As Leonard said, we have other places where we transform more than top-level and like within with { ... }.

But instead of reinventing the wheel and breaking it like here we should just reuse the logic that we have and that is battle-tested and known to work properly.

You must never transform any statement any deep, as you do not want to make the content of every { ... } an assertion and so on.

That is also exactly the difference.
If you don't use he Groovy Console which has a worse transpiler than we have, but copy the example from Leonard and OP to some transpiler spec and look at the results, you see that within the @Verify the it == b is made an implicit assertion which it must never be and thereby uses $spock_errorCollector and $spock_valueRecorder which are simply not defined in that scope, probably again because of reinventing a broken wheel and not inserting the fields where necessary.

In Leonards example you see that in the first "within with" indeed the !a and the b.every are treated as implicit assertions, but the important and correct point is, that the it == c is not treated as implicit assertion and thus also does not use those fields and thus no missing property exception is thrown.

So again, we should throw away the newly added broken rewriting logic and just use the existing battle tested and well working rewriting that does the right things. :-)

[Vampire]

Also, is it expected that you can define interactions in such verify methods?
o_O
And also in a quite strange way actually.
It is first transformed to the interaction registering calls and then those are transformed to an implicit condition.