Repository: rokucommunity/brighterscript
Pull request #1305 (2024-09-26 12:23:43)

Support omitting exception variable in bs files and convert exception var to an Expression
Our current `CatchStatement` stores the exception variable as a `Token`. This PR converts the `CatchStatement.exceptionVariable` property to an `Expression`, which supports `VariableExpression` and `TypeCastExpression`. This should more closely align with how TypeScript supports `catch(e as <any type you want here>)`. This was a breaking change to the AST, so it needed to be done as part of v1.0.0

Also supports completely omitting the exception variable when you don't need to use it. 

```BrighterScript
function init()
    try
        somethingDangerous()
    catch
        print "I live on the edge"
    end try
end function
```

transpiles to

```BrighterScript
function init()
    try
        somethingDangerous()
    catch e ' or __bsc_error if there's a local variable conflict
        print "I live on the edge"
    end try
end function
```


Patch:
diff --git a/docs/exceptions.md b/docs/exceptions.md
new file mode 100644
index 00000000..1a9bb565
--- /dev/null
+++ b/docs/exceptions.md
@@ -0,0 +1,51 @@
+# Exceptions
+## Omitting the exception variable
+### Standard use case
+There are times where you don't need to use the exception variable in a try/catch. BrighterScript allows you to omit the variable. At transpile time, we'll inject a variable so that your output is still valid brightscript
+
+```BrighterScript
+function init()
+    try
+        somethingDangerous()
+    catch
+        print "I live on the edge"
+    end try
+end function
+```
+
+transpiles to
+
+```BrighterScript
+function init()
+    try
+        somethingDangerous()
+    catch e
+        print "I live on the edge"
+    end try
+end function
+```
+
+### Exception variable name collision
+By default, bsc will name the omitted variable `e`. However, if you've already got a variable named `e` in your function body, we'll get out of your way and name it `__bsc_error` instead.
+
+```BrighterScript
+function init()
+    try
+        somethingDangerous()
+    catch
+        print "I live on the edge"
+    end try
+end function
+```
+
+transpiles to
+
+```BrighterScript
+function init()
+    try
+        somethingDangerous()
+    catch __bsc_error
+        print "I live on the edge"
+    end try
+end function
+```
diff --git a/docs/readme.md b/docs/readme.md
index ad6ffd3b..5670c0ff 100644
--- a/docs/readme.md
+++ b/docs/readme.md
@@ -50,6 +50,15 @@ enum RemoteButton
 end enum
 ```
 
+## [Exceptions](exceptions.md)
+```brighterscript
+try
+    somethingDangerous()
+catch 'look, no exception variable!
+    print "Living on the edge!"
+end try
+```
+
 ## [Namespaces](namespaces.md)
 ```brighterscript
 namespace util
@@ -133,4 +142,4 @@ end sub
 ```
 
 ## [Variable Shadowing](variable-shadowing.md)
-Name resolution rules for various types of shadowing.
\ No newline at end of file
+Name resolution rules for various types of shadowing.
diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts
index 43d089dd..7319b852 100644
--- a/src/DiagnosticMessages.ts
+++ b/src/DiagnosticMessages.ts
@@ -612,8 +612,8 @@ export let DiagnosticMessages = {
         code: 1116,
         severity: DiagnosticSeverity.Error
     }),
-    missingExceptionVarToFollowCatch: () => ({
-        message: `Missing exception variable after 'catch' keyword`,
+    expectedExceptionVarToFollowCatch: () => ({
+        message: `Expected exception variable after 'catch' keyword`,
         code: 1117,
         severity: DiagnosticSeverity.Error
     }),
diff --git a/src/astUtils/reflection.spec.ts b/src/astUtils/reflection.spec.ts
index eb5b8f6b..f20bc9b8 100644
--- a/src/astUtils/reflection.spec.ts
+++ b/src/astUtils/reflection.spec.ts
@@ -60,7 +60,7 @@ describe('reflection', () => {
         const namespace = new NamespaceStatement({ namespace: token, nameExpression: createVariableExpression('a'), body: body, endNamespace: token });
         const cls = new ClassStatement({ class: token, name: ident, body: [], endClass: token });
         const imports = new ImportStatement({ import: token, path: token });
-        const catchStmt = new CatchStatement({ catch: token, exceptionVariable: ident, catchBranch: block });
+        const catchStmt = new CatchStatement({ catch: token, exceptionVariableExpression: createVariableExpression('e'), catchBranch: block });
         const tryCatch = new TryCatchStatement({ try: token, tryBranch: block, catchStatement: catchStmt });
         const throwSt = new ThrowStatement({ throw: createToken(TokenKind.Throw) });
 
diff --git a/src/bscPlugin/validation/BrsFileValidator.spec.ts b/src/bscPlugin/validation/BrsFileValidator.spec.ts
index cbb16bb0..4eeebcdf 100644
--- a/src/bscPlugin/validation/BrsFileValidator.spec.ts
+++ b/src/bscPlugin/validation/BrsFileValidator.spec.ts
@@ -1211,7 +1211,47 @@ describe('BrsFileValidator', () => {
                 const varType = returnStmt.getSymbolTable().getSymbolType('someDate', { flags: SymbolTypeFlag.runtime, data: data });
                 expectTypeToBe(varType, StringType);
             });
+        });
+    });
+
+    describe('try/catch', () => {
+        it('allows omitting the exception variable in standard brightscript mode', () => {
+            program.setFile('source/main.brs', `
+                sub new()
+                    try
+                        print "hello"
+                    catch
+                        print "error"
+                    end try
+                end sub
+            `);
+            expectZeroDiagnostics(program);
+        });
 
+        it('shows diagnostic when omitting the exception variable in standard brightscript mode', () => {
+            program.setFile('source/main.brs', `
+                sub new()
+                    try
+                        print "hello"
+                    catch
+                        print "error"
+                    end try
+                end sub
+            `);
+            expectDiagnostics(program, []);
+        });
+
+        it('shows diagnostics when using  when omitting the exception variable in standard brightscript mode', () => {
+            program.setFile('source/main.brs', `
+                sub new()
+                    try
+                        print "hello"
+                    catch
+                        print "error"
+                    end try
+                end sub
+            `);
+            expectDiagnostics(program, []);
         });
     });
 });
diff --git a/src/bscPlugin/validation/BrsFileValidator.ts b/src/bscPlugin/validation/BrsFileValidator.ts
index 6d7ead3a..81e0bd27 100644
--- a/src/bscPlugin/validation/BrsFileValidator.ts
+++ b/src/bscPlugin/validation/BrsFileValidator.ts
@@ -1,4 +1,4 @@
-import { isAliasStatement, isArrayType, isBlock, isBody, isClassStatement, isConditionalCompileConstStatement, isConditionalCompileErrorStatement, isConditionalCompileStatement, isConstStatement, isDottedGetExpression, isDottedSetStatement, isEnumStatement, isForEachStatement, isForStatement, isFunctionExpression, isFunctionStatement, isImportStatement, isIndexedGetExpression, isIndexedSetStatement, isInterfaceStatement, isLibraryStatement, isLiteralExpression, isMethodStatement, isNamespaceStatement, isTypecastStatement, isUnaryExpression, isVariableExpression, isWhileStatement } from '../../astUtils/reflection';
+import { isAliasStatement, isArrayType, isBlock, isBody, isClassStatement, isConditionalCompileConstStatement, isConditionalCompileErrorStatement, isConditionalCompileStatement, isConstStatement, isDottedGetExpression, isDottedSetStatement, isEnumStatement, isForEachStatement, isForStatement, isFunctionExpression, isFunctionStatement, isImportStatement, isIndexedGetExpression, isIndexedSetStatement, isInterfaceStatement, isLibraryStatement, isLiteralExpression, isMethodStatement, isNamespaceStatement, isTypecastExpression, isTypecastStatement, isUnaryExpression, isVariableExpression, isWhileStatement } from '../../astUtils/reflection';
 import { createVisitor, WalkMode } from '../../astUtils/visitors';
 import { DiagnosticMessages } from '../../DiagnosticMessages';
 import type { BrsFile } from '../../files/BrsFile';
@@ -54,7 +54,7 @@ export class BrsFileValidator {
      * Walk the full AST
      */
     private walk() {
-
+        const isBrighterscript = this.event.file.parser.options.mode === ParseMode.BrighterScript;
 
         const visitor = createVisitor({
             MethodStatement: (node) => {
@@ -199,7 +199,35 @@ export class BrsFileValidator {
                 node.parent.getSymbolTable().addSymbol(node.tokens.name.text, { definingNode: node, isInstance: true }, nodeType, SymbolTypeFlag.runtime);
             },
             CatchStatement: (node) => {
-                node.parent.getSymbolTable().addSymbol(node.tokens.exceptionVariable.text, { definingNode: node, isInstance: true }, DynamicType.instance, SymbolTypeFlag.runtime);
+                //brs and bs both support variableExpression for the exception variable
+                if (isVariableExpression(node.exceptionVariableExpression)) {
+                    node.parent.getSymbolTable().addSymbol(
+                        node.exceptionVariableExpression.getName(),
+                        { definingNode: node, isInstance: true },
+                        //TODO I think we can produce a slightly more specific type here (like an AA but with the known exception properties)
+                        DynamicType.instance,
+                        SymbolTypeFlag.runtime
+                    );
+                    //brighterscript allows catch without an exception variable
+                } else if (isBrighterscript && !node.exceptionVariableExpression) {
+                    //this is fine
+
+                    //brighterscript allows a typecast expression here
+                } else if (isBrighterscript && isTypecastExpression(node.exceptionVariableExpression) && isVariableExpression(node.exceptionVariableExpression.obj)) {
+                    node.parent.getSymbolTable().addSymbol(
+                        node.exceptionVariableExpression.obj.getName(),
+                        { definingNode: node, isInstance: true },
+                        node.exceptionVariableExpression.getType({ flags: SymbolTypeFlag.runtime }),
+                        SymbolTypeFlag.runtime
+                    );
+
+                    //no other expressions are allowed here
+                } else {
+                    this.event.program.diagnostics.register({
+                        ...DiagnosticMessages.expectedExceptionVarToFollowCatch(),
+                        location: node.exceptionVariableExpression?.location ?? node.tokens.catch?.location
+                    });
+                }
             },
             DimStatement: (node) => {
                 if (node.tokens.name) {
diff --git a/src/files/BrsFile.spec.ts b/src/files/BrsFile.spec.ts
index b1db5d7f..59ead6a5 100644
--- a/src/files/BrsFile.spec.ts
+++ b/src/files/BrsFile.spec.ts
@@ -2175,11 +2175,66 @@ describe('BrsFile', () => {
                         try
                             print m.b.c
                         catch e
-                            print e
+                            print "crash"
+                        end try
+                    end sub
+                `);
+            });
+
+            it('recognizes the exception variable', async () => {
+                await testTranspile(`
+                    sub main()
+                        try
+                            print m.b.c
+                        catch e
+                            message = e.message
+                            print message
                         end try
                     end sub
                 `);
             });
+
+            it('supports omitting the exception variable in brighterscript mode (we auto-add it at transpile time)', async () => {
+                await testTranspile(`
+                    sub new()
+                        try
+                            print "hello"
+                        catch
+                            print "error"
+                        end try
+                    end sub
+                `, `
+                    sub new()
+                        try
+                            print "hello"
+                        catch e
+                            print "error"
+                        end try
+                    end sub
+                `, undefined, 'source/main.bs');
+            });
+
+            it('uses alternate name when `e` is already a local var', async () => {
+                await testTranspile(`
+                    sub new()
+                        e = "ello love"
+                        try
+                            print "hello"
+                        catch
+                            print "error"
+                        end try
+                    end sub
+                `, `
+                    sub new()
+                        e = "ello love"
+                        try
+                            print "hello"
+                        catch __bsc_error
+                            print "error"
+                        end try
+                    end sub
+                `, undefined, 'source/main.bs');
+            });
         });
 
         describe('namespaces', () => {
diff --git a/src/parser/Parser.ts b/src/parser/Parser.ts
index e25ede19..1c3206a2 100644
--- a/src/parser/Parser.ts
+++ b/src/parser/Parser.ts
@@ -1751,17 +1751,21 @@ export class Parser {
             });
         } else {
             const catchToken = this.advance();
-            const exceptionVarToken = this.tryConsume(DiagnosticMessages.missingExceptionVarToFollowCatch(), TokenKind.Identifier, ...this.allowedLocalIdentifiers) as Identifier;
-            if (exceptionVarToken) {
-                // force it into an identifier so the AST makes some sense
-                exceptionVarToken.kind = TokenKind.Identifier;
+
+            //get the exception variable as an expression
+            let exceptionVariableExpression: Expression;
+            //if we consumed any statement separators, that means we don't have an exception variable
+            if (this.consumeStatementSeparators(true)) {
+                //no exception variable. That's fine in BrighterScript but not in brightscript. But that'll get caught by the validator later...
+            } else {
+                exceptionVariableExpression = this.expression(true);
+                this.consumeStatementSeparators();
             }
-            //ensure statement sepatator
-            this.consumeStatementSeparators();
+
             const catchBranch = this.block(TokenKind.EndTry);
             catchStmt = new CatchStatement({
                 catch: catchToken,
-                exceptionVariable: exceptionVarToken,
+                exceptionVariableExpression: exceptionVariableExpression,
                 catchBranch: catchBranch
             });
         }
diff --git a/src/parser/Statement.spec.ts b/src/parser/Statement.spec.ts
index 0edb52dd..c63ac60d 100644
--- a/src/parser/Statement.spec.ts
+++ b/src/parser/Statement.spec.ts
@@ -158,7 +158,7 @@ describe('Statement', () => {
             const namespace = new NamespaceStatement({ namespace: token, nameExpression: createVariableExpression('a'), body: body, endNamespace: token });
             const cls = new ClassStatement({ class: token, name: ident, body: [], endClass: token });
             const imports = new ImportStatement({ import: token, path: token });
-            const catchStmt = new CatchStatement({ catch: token, exceptionVariable: ident, catchBranch: block });
+            const catchStmt = new CatchStatement({ catch: token, exceptionVariableExpression: createVariableExpression('e'), catchBranch: block });
             const tryCatch = new TryCatchStatement({ try: token, tryBranch: block, catchStatement: catchStmt });
             const throwSt = new ThrowStatement({ throw: createToken(TokenKind.Throw) });
 
diff --git a/src/parser/Statement.ts b/src/parser/Statement.ts
index 12a7c0c9..d6927d72 100644
--- a/src/parser/Statement.ts
+++ b/src/parser/Statement.ts
@@ -349,7 +349,7 @@ export class Block extends Statement {
         } else if (isCatchStatement(this.parent) && isTryCatchStatement(this.parent?.parent)) {
             lastBitBefore = util.createBoundingLocation(
                 this.parent.tokens.catch,
-                this.parent.tokens.exceptionVariable
+                this.parent.exceptionVariableExpression
             );
             firstBitAfter = this.parent.parent.tokens.endTry?.location;
         }
@@ -3109,27 +3109,28 @@ export class TryCatchStatement extends Statement {
 export class CatchStatement extends Statement {
     constructor(options?: {
         catch?: Token;
-        exceptionVariable?: Identifier;
+        exceptionVariableExpression?: Expression;
         catchBranch?: Block;
     }) {
         super();
         this.tokens = {
-            catch: options?.catch,
-            exceptionVariable: options?.exceptionVariable
+            catch: options?.catch
         };
+        this.exceptionVariableExpression = options?.exceptionVariableExpression;
         this.catchBranch = options?.catchBranch;
         this.location = util.createBoundingLocation(
             this.tokens.catch,
-            this.tokens.exceptionVariable,
+            this.exceptionVariableExpression,
             this.catchBranch
         );
     }
 
     public readonly tokens: {
         readonly catch?: Token;
-        readonly exceptionVariable?: Identifier;
     };
 
+    public readonly exceptionVariableExpression?: Expression;
+
     public readonly catchBranch?: Block;
 
     public readonly kind = AstNodeKind.CatchStatement;
@@ -3140,7 +3141,12 @@ export class CatchStatement extends Statement {
         return [
             state.transpileToken(this.tokens.catch, 'catch'),
             ' ',
-            this.tokens.exceptionVariable?.text ?? 'e',
+            this.exceptionVariableExpression?.transpile(state) ?? [
+                //use the variable named `e` if it doesn't exist in this function body. otherwise use '__bsc_error' just to make sure we're out of the way
+                this.getSymbolTable()?.hasSymbol('e', SymbolTypeFlag.runtime)
+                    ? '__bsc_error'
+                    : 'e'
+            ],
             ...(this.catchBranch?.transpile(state) ?? [])
         ];
     }


Test patch:
diff --git a/src/parser/tests/statement/Throw.spec.ts b/src/parser/tests/statement/Throw.spec.ts
index ccb8bf78..e18a4b4f 100644
--- a/src/parser/tests/statement/Throw.spec.ts
+++ b/src/parser/tests/statement/Throw.spec.ts
@@ -3,15 +3,17 @@ import type { TryCatchStatement, ThrowStatement } from '../../Statement';
 import { DiagnosticMessages } from '../../../DiagnosticMessages';
 import { LiteralExpression } from '../../Expression';
 import { Parser } from '../../Parser';
+import { expectDiagnostics, expectZeroDiagnostics } from '../../../testHelpers.spec';
 
 describe('parser ThrowStatement', () => {
     it('parses properly', () => {
         const parser = Parser.parse(`
             try
                 throw "some message"
-            catch
+            catch e
             end try
         `);
+        expectZeroDiagnostics(parser);
         const throwStatement = (parser.ast.statements[0] as TryCatchStatement).tryBranch!.statements[0] as ThrowStatement;
         //the statement should still exist and have null expression
         expect(throwStatement).to.exist;
@@ -25,7 +27,9 @@ describe('parser ThrowStatement', () => {
             catch
             end try
         `);
-        expect(parser.diagnostics[0]?.message).to.eql(DiagnosticMessages.missingExceptionExpressionAfterThrowKeyword().message);
+        expectDiagnostics(parser, [
+            DiagnosticMessages.missingExceptionExpressionAfterThrowKeyword().message
+        ]);
         const throwStatement = (parser.ast.statements[0] as TryCatchStatement).tryBranch!.statements[0] as ThrowStatement;
         //the statement should still exist and have null expression
         expect(throwStatement).to.exist;
diff --git a/src/parser/tests/statement/TryCatch.spec.ts b/src/parser/tests/statement/TryCatch.spec.ts
index c74d3866..33a4eaba 100644
--- a/src/parser/tests/statement/TryCatch.spec.ts
+++ b/src/parser/tests/statement/TryCatch.spec.ts
@@ -2,7 +2,7 @@ import { expect } from '../../../chai-config.spec';
 import { Parser } from '../../Parser';
 import { TryCatchStatement } from '../../Statement';
 import { isFunctionExpression } from '../../../astUtils/reflection';
-import type { FunctionExpression } from '../../Expression';
+import type { FunctionExpression, VariableExpression } from '../../Expression';
 import { expectDiagnosticsIncludes } from '../../../testHelpers.spec';
 import { DiagnosticMessages } from '../../../DiagnosticMessages';
 
@@ -27,7 +27,7 @@ describe('parser try/catch', () => {
         expect(stmt.catchStatement).to.exist;
         const cstmt = stmt.catchStatement;
         expect(cstmt!.tokens.catch?.text).to.eql('catch');
-        expect(cstmt!.tokens.exceptionVariable!.text).to.eql('e');
+        expect((cstmt!.exceptionVariableExpression as VariableExpression).getName()).to.eql('e');
         expect(cstmt!.catchBranch).to.exist.and.ownProperty('statements').to.be.lengthOf(1);
         expect(stmt.tokens.endTry?.text).to.eql('end try');
     });
diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts
index 2bfe14a3..1d65d728 100644
--- a/src/testHelpers.spec.ts
+++ b/src/testHelpers.spec.ts
@@ -82,8 +82,10 @@ function cloneDiagnostic(actualDiagnosticInput: BsDiagnostic, expectedDiagnostic
         ['message', 'code', 'severity', 'relatedInformation']
     );
     //clone Location if available
-    if (expectedDiagnostic.location) {
-        actualDiagnostic.location = cloneObject(actualDiagnosticInput.location, expectedDiagnostic.location, ['uri', 'range']) as any;
+    if (expectedDiagnostic?.location) {
+        actualDiagnostic.location = actualDiagnosticInput.location
+            ? cloneObject(actualDiagnosticInput.location, expectedDiagnostic.location, ['uri', 'range']) as any
+            : undefined;
     }
     //deep clone relatedInformation if available
     if (actualDiagnostic.relatedInformation) {
@@ -173,7 +175,7 @@ export function expectZeroDiagnostics(arg: DiagnosticCollection) {
         for (const diagnostic of diagnostics) {
             //escape any newlines
             diagnostic.message = diagnostic.message.replace(/\r/g, '\\r').replace(/\n/g, '\\n');
-            message += `\n        • bs${diagnostic.code} "${diagnostic.message}" at ${diagnostic.location?.uri ?? ''}#(${diagnostic.location.range?.start.line}:${diagnostic.location.range?.start.character})-(${diagnostic.location.range?.end.line}:${diagnostic.location.range?.end.character})`;
+            message += `\n        • bs${diagnostic.code} "${diagnostic.message}" at ${diagnostic.location?.uri ?? ''}#(${diagnostic.location?.range?.start.line}:${diagnostic.location?.range?.start.character})-(${diagnostic.location?.range?.end.line}:${diagnostic.location?.range?.end.character})`;
             //print the line containing the error (if we can find it)srcPath
             if (arg instanceof Program) {
                 const file = arg.getFile(diagnostic.location.uri);