Implemented partial parameter definitions

Until now the parameters either included all the data table header elements or none.

Their order wasn't enforced, but was replaced with a method call, based on the header,
causing parameter values to be mixed up.

This is changed now to variable number parameter definition, where only the order is enforced:
* first you can define the type of any variable from the data table, followed by arbitrary
number of extra parameters (called with `null`, no default values supported yet)
** the data table header order is compile-time checked
** the extra parameters can serve as type-safety declarations outside the method body
** provided default parameter values are compile-time rejected (not supported yet)
** primitives are auto-wrapped to nullable types (they need default values)

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

@@ -30,6 +30,9 @@
 import org.spockframework.runtime.model.DataProviderMetadata;
 import org.spockframework.util.*;
 
+import static org.spockframework.util.ObjectUtil.firstNonNull;
+import static org.spockframework.util.ReflectionUtil.getDefaultValue;
+
 /**
  *
  * @author Peter Niederwieser
@@ -66,11 +69,37 @@ private void rewrite() {
         resources.getErrorReporter().error(e);
       }
 
+    validateParameterOrder();
+
     whereBlock.getAst().clear();
-    handleFeatureParameters();
+    addFeatureParameters();
     createDataProcessorMethod();
   }
 
+  private void validateParameterOrder() {
+    Iterator<VariableExpression> dataIterator = dataProcessorVars.iterator();
+
+    for (Parameter parameter : whereBlock.getParent().getAst().getParameters()) {
+      String parameterName = parameter.getName();
+
+      if ((find(dataIterator, parameterName) == null) && isDataProcessorVariable(parameterName)) {
+        String message = String.format("Parameter '%s' is in the wrong order!", parameterName);
+        resources.getErrorReporter().error(new InvalidSpecCompileException(parameter, message));
+        return;
+      }
+    }
+  }
+
+  @Nullable
+  private VariableExpression find(Iterator<VariableExpression> dataIterator, String name) {
+    while (dataIterator.hasNext()) {
+      VariableExpression variable = dataIterator.next();
+      if (name.equals(variable.getText()))
+        return variable;
+    }
+    return null;
+  }
+
   private void rewriteWhereStat(ListIterator<Statement> stats) throws InvalidSpecCompileException {
     Statement stat = stats.next();
     BinaryExpression binExpr = AstUtil.getExpression(stat, BinaryExpression.class);
@@ -300,44 +329,72 @@ private void verifyDataProcessorVariable(VariableExpression varExpr) {
       return;
     }
 
-    if (getDataProcessorVariable(varExpr.getName()) != null) {
+    if (isDataProcessorVariable(varExpr.getName())) {
       resources.getErrorReporter().error(varExpr, "Duplicate declaration of data variable '%s'", varExpr.getName());
       return;
     }
-
-    if (whereBlock.getParent().getAst().getParameters().length > 0 && !(accessedVar instanceof Parameter)) {
-      resources.getErrorReporter().error(varExpr,
-          "Data variable '%s' needs to be declared as method parameter",
-          varExpr.getName());
-    }
   }
 
-  private VariableExpression getDataProcessorVariable(String name) {
+  private boolean isDataProcessorVariable(String name) {
     for (VariableExpression var : dataProcessorVars)
-      if (var.getName().equals(name)) return var;
+      if (name.equals(var.getName()))
+        return true;
+    return false;
+  }
 
-    return null;
+  private void addFeatureParameters() {
+    MethodNode methodNode = whereBlock.getParent().getAst();
+
+    List<Parameter> newParameters = new ArrayList<Parameter>();
+
+    Map<String, Parameter> parameterTypes = getParameters(methodNode);
+    newParameters.addAll(getDataProcessorParameters(parameterTypes));
+    newParameters.addAll(getExtraParameters(parameterTypes));
+
+    methodNode.setParameters(newParameters.toArray(new Parameter[newParameters.size()]));
+  }
+
+  private Map<String, Parameter> getParameters(MethodNode methodNode) {
+    Map<String, Parameter> result = new LinkedHashMap<String, Parameter>();
+    for (Parameter parameter : methodNode.getParameters())
+      result.put(parameter.getName(), parameter);
+    return result;
   }
 
-  private void handleFeatureParameters() {
-    Parameter[] parameters = whereBlock.getParent().getAst().getParameters();
-    if (parameters.length == 0)
-      addFeatureParameters();
-    else
-      checkAllParametersAreDataVariables(parameters);
+  private List<Parameter> getDataProcessorParameters(Map<String, Parameter> parameterTypes) {
+    List<Parameter> results = new ArrayList<Parameter>();
+    for (VariableExpression variableExpression : dataProcessorVars) {
+      String name = variableExpression.getName();
+      Parameter parameter = firstNonNull(parameterTypes.remove(name),
+                                         new Parameter(ClassHelper.DYNAMIC_TYPE, name));
+      validateDefaultValue(parameter);
+      results.add(parameter);
+    }
+    return results;
   }
 
-  private void checkAllParametersAreDataVariables(Parameter[] parameters) {
-    for (Parameter param : parameters)
-      if (getDataProcessorVariable(param.getName()) == null)
-        resources.getErrorReporter().error(param, "Parameter '%s' does not refer to a data variable", param.getName());
+  private List<Parameter> getExtraParameters(Map<String, Parameter> parameterTypes) {
+    List<Parameter> results = new ArrayList<Parameter>();
+    for (Parameter parameter : parameterTypes.values()) {
+      validateDefaultValue(parameter);
+      ClassNode type = getNullableType(parameter);
+      results.add(new Parameter(type, parameter.getName()));
+    }
+    return results;
   }
 
-  private void addFeatureParameters() {
-    Parameter[] parameters = new Parameter[dataProcessorVars.size()];
-    for (int i = 0; i < dataProcessorVars.size(); i++)
-      parameters[i] = new Parameter(ClassHelper.DYNAMIC_TYPE, dataProcessorVars.get(i).getName());
-    whereBlock.getParent().getAst().setParameters(parameters);
+  private void validateDefaultValue(Parameter parameter) {
+    if (parameter.hasInitialExpression()) {
+      String message = String.format("Parameter '%s' has default value, which is not supported yet!", parameter.getName());
+      resources.getErrorReporter().error(new InvalidSpecCompileException(parameter, message));
+    }
+  }
+
+  private ClassNode getNullableType(Parameter parameter) {
+    ClassNode type = parameter.getType();
+    if (type.getTypeClass().isPrimitive())
+      type = ClassHelper.make(getDefaultValue(type.getTypeClass()).getClass());
+    return type;
   }
 
   @SuppressWarnings("unchecked")

spock-core/src/main/java/org/spockframework/runtime/ParameterizedSpecRunner.java

@@ -16,12 +16,10 @@
 
 package org.spockframework.runtime;
 
-import java.util.Iterator;
-import java.util.List;
+import java.util.*;
 
 import static org.spockframework.runtime.RunStatus.*;
 import org.spockframework.runtime.model.*;
-import org.spockframework.runtime.GroovyRuntimeUtil;
 
 /**
  * Adds the ability to run parameterized features.
@@ -116,7 +114,14 @@ private void runIterations(Iterator[] iterators, int estimatedNumIterations) {
     if (runStatus != OK) return;
 
     while (haveNext(iterators)) {
-      initializeAndRunIteration(nextArgs(iterators), estimatedNumIterations);
+      List<Object> dataValues = new ArrayList<Object>(Arrays.asList(nextArgs(iterators)));
+
+      /* TODO should we take default parameter values into consideration instead? */
+      int extraParameterCount = currentFeature.getDataVariables().size() - dataValues.size();
+      for (int i = 0; i < extraParameterCount; i++)
+        dataValues.add(null);
+
+      initializeAndRunIteration(dataValues.toArray(), estimatedNumIterations);
 
       if (resetStatus(ITERATION) != OK) break;
       // no iterators => no data providers => only derived parameterizations => limit to one iteration
@@ -173,7 +178,7 @@ private SpockExecutionException createDifferentNumberOfDataValuesException(DataP
 
   // advances iterators and computes args
   private Object[] nextArgs(Iterator[] iterators) {
-    if (runStatus != OK) return null;
+    if (runStatus != OK) return EMPTY_ARGS;
 
     Object[] next = new Object[iterators.length];
     for (int i = 0; i < iterators.length; i++)
@@ -182,15 +187,15 @@ private Object[] nextArgs(Iterator[] iterators) {
       } catch (Throwable t) {
         runStatus = supervisor.error(
             new ErrorInfo(currentFeature.getDataProviders().get(i).getDataProviderMethod(), t));
-        return null;
+        return EMPTY_ARGS;
       }
 
     try {
       return (Object[])invokeRaw(sharedInstance, currentFeature.getDataProcessorMethod(), next);
     } catch (Throwable t) {
       runStatus = supervisor.error(
           new ErrorInfo(currentFeature.getDataProcessorMethod(), t));
-      return null;
+      return EMPTY_ARGS;
     }
   }
 }

spock-core/src/main/java/org/spockframework/util/ObjectUtil.java

@@ -46,6 +46,14 @@ public static boolean eitherNull(Object... objs) {
     return false;
   }
 
+  public static <T> T firstNonNull(T... objs) {
+    for (T obj : objs) {
+      if (obj != null)
+        return obj;
+    }
+    throw new IllegalArgumentException("All elements were null!");
+  }
+
   @SuppressWarnings("unchecked")
   public static @Nullable <T> T asInstance(Object obj, Class<T> type) {
     return type.isInstance(obj) ? (T) obj : null;
@@ -57,4 +65,4 @@ public static <T extends Comparable<T>> int compare(T comparable1, T comparable2
     if (comparable2 == null) return 1;
     return comparable1.compareTo(comparable2);
   }
-}
+}

spock-specs/src/test/groovy/org/spockframework/smoke/parameterization/DataTables.groovy

@@ -28,8 +28,6 @@ class DataTables extends EmbeddedSpecification {
   @Shared
   def sharedField = 42
 
-  def instanceField = 42
-
   def "basic usage"() {
     expect:
     Math.max(a, b) == c
@@ -138,6 +136,51 @@ a | a
     3 | "wow"
   }
 
+  def 'invalid parameter order should cause a compile error'() {
+    when:
+    compiler.compileSpecBody """
+      def 'invalid parameter order'(${parameters.join(', ')}) {
+        expect: false
+        where:  a << [1]
+
+                b | c
+                2 | 3
+
+                d = 4
+      }
+    """
+
+    then:
+    thrown InvalidSpecCompileException
+
+    where: 'all invalid permutations of the parameters'
+    parameters << ('a'..'e').subsequences().collectMany { it.permutations() }.findAll { it != it.sort(false) }
+  }
+
+  def 'parameters should be ordered according to data table header'() {
+    expect: runner.runSpecBody """
+      def 'valid parameter order'(${parameters.join(', ')}) {
+        expect: a + b + c + d == (1..4).sum()
+        where:  a << [1]
+
+                b | c
+                2 | 3
+
+                d = 4
+      }
+    """
+
+    where: 'all valid permutations of the parameters'
+    parameters << ('a'..'e').subsequences().collectMany { it.permutations() }.findAll { it == it.sort(false) }
+  }
+
+  def 'partial parameter definition is possible'(int b, c) {
+    expect: a + b + c > 0
+    where:  a | b | c
+            1 | 2 | 3
+            4 | 5 | 6
+  }
+
   def "pseudo-column can be omitted from parameter list"(a) {
     expect:
     a == 3
@@ -156,15 +199,14 @@ a | a
     3 | _
   }
 
-  def "tables can be mixed with other parameterizations"() {
-    expect:
-    [a, b, c, d] == [1, 2, 3, 4]
-
-    where:
-    a << [1]
-    b | c
-    2 | 3
-    d = c + 1
+  def 'tables can be mixed with other parameterizations'(int a, int b, int c, int d) {
+    expect: [a, b, c, d] == [1, 2, 3, 4] 
+    where:  a << [1]
+
+            b | c
+            2 | 3
+
+            d = c + 1
   }
 
   def "cells may contain arbitrary expressions"() {