Improve the error messages printed by the shell when a flow c'tor doesn't match.

tools/shell/src/main/kotlin/net/corda/tools/shell/InteractiveShell.kt

@@ -47,8 +47,7 @@ import java.io.FileDescriptor
 import java.io.FileInputStream
 import java.io.InputStream
 import java.io.PrintWriter
-import java.lang.reflect.InvocationTargetException
+import java.lang.reflect.*
-import java.lang.reflect.UndeclaredThrowableException
 import java.nio.file.Path
 import java.util.*
 import java.util.concurrent.CountDownLatch
@@ -300,6 +299,38 @@ object InteractiveShell {
         override fun toString() = (listOf("No applicable constructor for flow. Problems were:") + errors).joinToString(System.lineSeparator())
     }
 
+    /**
+     * Tidies up a possibly generic type name by chopping off the package names of classes in a hard-coded set of
+     * hierarchies that are known to be widely used and recognised, and also not have (m)any ambiguous names in them.
+     *
+     * This is used for printing error messages when something doesn't match.
+     */
+    private fun maybeAbbreviateGenericType(type: Type, extraRecognisedPackage: String): String {
+        val packagesToAbbreviate = listOf("java.", "net.corda.core.", "kotlin.", extraRecognisedPackage)
+
+        fun shouldAbbreviate(typeName: String) = packagesToAbbreviate.any { typeName.startsWith(it) }
+        fun abbreviated(typeName: String) = if (shouldAbbreviate(typeName)) typeName.split('.').last() else typeName
+
+        fun innerLoop(type: Type): String = when (type) {
+            is ParameterizedType -> {
+                val args: List<String> = type.actualTypeArguments.map(::innerLoop)
+                abbreviated(type.rawType.typeName) + '<' + args.joinToString(", ") + '>'
+            }
+            is GenericArrayType -> {
+                innerLoop(type.genericComponentType) + "[]"
+            }
+            is Class<*> -> {
+                if (type.isArray)
+                    abbreviated(type.simpleName)
+                else
+                    abbreviated(type.name).replace('$', '.')
+            }
+            else -> type.toString()
+        }
+
+        return innerLoop(type)
+    }
+
     // TODO: This utility is generally useful and might be better moved to the node class, or an RPC, if we can commit to making it stable API.
     /**
      * Given a [FlowLogic] class and a string in one-line Yaml form, finds an applicable constructor and starts
@@ -319,10 +350,17 @@ object InteractiveShell {
         // and keep track of the reasons we failed so we can print them out if no constructors are usable.
         val parser = StringToMethodCallParser(clazz, om)
         val errors = ArrayList<String>()
+
+        val classPackage = clazz.packageName
         for (ctor in clazz.constructors) {
             var paramNamesFromConstructor: List<String>? = null
+
             fun getPrototype(): List<String> {
-                val argTypes = ctor.genericParameterTypes.map { it.typeName }
+                val argTypes = ctor.genericParameterTypes.map { it: Type ->
+                    // If the type name is in the net.corda.core or java namespaces, chop off the package name
+                    // because these hierarchies don't have (m)any ambiguous names and the extra detail is just noise.
+                    maybeAbbreviateGenericType(it, classPackage)
+                }
                 return paramNamesFromConstructor!!.zip(argTypes).map { (name, type) -> "$name: $type" }
             }
 

tools/shell/src/test/java/net/corda/tools/shell/InteractiveShellJavaTest.java

@@ -52,8 +52,8 @@ public class InteractiveShellJavaTest {
             }
         }
 
-        public FlowA(Integer b) {
+        public FlowA(int b) {
-            this(b.toString());
+            this(Integer.valueOf(b).toString());
         }
 
         public FlowA(Integer b, String c) {
@@ -111,6 +111,9 @@ public class InteractiveShellJavaTest {
             this.a = a;
         }
 
+        public FlowB(Amount<Currency> amount, int abc) {
+        }
+
         @Nullable
         @Override
         public ProgressTracker getProgressTracker() {
@@ -142,6 +145,7 @@ public class InteractiveShellJavaTest {
             this.label = label;
         }
 
+        @SuppressWarnings("unused")  // Used via reflection.
         public String getLabel() {
             return label;
         }
@@ -160,17 +164,17 @@ public class InteractiveShellJavaTest {
 
     private void check(String input, String expected, Class<? extends StringFlow> flowClass) throws InteractiveShell.NoApplicableConstructor {
         InteractiveShell.INSTANCE.runFlowFromString((clazz, args) -> {
-
             StringFlow instance = null;
             try {
                 instance = (StringFlow)clazz.getConstructor(Arrays.stream(args).map(Object::getClass).toArray(Class[]::new)).newInstance(args);
             } catch (Exception e) {
                 System.out.println(e);
+                throw new RuntimeException(e);
             }
             output = instance.getA();
             OpenFuture<String> future = CordaFutureImplKt.openFuture();
             future.set("ABC");
-            return new FlowProgressHandleImpl(StateMachineRunId.Companion.createRandom(), future, Observable.just("Some string"));
+            return new FlowProgressHandleImpl<String>(StateMachineRunId.Companion.createRandom(), future, Observable.just("Some string"));
         }, input, flowClass, om);
         assertEquals(input, expected, output);
     }
@@ -245,4 +249,14 @@ public class InteractiveShellJavaTest {
     public void unwrapLambda() throws InteractiveShell.NoApplicableConstructor {
         check("party: \"" + megaCorp.getName() + "\", a: Bambam", "Bambam", FlowB.class);
     }
+
+    @Test
+    public void niceErrors() {
+        // Most cases are checked in the Kotlin test, so we only check raw types here.
+        try {
+            check("amount: $100", "", FlowB.class);
+        } catch (InteractiveShell.NoApplicableConstructor e) {
+            assertEquals("[amount: Amount<Currency>, abc: int]: missing parameter abc", e.getErrors().get(1));
+        }
+    }
 }

tools/shell/src/test/kotlin/net/corda/tools/shell/InteractiveShellTest.kt

@@ -14,12 +14,13 @@ import net.corda.core.internal.concurrent.openFuture
 import net.corda.core.messaging.FlowProgressHandleImpl
 import net.corda.core.utilities.ProgressTracker
 import net.corda.node.services.identity.InMemoryIdentityService
-import net.corda.testing.internal.DEV_ROOT_CA
 import net.corda.testing.core.TestIdentity
+import net.corda.testing.internal.DEV_ROOT_CA
 import org.junit.Test
 import rx.Observable
 import java.util.*
 import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
 
 class InteractiveShellTest {
     companion object {
@@ -28,7 +29,7 @@ class InteractiveShellTest {
 
     @Suppress("UNUSED")
     class FlowA(val a: String) : FlowLogic<String>() {
-        constructor(b: Int?) : this(b.toString())
+        constructor(b: Int) : this(b.toString())
         constructor(b: Int?, c: String) : this(b.toString() + c)
         constructor(amount: Amount<Currency>) : this(amount.toString())
         constructor(pair: Pair<Amount<Currency>, SecureHash.SHA256>) : this(pair.toString())
@@ -48,7 +49,6 @@ class InteractiveShellTest {
     private fun check(input: String, expected: String) {
         var output: String? = null
         InteractiveShell.runFlowFromString({ clazz, args ->
-
             val instance = clazz.getConstructor(*args.map { it!!::class.java }.toTypedArray()).newInstance(*args) as FlowA
             output = instance.a
             val future = openFuture<String>()
@@ -101,6 +101,27 @@ class InteractiveShellTest {
     @Test(expected = InteractiveShell.NoApplicableConstructor::class)
     fun flowTooManyParams() = check("b: 12, c: Yo, d: Bar", "")
 
+    @Test
+    fun niceTypeNamesInErrors() {
+        val e = assertFailsWith<InteractiveShell.NoApplicableConstructor> {
+            check("", expected = "")
+        }
+        val correct = setOf(
+                "[amounts: Amount<InteractiveShellTest.UserValue>[]]: missing parameter amounts",
+                "[amount: Amount<Currency>]: missing parameter amount",
+                "[pair: Pair<Amount<Currency>, SecureHash.SHA256>]: missing parameter pair",
+                "[party: Party]: missing parameter party",
+                "[b: Integer, amount: Amount<InteractiveShellTest.UserValue>]: missing parameter b",
+                "[b: String[]]: missing parameter b",
+                "[b: Integer, c: String]: missing parameter b",
+                "[a: String]: missing parameter a",
+                "[b: int]: missing parameter b"
+        )
+        val errors = e.errors.toHashSet()
+        errors.removeAll(correct)
+        assert(errors.isEmpty()) { errors.joinToString(", ") }
+    }
+
     @Test
     fun party() = check("party: \"${megaCorp.name}\"", megaCorp.name.toString())