Fix @verify and @VerifyAll method compilation (#2249)

Fixes #2150

---------

Co-authored-by: Leonard Brünings <[email protected]>

Author: Vampire

docs/release_notes.adoc

@@ -71,6 +71,7 @@ will now require that the spec is annotated with <<parallel_execution.adoc#isola
 
 === Misc
 
+* Fix handling of `@Verify` and `@VerifyAll` spockIssue:2150[]
 * Fix handling of condition method calls spockPull:2162[]
 * Fix vararg handling in SpyStatic spockIssue:2161[]
 * Fix incompatibility with JUnit 6 in OSGi environment spockIssue:2231[]

docs/spock_primer.adoc

@@ -577,13 +577,17 @@ false                         ...
 
 Not very helpful. Fortunately, we can do better:
 
+[#at-verify-helper-method]
+=== Using `@Verify` helper methods to assert expectations
+
 [source,groovy,indent=0]
 ----
 include::{sourcedir}/primer/VerifyMethodsDocSpec.groovy[tag=verify-helper-method]
 ----
 
-When factoring out conditions into a helper method, you need to ensure the method has return type `void`.
-Otherwise, Spock might interpret the return value as a failing condition, which is not what we want.
+When factoring out conditions into a `@Verify` annotated helper method, you need to ensure the method has a return type of `void`, or uses a dynamic return type (`def` or no declared type).
+After compilation, it will be `void` in both cases, and any other return type causes a compilation error, as Spock might interpret the return value as a failing condition otherwise.
+You can use `@Verify` for instance methods or static methods of a Specification class, or in separate utility classes.
 
 As expected, the improved helper method tells us exactly what's wrong:
 
@@ -684,6 +688,8 @@ Alternatively, `verifyAll` conditions can be extracted into a helper method anno
 include::{sourcedir}/primer/VerifyMethodsDocSpec.groovy[tag=verify-all-helper-method]
 ----
 
+It has the same return type constraints as the <<at-verify-helper-method,@Verify>> methods.
+
 == Using `verifyEach` to assert on each element of an `Iterable`
 
 There are several ways to do assertions on `Iterable` or `Collection`.

spock-core/src/main/java/org/spockframework/compiler/AbstractDeepBlockRewriter.java

@@ -29,7 +29,7 @@ public class AbstractDeepBlockRewriter extends StatementReplacingVisitorSupport
   protected BinaryExpression currBinaryExpr;
   protected MethodCallExpression currMethodCallExpr;
   protected ClosureExpression currClosure;
-  protected ISpecialMethodCall currSpecialMethodCall = NoSpecialMethodCall.INSTANCE;
+  protected ISpecialMethodCall currSpecialMethodCall;
   protected Collection<Statement> pastSpecialMethodCallStats = new HashSet<>();
 
   // following fields are filled in by subclasses
@@ -41,8 +41,13 @@ public class AbstractDeepBlockRewriter extends StatementReplacingVisitorSupport
   protected final List<Statement> thenBlockInteractionStats = new ArrayList<>();
 
   public AbstractDeepBlockRewriter(Block block, AstNodeCache nodeCache) {
+    this(block, nodeCache, NoSpecialMethodCall.INSTANCE);
+  }
+
+  public AbstractDeepBlockRewriter(Block block, AstNodeCache nodeCache, ISpecialMethodCall currSpecialMethodCall) {
     this.block = block;
     this.nodeCache = nodeCache;
+    this.currSpecialMethodCall = currSpecialMethodCall;
   }
 
   protected void conditionFound() {

spock-core/src/main/java/org/spockframework/compiler/AbstractSpecVisitor.java

@@ -43,6 +43,8 @@ public void visitWhenBlock(WhenBlock block) throws Exception {}
   @Override
   public void visitThenBlock(ThenBlock block) throws Exception {}
   @Override
+  public void visitVerifyBlock(VerifyBlock block) throws Exception {}
+  @Override
   public void visitCleanupBlock(CleanupBlock block) throws Exception {}
   @Override
   public void visitWhereBlock(WhereBlock block) throws Exception {}

spock-core/src/main/java/org/spockframework/compiler/AstNodeCache.java

@@ -25,6 +25,8 @@
 import spock.lang.Specification;
 
 import org.codehaus.groovy.ast.*;
+import spock.lang.Verify;
+import spock.lang.VerifyAll;
 
 import java.util.Set;
 
@@ -46,6 +48,7 @@ public class AstNodeCache {
   public final ClassNode DataVariableMultiplication = ClassHelper.makeWithoutCaching(DataVariableMultiplication.class);
   public final ClassNode DataVariableMultiplicationFactor = ClassHelper.makeWithoutCaching(DataVariableMultiplicationFactor.class);
   public final ClassNode BlockInfo = ClassHelper.makeWithoutCaching(BlockInfo.class);
+  public final ClassNode Void = ClassHelper.makeWithoutCaching(void.class);
 
   public final MethodNode Specification_GetSpecificationContext =
     Specification.getDeclaredMethods(Identifiers.GET_SPECIFICATION_CONTEXT).get(0);

spock-core/src/main/java/org/spockframework/compiler/DeepBlockRewriter.java

@@ -22,7 +22,6 @@
 import org.codehaus.groovy.ast.stmt.Statement;
 import org.codehaus.groovy.syntax.Types;
 import org.spockframework.compiler.model.*;
-import org.spockframework.util.Identifiers;
 import org.spockframework.util.Nullable;
 
 import java.util.List;
@@ -43,13 +42,17 @@
  * @author Peter Niederwieser
  */
 public class DeepBlockRewriter extends AbstractDeepBlockRewriter {
-  private final ISpecRewriteResources resources;
+  private final IRewriteResources resources;
   private boolean insideInteraction = false;
   private int interactionClosureDepth = 0;
   private int closureDepth = 0;
 
-  public DeepBlockRewriter(ISpecRewriteResources resources) {
-    super(resources.getCurrentBlock(), resources.getAstNodeCache());
+  public DeepBlockRewriter(IRewriteResources resources) {
+    this(resources, NoSpecialMethodCall.INSTANCE);
+  }
+
+  public DeepBlockRewriter(IRewriteResources resources, ISpecialMethodCall currSpecialMethodCall) {
+    super(resources.getCurrentBlock(), resources.getAstNodeCache(), currSpecialMethodCall);
     this.resources = resources;
   }
 
@@ -72,7 +75,7 @@ private String getValueRecorderSuffix() {
   }
 
   private String getErrorCollectorSuffix() {
-    return groupConditionFound ? String.valueOf(closureDepth) : "";
+    return (groupConditionFound && (closureDepth > 0)) ? String.valueOf(closureDepth) : "";
   }
 
   @Override
@@ -187,6 +190,11 @@ private boolean handleInteraction(InteractionRewriter rewriter, ExpressionStatem
       return true;
     }
 
+    if (block instanceof VerifyBlock) {
+      resources.getErrorReporter().error(stat, "Interactions are not allowed in '@Verify' and '@VerifyAll' methods.");
+      return true;
+    }
+
     replaceVisitedStatementWith(interaction);
     interactionFound = true;
     return true;

spock-core/src/main/java/org/spockframework/compiler/IRewriteResources.java

@@ -17,14 +17,27 @@
 package org.spockframework.compiler;
 
 import org.codehaus.groovy.ast.ASTNode;
-import org.spockframework.compiler.condition.IConditionErrorRecorders;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.expr.VariableExpression;
 
-import java.util.List;
+import org.spockframework.compiler.condition.IConditionErrorRecorders;
+import org.spockframework.compiler.model.Block;
+import org.spockframework.compiler.model.Method;
 
 /**
+ *
  * @author Peter Niederwieser
  */
 public interface IRewriteResources {
+  Method getCurrentMethod();
+
+  Block getCurrentBlock();
+
+  VariableExpression captureOldValue(Expression oldValue);
+
+  MethodCallExpression getMockInvocationMatcher();
+
   AstNodeCache getAstNodeCache();
 
   String getSourceText(ASTNode node);

spock-core/src/main/java/org/spockframework/compiler/InteractionRewriter.java

@@ -36,7 +36,7 @@
  * @author Peter Niederwieser
  */
 public class InteractionRewriter {
-  private final ISpecRewriteResources resources;
+  private final IRewriteResources resources;
   private final ClosureExpression activeWithOrMockClosure;
 
   // information about the interaction; filled in by parse()
@@ -52,7 +52,7 @@ public class InteractionRewriter {
   // "new InteractionBuilder(..).setCount(..).setTarget(..).setMethod(..).addArg(..).addResult(..).build()"
   private Expression builderExpr;
 
-  public InteractionRewriter(ISpecRewriteResources resources, @Nullable ClosureExpression activeWithOrMockClosure) {
+  public InteractionRewriter(IRewriteResources resources, @Nullable ClosureExpression activeWithOrMockClosure) {
     this.resources = resources;
     this.activeWithOrMockClosure = activeWithOrMockClosure;
   }

spock-core/src/main/java/org/spockframework/compiler/SpecParser.java

@@ -24,7 +24,10 @@
 import org.codehaus.groovy.ast.*;
 import org.codehaus.groovy.ast.expr.ConstantExpression;
 import org.codehaus.groovy.ast.stmt.Statement;
+import spock.lang.Verify;
+import spock.lang.VerifyAll;
 
+import static org.spockframework.compiler.AstUtil.hasAnnotation;
 import static org.spockframework.util.Identifiers.*;
 
 /**
@@ -34,13 +37,15 @@
  * @author Peter Niederwieser
  */
 public class SpecParser implements GroovyClassVisitor {
+  private final AstNodeCache nodeCache;
   private final ErrorReporter errorReporter;
 
   private Spec spec;
   private int fieldCount = 0;
   private int featureMethodCount = 0;
 
-  public SpecParser(ErrorReporter errorReporter) {
+  public SpecParser(AstNodeCache nodeCache, ErrorReporter errorReporter) {
+    this.nodeCache = nodeCache;
     this.errorReporter = errorReporter;
   }
 
@@ -65,7 +70,7 @@ public void visitField(FieldNode gField) {
     if (gField.isStatic()) return;
 
     Field field = new Field(spec, gField, fieldCount++);
-    field.setShared(AstUtil.hasAnnotation(gField, Shared.class));
+    field.setShared(hasAnnotation(gField, Shared.class));
     field.setOwner(owner);
     spec.getFields().add(field);
   }
@@ -90,6 +95,8 @@ public void visitMethod(MethodNode method) {
       buildFixtureMethod(method);
     else if (isFeatureMethod(method))
       buildFeatureMethod(method);
+    else if (isVerifyMethod(method))
+      buildVerifyMethod(method);
     else buildHelperMethod(method);
   }
 
@@ -176,6 +183,29 @@ private void buildFeatureMethod(MethodNode method) {
     spec.getMethods().add(feature);
   }
 
+  private boolean isVerifyMethod(MethodNode method) {
+    boolean hasVerifyAnnotation = hasAnnotation(method, Verify.class);
+    boolean hasVerifyAllAnnotation = hasAnnotation(method, VerifyAll.class);
+    if (hasVerifyAnnotation && hasVerifyAllAnnotation) {
+      errorReporter.error(method, "Method '%s' cannot be annotated with both @Verify and @VerifyAll.", method.getName());
+      return false;
+    }
+    return hasVerifyAnnotation || hasVerifyAllAnnotation;
+  }
+
+  private void buildVerifyMethod(MethodNode method) {
+    VerifyMethod.verifyReturnType(method, errorReporter);
+    method.setReturnType(nodeCache.Void);
+
+    Method verify;
+    if (hasAnnotation(method, Verify.class)) {
+      verify = new VerifyMethod(spec, method);
+    } else {
+      verify = new VerifyAllMethod(spec, method);
+    }
+    spec.getMethods().add(verify);
+  }
+
   private void buildHelperMethod(MethodNode method) {
     Method helper = new HelperMethod(spec, method);
     spec.getMethods().add(helper);

spock-core/src/main/java/org/spockframework/compiler/SpecRewriter.java

@@ -28,6 +28,7 @@
 import org.spockframework.compiler.model.*;
 import org.spockframework.runtime.GroovyRuntimeUtil;
 import org.spockframework.runtime.SpockException;
+import org.spockframework.util.Identifiers;
 import org.spockframework.util.InternalIdentifiers;
 import org.spockframework.util.ObjectUtil;
 import org.spockframework.util.ReflectionUtil;
@@ -45,7 +46,7 @@
  * @author Peter Niederwieser
  */
 // IDEA: mock controller / leaveScope calls should only be inserted when necessary (increases robustness)
-public class SpecRewriter extends AbstractSpecVisitor implements ISpecRewriteResources {
+public class SpecRewriter extends AbstractSpecVisitor implements IRewriteResources {
   // https://issues.apache.org/jira/browse/GROOVY-10403
   // needed for groovy-4 compatibility and only available since groovy-4
   private static final java.lang.reflect.Method GET_PLAIN_NODE_REFERENCE = ReflectionUtil.getMethodBySignature(ClassNode.class, "getPlainNodeReference", boolean.class);
@@ -412,12 +413,16 @@ public void visitMethodAgain(Method method) {
       method.getStatements().add(createMockControllerCall(nodeCache.MockController_LeaveScope));
     }
 
+    if (method instanceof VerifyAllMethod) {
+      if (methodHasCondition) {
+        errorRecorders.defineErrorCollector(method.getStatements());
+      }
+    } else if (methodHasDeepNonGroupedCondition) {
+      errorRecorders.defineErrorRethrower(method.getStatements());
+    }
     if (methodHasCondition) {
       errorRecorders.defineValueRecorder(method.getStatements());
     }
-    if (methodHasDeepNonGroupedCondition) {
-      errorRecorders.defineErrorRethrower(method.getStatements());
-    }
   }
 
 
@@ -474,13 +479,24 @@ private MethodCallExpression createBlockListenerCall(Block block, MethodNode blo
   public void visitAnyBlock(Block block) {
     this.block = block;
     if (block instanceof ThenBlock) return;
+    if (block instanceof VerifyBlock) return;
 
     DeepBlockRewriter deep = new DeepBlockRewriter(this);
     deep.visit(block);
     methodHasCondition |= deep.isConditionFound();
     methodHasDeepNonGroupedCondition |= deep.isDeepNonGroupedConditionFound();
   }
 
+  @Override
+  public void visitVerifyBlock(VerifyBlock block) {
+    DeepBlockRewriter deep = new DeepBlockRewriter(this, new SpecialMethodCall(
+      method instanceof VerifyAllMethod ? Identifiers.VERIFY_ALL : Identifiers.WITH,
+      null, null, null, null, null, false, nodeCache));
+    deep.visit(block);
+    methodHasCondition |= deep.isConditionFound();
+    methodHasDeepNonGroupedCondition |= deep.isDeepNonGroupedConditionFound();
+  }
+
   @Override
   public void visitThenBlock(ThenBlock block) {
     if (block.isFirstInChain()) thenBlockChainHasExceptionCondition = false;
@@ -651,11 +667,6 @@ private CatchStatement createHandleSuppressedThrowableStatement(VariableExpressi
 
   // IRewriteResources members
 
-  @Override
-  public Spec getCurrentSpec() {
-    return spec;
-  }
-
   @Override
   public Method getCurrentMethod() {
     return method;