Merge pull request #247 from rokucommunity/function-declaration-name-…

…validation

flag functions with keyword names.

src/lexer/TokenKind.ts

@@ -132,6 +132,10 @@ export enum TokenKind {
     True = 'True',
     Type = 'Type',
     While = 'While',
+    Try = 'Try',
+    Catch = 'Catch',
+    EndTry = 'EndTry',
+    Throw = 'Throw',
 
     //misc
     Library = 'Library',
@@ -211,6 +215,7 @@ export const ReservedWords = new Set([
     'sub',
     'tab',
     'then',
+    'throw',
     'to',
     'true',
     'type',
@@ -297,7 +302,10 @@ export const Keywords: Record<string, TokenKind> = {
     'source_function_name': TokenKind.SourceFunctionNameLiteral,
     'source_location': TokenKind.SourceLocationLiteral,
     'pkg_path': TokenKind.PkgPathLiteral,
-    'pkg_location': TokenKind.PkgLocationLiteral
+    'pkg_location': TokenKind.PkgLocationLiteral,
+    try: TokenKind.Try,
+    catch: TokenKind.Catch,
+    endtry: TokenKind.EndTry
 };
 //hide the constructor prototype method because it causes issues
 Keywords.constructor = undefined;
@@ -412,7 +420,11 @@ export const AllowedProperties = [
     TokenKind.SourceFunctionNameLiteral,
     TokenKind.SourceLocationLiteral,
     TokenKind.PkgPathLiteral,
-    TokenKind.PkgLocationLiteral
+    TokenKind.PkgLocationLiteral,
+    TokenKind.Try,
+    TokenKind.Catch,
+    TokenKind.EndTry,
+    TokenKind.Throw
 ];
 
 /** List of TokenKind that are allowed as local var identifiers. */
@@ -441,7 +453,10 @@ export const AllowedLocalIdentifiers = [
     TokenKind.Override,
     TokenKind.Namespace,
     TokenKind.EndNamespace,
-    TokenKind.Import
+    TokenKind.Import,
+    TokenKind.Try,
+    TokenKind.Catch,
+    TokenKind.EndTry
 ];
 
 export const BrighterScriptSourceLiterals = [
@@ -508,10 +523,59 @@ export const DisallowedLocalIdentifiers = [
     TokenKind.SourceFunctionNameLiteral,
     TokenKind.SourceLocationLiteral,
     TokenKind.PkgPathLiteral,
-    TokenKind.PkgLocationLiteral
+    TokenKind.PkgLocationLiteral,
+    TokenKind.Throw
 ];
 
 export const DisallowedLocalIdentifiersText = new Set([
     'run',
     ...DisallowedLocalIdentifiers.map(x => x.toLowerCase())
 ]);
+
+/**
+ * List of string versions of TokenKind and various globals that are NOT allowed as scope function names.
+ * Used to throw more helpful "you can't use a reserved word as a function name" errors.
+ */
+export const DisallowedFunctionIdentifiers = [
+    TokenKind.And,
+    TokenKind.CreateObject,
+    TokenKind.Dim,
+    TokenKind.Each,
+    TokenKind.Else,
+    TokenKind.ElseIf,
+    TokenKind.End,
+    TokenKind.EndFunction,
+    TokenKind.EndIf,
+    TokenKind.EndSub,
+    TokenKind.EndWhile,
+    TokenKind.Exit,
+    TokenKind.ExitWhile,
+    TokenKind.False,
+    TokenKind.For,
+    TokenKind.Function,
+    TokenKind.Goto,
+    TokenKind.If,
+    TokenKind.Invalid,
+    TokenKind.Let,
+    TokenKind.Next,
+    TokenKind.Not,
+    TokenKind.ObjFun,
+    TokenKind.Or,
+    TokenKind.Print,
+    TokenKind.Rem,
+    TokenKind.Return,
+    TokenKind.Step,
+    TokenKind.Sub,
+    TokenKind.Tab,
+    TokenKind.Then,
+    TokenKind.To,
+    TokenKind.True,
+    TokenKind.Type,
+    TokenKind.While,
+    TokenKind.Throw
+];
+
+export const DisallowedFunctionIdentifiersText = new Set([
+    'run',
+    ...DisallowedFunctionIdentifiers.map(x => x.toLowerCase())
+]);

src/parser/Parser.Class.spec.ts

@@ -52,7 +52,7 @@ describe('parser class', () => {
                 end class
             `);
             let { statements, diagnostics } = Parser.parse(tokens, { mode: ParseMode.BrighterScript });
-            expect(diagnostics).to.be.lengthOf(0);
+            expect(diagnostics[0]?.message).not.to.exist;
             expect(statements[0]).instanceof(ClassStatement);
         });
     }
@@ -70,6 +70,74 @@ describe('parser class', () => {
         });
     }
 
+    it('does not allow "throw" to be defined as a local var', () => {
+        const parser = Parser.parse(`
+            sub main()
+                'not allowed to define throw as local var
+                throw = true
+            end sub
+        `);
+
+        expect(parser.diagnostics[0]?.message).to.eql(DiagnosticMessages.cannotUseReservedWordAsIdentifier('throw').message);
+    });
+
+    it('does not allow function named "throw"', () => {
+        const parser = Parser.parse(`
+            'not allowed to define a function called "throw"
+            sub throw()
+            end sub
+        `);
+
+        expect(parser.diagnostics[0]?.message).to.eql(DiagnosticMessages.cannotUseReservedWordAsIdentifier('throw').message);
+    });
+
+    it('supports the try/catch keywords in various places', () => {
+        const parser = Parser.parse(`
+            sub main()
+                'allowed to be local vars
+                try = true
+                catch = true
+                endTry = true
+                'not allowed to use throw as local variable
+                'throw = true
+
+                'allowed to be object props
+                person = {
+                try: true,
+                catch: true,
+                endTry: true,
+                throw: true
+                }
+
+                person.try = true
+                person.catch = true
+                person.endTry = true
+                person.throw = true
+
+                'allowed as object property reference
+                print person.try
+                print person.catch
+                print person.endTry
+                print person.throw
+            end sub
+
+            sub try()
+            end sub
+
+            sub catch()
+            end sub
+
+            sub endTry()
+            end sub
+
+            'not allowed to define a function called "throw"
+            ' sub throw()
+            ' end sub
+        `);
+
+        expect(parser.diagnostics[0]?.message).not.to.exist;
+    });
+
     it('parses empty class', () => {
         let { tokens } = Lexer.scan(`
                 class Person

src/parser/Parser.ts

@@ -8,6 +8,7 @@ import {
     AllowedLocalIdentifiers,
     AssignmentOperators,
     DisallowedLocalIdentifiersText,
+    DisallowedFunctionIdentifiersText,
     AllowedProperties,
     Lexer,
     BrighterScriptSourceLiterals,
@@ -404,7 +405,7 @@ export class Parser {
 
                 //methods (function/sub keyword OR identifier followed by opening paren)
                 if (this.checkAny(TokenKind.Function, TokenKind.Sub) || (this.checkAny(TokenKind.Identifier, ...AllowedProperties) && this.checkNext(TokenKind.LeftParen))) {
-                    let funcDeclaration = this.functionDeclaration(false);
+                    let funcDeclaration = this.functionDeclaration(false, false);
 
                     //remove this function from the lists because it's not a callable
                     const functionStatement = this._references.functionStatements.pop();
@@ -542,9 +543,9 @@ export class Parser {
      */
     private callExpressions = [];
 
-    private functionDeclaration(isAnonymous: true): FunctionExpression;
-    private functionDeclaration(isAnonymous: false): FunctionStatement;
-    private functionDeclaration(isAnonymous: boolean) {
+    private functionDeclaration(isAnonymous: true, checkIdentifier?: boolean): FunctionExpression;
+    private functionDeclaration(isAnonymous: false, checkIdentifier?: boolean): FunctionStatement;
+    private functionDeclaration(isAnonymous: boolean, checkIdentifier = true) {
         let previousCallExpressions = this.callExpressions;
         this.callExpressions = [];
         try {
@@ -600,6 +601,14 @@ export class Parser {
                         range: name.range
                     });
                 }
+
+                //flag functions with keywords for names (only for standard functions)
+                if (checkIdentifier && DisallowedFunctionIdentifiersText.has(name.text.toLowerCase())) {
+                    this.diagnostics.push({
+                        ...DiagnosticMessages.cannotUseReservedWordAsIdentifier(name.text),
+                        range: name.range
+                    });
+                }
             }
 
             let params = [] as FunctionParameterExpression[];