Repository: sjbarag/brs
Pull request #663 (2021-06-21 22:01:37)

fix(stdlib, interp): fix `RoSGNode#callFunc` argument error reporting
# Summary

This fixes a bug where we would throw a _Javascript_ error and display the resulting stack trace to users who used `RoSGNode#callFunc` with incorrect/missing arguments. I also consolidated our "incorrect arguments" error and removed some `finally`s that weren't behaving the way we expected them to.


Patch:
diff --git a/src/brsTypes/Callable.ts b/src/brsTypes/Callable.ts
index 7323ebc..d24d60c 100644
--- a/src/brsTypes/Callable.ts
+++ b/src/brsTypes/Callable.ts
@@ -3,11 +3,8 @@ import * as Brs from ".";
 import * as Expr from "../parser/Expression";
 import { Scope } from "../interpreter/Environment";
 import { Location } from "../lexer";
-import { Int32 } from "./Int32";
-import { Float } from "./Float";
-import { Double } from "./Double";
-import { Int64 } from "./Int64";
 import { tryCoerce } from "./coercion";
+import { generateArgumentMismatchError } from "../interpreter/ArgumentMismatch";
 
 /** An argument to a BrightScript `function` or `sub`. */
 export interface Argument {
@@ -157,10 +154,7 @@ export class Callable implements Brs.BrsValue {
     call(interpreter: Interpreter, ...args: Brs.BrsType[]) {
         let satisfiedSignature = this.getFirstSatisfiedSignature(args);
         if (satisfiedSignature == null) {
-            throw new Error(
-                "BrightScript function called without first checking for satisfied signatures. " +
-                    "Ensure `Callable#getAllSignatureMismatches` is called before `Callable#call`."
-            );
+            interpreter.addError(generateArgumentMismatchError(this, args, interpreter.location));
         }
 
         let { signature, impl } = satisfiedSignature;
diff --git a/src/brsTypes/components/RoSGNode.ts b/src/brsTypes/components/RoSGNode.ts
index 7369529..59802fd 100644
--- a/src/brsTypes/components/RoSGNode.ts
+++ b/src/brsTypes/components/RoSGNode.ts
@@ -27,6 +27,7 @@ import { roInvalid } from "./RoInvalid";
 import type * as MockNodeModule from "../../extensions/MockNode";
 import { BlockEnd } from "../../parser/Statement";
 import { Stmt } from "../../parser";
+import { generateArgumentMismatchError } from "../../interpreter/ArgumentMismatch";
 
 interface BrsCallback {
     interpreter: Interpreter;
@@ -643,8 +644,16 @@ export class RoSGNode extends BrsComponent implements BrsValue, BrsIterable {
                             // Determine whether the function should get arguments or not.
                             if (functionToCall.getFirstSatisfiedSignature(functionargs)) {
                                 return functionToCall.call(subInterpreter, ...functionargs);
-                            } else {
+                            } else if (functionToCall.getFirstSatisfiedSignature([])) {
                                 return functionToCall.call(subInterpreter);
+                            } else {
+                                return interpreter.addError(
+                                    generateArgumentMismatchError(
+                                        functionToCall,
+                                        functionargs,
+                                        subInterpreter.stack[subInterpreter.stack.length - 1]
+                                    )
+                                );
                             }
                         } catch (reason) {
                             if (!(reason instanceof Stmt.ReturnValue)) {
diff --git a/src/interpreter/ArgumentMismatch.ts b/src/interpreter/ArgumentMismatch.ts
new file mode 100644
index 0000000..61fbb4b
--- /dev/null
+++ b/src/interpreter/ArgumentMismatch.ts
@@ -0,0 +1,59 @@
+import { ValueKind, SignatureAndMismatches, MismatchReason, Callable, BrsType } from "../brsTypes";
+import { BrsError } from "../Error";
+import { Location } from "../lexer";
+
+function formatMismatch(functionName: string, mismatchedSignature: SignatureAndMismatches) {
+    let sig = mismatchedSignature.signature;
+    let mismatches = mismatchedSignature.mismatches;
+
+    let messageParts = [];
+
+    let args = sig.args
+        .map((a) => {
+            let requiredArg = `${a.name.text} as ${ValueKind.toString(a.type.kind)}`;
+            if (a.defaultValue) {
+                return `[${requiredArg}]`;
+            } else {
+                return requiredArg;
+            }
+        })
+        .join(", ");
+    messageParts.push(`function ${functionName}(${args}) as ${ValueKind.toString(sig.returns)}:`);
+    messageParts.push(
+        ...mismatches
+            .map((mm) => {
+                switch (mm.reason) {
+                    case MismatchReason.TooFewArguments:
+                        return `* ${functionName} requires at least ${mm.expected} arguments, but received ${mm.received}.`;
+                    case MismatchReason.TooManyArguments:
+                        return `* ${functionName} accepts at most ${mm.expected} arguments, but received ${mm.received}.`;
+                    case MismatchReason.ArgumentTypeMismatch:
+                        return `* Argument '${mm.argName}' must be of type ${mm.expected}, but received ${mm.received}.`;
+                }
+            })
+            .map((line) => `    ${line}`)
+    );
+
+    return messageParts.map((line) => `    ${line}`).join("\n");
+}
+
+export function generateArgumentMismatchError(
+    callee: Callable,
+    args: BrsType[],
+    location: Location
+): BrsError {
+    let functionName = callee.getName();
+    let mismatchedSignatures = callee.getAllSignatureMismatches(args);
+
+    let header;
+    let messages;
+    if (mismatchedSignatures.length === 1) {
+        header = `Provided arguments don't match ${functionName}'s signature.`;
+        messages = [formatMismatch(functionName, mismatchedSignatures[0])];
+    } else {
+        header = `Provided arguments don't match any of ${functionName}'s signatures.`;
+        messages = mismatchedSignatures.map(formatMismatch.bind(null, functionName));
+    }
+
+    return new BrsError([header, ...messages].join("\n"), location);
+}
diff --git a/src/interpreter/index.ts b/src/interpreter/index.ts
index 064ed5e..ac75fb9 100644
--- a/src/interpreter/index.ts
+++ b/src/interpreter/index.ts
@@ -11,17 +11,13 @@ import {
     BrsString,
     isBrsBoolean,
     Int32,
-    Int64,
     isBrsCallable,
     Uninitialized,
     RoArray,
     isIterable,
-    SignatureAndMismatches,
-    MismatchReason,
     Callable,
     BrsNumber,
     Float,
-    Double,
     tryCoerce,
 } from "../brsTypes";
 
@@ -47,6 +43,7 @@ import { ComponentDefinition } from "../componentprocessor";
 import pSettle from "p-settle";
 import { CoverageCollector } from "../coverage";
 import { ManifestValue } from "../preprocessor/Manifest";
+import { generateArgumentMismatchError } from "./ArgumentMismatch";
 
 /** The set of options used to configure an interpreter's execution. */
 export interface ExecutionOptions {
@@ -237,15 +234,19 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
         // Set the focused node of the sub env, because our current env has the most up-to-date reference.
         newEnv.setFocusedNode(this._environment.getFocusedNode());
 
+        let returnValue;
         try {
             this._environment = newEnv;
-            return func(this);
+            returnValue = func(this);
+            this._environment = originalEnvironment;
+            this._environment.setFocusedNode(newEnv.getFocusedNode());
         } catch (err) {
-            throw err;
-        } finally {
             this._environment = originalEnvironment;
             this._environment.setFocusedNode(newEnv.getFocusedNode());
+            throw err;
         }
+
+        return returnValue;
     }
 
     exec(statements: ReadonlyArray<Stmt.Statement>, ...args: BrsType[]) {
@@ -1170,57 +1171,8 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
                 return returnedValue || BrsInvalid.Instance;
             }
         } else {
-            function formatMismatch(mismatchedSignature: SignatureAndMismatches) {
-                let sig = mismatchedSignature.signature;
-                let mismatches = mismatchedSignature.mismatches;
-
-                let messageParts = [];
-
-                let args = sig.args
-                    .map((a) => {
-                        let requiredArg = `${a.name.text} as ${ValueKind.toString(a.type.kind)}`;
-                        if (a.defaultValue) {
-                            return `[${requiredArg}]`;
-                        } else {
-                            return requiredArg;
-                        }
-                    })
-                    .join(", ");
-                messageParts.push(
-                    `function ${functionName}(${args}) as ${ValueKind.toString(sig.returns)}:`
-                );
-                messageParts.push(
-                    ...mismatches
-                        .map((mm) => {
-                            switch (mm.reason) {
-                                case MismatchReason.TooFewArguments:
-                                    return `* ${functionName} requires at least ${mm.expected} arguments, but received ${mm.received}.`;
-                                case MismatchReason.TooManyArguments:
-                                    return `* ${functionName} accepts at most ${mm.expected} arguments, but received ${mm.received}.`;
-                                case MismatchReason.ArgumentTypeMismatch:
-                                    return `* Argument '${mm.argName}' must be of type ${mm.expected}, but received ${mm.received}.`;
-                            }
-                        })
-                        .map((line) => `    ${line}`)
-                );
-
-                return messageParts.map((line) => `    ${line}`).join("\n");
-            }
-
-            let mismatchedSignatures = callee.getAllSignatureMismatches(args);
-
-            let header;
-            let messages;
-            if (mismatchedSignatures.length === 1) {
-                header = `Provided arguments don't match ${functionName}'s signature.`;
-                messages = [formatMismatch(mismatchedSignatures[0])];
-            } else {
-                header = `Provided arguments don't match any of ${functionName}'s signatures.`;
-                messages = mismatchedSignatures.map(formatMismatch);
-            }
-
             return this.addError(
-                new BrsError([header, ...messages].join("\n"), expression.closingParen.location)
+                generateArgumentMismatchError(callee, args, expression.closingParen.location)
             );
         }
     }
@@ -1671,8 +1623,10 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
         let value;
         try {
             value = expression.accept<BrsType>(this);
-        } finally {
             this.stack.pop();
+        } catch (err) {
+            this.stack.pop();
+            throw err;
         }
 
         return value;
@@ -1686,8 +1640,10 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
         let value;
         try {
             value = statement.accept<BrsType>(this);
-        } finally {
             this.stack.pop();
+        } catch (err) {
+            this.stack.pop();
+            throw err;
         }
 
         return value;
@@ -1697,7 +1653,7 @@ export class Interpreter implements Expr.Visitor<BrsType>, Stmt.Visitor<BrsType>
      * Emits an error via this processor's `events` property, then throws it.
      * @param err the ParseError to emit then throw
      */
-    private addError(err: BrsError): never {
+    public addError(err: BrsError): never {
         this.errors.push(err);
         this.events.emit("err", err);
         throw err;


Test patch:
diff --git a/test/e2e/CallFunc.test.js b/test/e2e/CallFunc.test.js
index 0d77b0a..1063d9d 100644
--- a/test/e2e/CallFunc.test.js
+++ b/test/e2e/CallFunc.test.js
@@ -1,5 +1,7 @@
 const { execute } = require("../../lib");
-const { createMockStreams, resourceFile, allArgs } = require("./E2ETests");
+let { createMockStreams, resourceFile, allArgs } = require("./E2ETests");
+
+resourceFile = resourceFile.bind(null, "components", "call-func", "scripts");
 
 describe("callFunc", () => {
     let outputStreams;
@@ -18,10 +20,7 @@ describe("callFunc", () => {
     });
 
     test("components/call-func/scripts/main.brs", async () => {
-        await execute(
-            [resourceFile("components", "call-func", "scripts", "main.brs")],
-            outputStreams
-        );
+        await execute([resourceFile("main.brs")], outputStreams);
 
         expect(allArgs(outputStreams.stdout.write).filter((arg) => arg !== "\n")).toEqual([
             "component: inside oneArg, args.test: ",
@@ -61,4 +60,32 @@ describe("callFunc", () => {
             "Warning calling function in CallFuncTestbed: no function interface specified for privateFunc",
         ]);
     });
+
+    test("components/call-func/scripts/throw/wrong-arg-type.brs", async () => {
+        await execute([resourceFile("throw", "wrong-arg-type.brs")], outputStreams);
+
+        expect(
+            allArgs(outputStreams.stderr.write)
+                .filter((arg) => arg !== "\n")
+                .map((arg) => arg.trim())
+        ).toEqual([
+            expect.stringContaining(
+                `test/e2e/resources/components/call-func/scripts/throw/wrong-arg-type.brs(5,4-46): Provided arguments don't match stronglyTyped's signature.
+    function stronglyTyped(arg1 as String) as String:
+        * Argument 'arg1' must be of type String, but received Object.`
+            ),
+        ]);
+    });
+
+    test("components/call-func/scripts/throw/no-args.brs", async () => {
+        await execute([resourceFile("throw", "no-args.brs")], outputStreams);
+
+        expect(allArgs(outputStreams.stderr.write).filter((arg) => arg !== "\n")).toEqual([
+            expect.stringContaining(
+                `test/e2e/resources/components/call-func/scripts/throw/no-args.brs(5,4-34): Provided arguments don't match stronglyTyped's signature.
+    function stronglyTyped(arg1 as String) as String:
+        * stronglyTyped requires at least 1 arguments, but received 0.`
+            ),
+        ]);
+    });
 });
diff --git a/test/e2e/resources/components/call-func/CallFuncTestbed.xml b/test/e2e/resources/components/call-func/CallFuncTestbed.xml
index 2b6b2ae..ec19fff 100644
--- a/test/e2e/resources/components/call-func/CallFuncTestbed.xml
+++ b/test/e2e/resources/components/call-func/CallFuncTestbed.xml
@@ -7,5 +7,6 @@
         <function name="returnValFuncOneArg" />
         <function name="overridenParentFunc" />
         <function name="testObserve" />
+        <function name="stronglyTyped" />
     </interface>
 </component>
diff --git a/test/e2e/resources/components/call-func/scripts/CallFuncTestbed.brs b/test/e2e/resources/components/call-func/scripts/CallFuncTestbed.brs
index cd02956..56b2aeb 100644
--- a/test/e2e/resources/components/call-func/scripts/CallFuncTestbed.brs
+++ b/test/e2e/resources/components/call-func/scripts/CallFuncTestbed.brs
@@ -44,3 +44,7 @@ end sub
 sub onObserveMeChanged(event as object)
     print "callFunc can trigger observeField"
 end sub
+
+function stronglyTyped(arg1 as string) as string
+    return "CallFuncTestBed::stronglyTyped"
+end function
diff --git a/test/e2e/resources/components/call-func/scripts/throw/no-args.brs b/test/e2e/resources/components/call-func/scripts/throw/no-args.brs
new file mode 100644
index 0000000..3f69440
--- /dev/null
+++ b/test/e2e/resources/components/call-func/scripts/throw/no-args.brs
@@ -0,0 +1,6 @@
+sub main()
+    node = createObject("RoSGNode", "CallFuncTestbed")
+
+    ' call a function that requires typed arguments with _no_ arguments
+    node.callFunc("stronglyTyped")
+end sub
diff --git a/test/e2e/resources/components/call-func/scripts/throw/wrong-arg-type.brs b/test/e2e/resources/components/call-func/scripts/throw/wrong-arg-type.brs
new file mode 100644
index 0000000..028eb07
--- /dev/null
+++ b/test/e2e/resources/components/call-func/scripts/throw/wrong-arg-type.brs
@@ -0,0 +1,6 @@
+sub main()
+    node = createObject("RoSGNode", "CallFuncTestbed")
+
+    ' call a function that requires typed arguments with the wrong type
+    node.callFunc("stronglyTyped", { a: "b" })
+end sub