Refined the stdlib function collision diagnostics.

Fixes #70.

src/DiagnosticMessages.ts

@@ -64,8 +64,8 @@ export let DiagnosticMessages = {
         code: 1010,
         severity: DiagnosticSeverity.Hint
     }),
-    localVarShadowsGlobalFunction: (localName: string, globalLocation: string) => ({
-        message: `Local var '${localName}' has same name as global function in '${globalLocation}' and will never be called.`,
+    localVarFunctionShadowsParentFunction: (scopeName: 'stdlib' | 'scope') => ({
+        message: `Local variable function has same name as ${scopeName} function and will never be called.`,
         code: 1011,
         severity: DiagnosticSeverity.Warning
     }),
@@ -534,6 +534,16 @@ export let DiagnosticMessages = {
         message: `Component script auto-import found '.bs' and '.brs' files with the same name and will import only the '.bs' file`,
         code: 1103,
         severity: DiagnosticSeverity.Warning
+    }),
+    localVarShadowedByScopedFunction: () => ({
+        message: `Local var has same name as scoped function and will not be accessible`,
+        code: 1104,
+        severity: DiagnosticSeverity.Warning
+    }),
+    scopeFunctionShadowedByBuiltInFunction: () => ({
+        message: `Scope function has same name as built-in function and will not be accessible`,
+        code: 1105,
+        severity: DiagnosticSeverity.Warning
     })
 };
 

src/Program.ts

@@ -15,6 +15,7 @@ import { platformFile } from './platformCallables';
 import { standardizePath as s, util } from './util';
 import { XmlScope } from './XmlScope';
 import { DiagnosticFilterer } from './DiagnosticFilterer';
+import { Callable } from './brsTypes';
 
 export class Program {
     constructor(

src/Scope.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai';
 import { EventEmitter } from 'events';
 import * as sinonImport from 'sinon';
-import { Position } from 'vscode-languageserver';
+import { Position, Range } from 'vscode-languageserver';
 import { standardizePath as s } from './util';
 import { Scope } from './Scope';
 import { DiagnosticMessages } from './DiagnosticMessages';
@@ -188,21 +188,123 @@ describe('Scope', () => {
             expect(program.getDiagnostics()).to.be.lengthOf(0);
         });
 
-        it('detects local functions with same name as global', async () => {
-            await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
-                sub Main()
-                    SayHi = sub()
-                        print "Hi from inner"
+        describe('function shadowing', () => {
+            it('warns when local var function has same name as stdlib function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        str = function(p)
+                            return "override"
+                        end function
+                        print str(12345) 'prints "12345" (i.e. our local function is never used)
                     end sub
-                end sub
-                sub SayHi()
-                    print "Hi from outer"
-                end sub
-            `);
-            await program.validate();
-            let diagnostics = program.getDiagnostics();
-            expect(diagnostics).to.be.lengthOf(1);
-            expect(diagnostics[0].code).to.equal(DiagnosticMessages.localVarShadowsGlobalFunction('', '').code);
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics().map(x => {
+                    return {
+                        message: x.message,
+                        range: x.range
+                    };
+                });
+                expect(diagnostics[0]).to.exist.and.to.eql({
+                    message: DiagnosticMessages.localVarFunctionShadowsParentFunction('stdlib').message,
+                    range: Range.create(2, 24, 2, 27)
+                });
+            });
+
+            it('warns when local var has same name as built-in function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        str = 12345
+                        print str ' prints "12345" (i.e. our local variable is allowed to shadow the built-in function name)
+                    end sub
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics();
+                expect(diagnostics[0]?.message).not.to.exist;
+            });
+
+            it('warns when local var has same name as built-in function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        str = 6789
+                        print str(12345) ' prints "12345" (i.e. our local variable did not override the callable global function)
+                    end sub
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics();
+                expect(diagnostics[0]?.message).not.to.exist;
+            });
+
+            it('detects local function with same name as scope function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        getHello = function()
+                            return "override"
+                        end function
+                        print getHello() 'prints "hello" (i.e. our local variable is never called)
+                    end sub
+                    
+                    function getHello()
+                        return "hello"
+                    end function
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics().map(x => {
+                    return {
+                        message: x.message,
+                        range: x.range
+                    };
+                });
+                expect(diagnostics[0]).to.exist.and.to.eql({
+                    message: DiagnosticMessages.localVarFunctionShadowsParentFunction('scope').message,
+                    range: Range.create(2, 24, 2, 32)
+                });
+            });
+
+            it('detects local function with same name as scope function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        getHello = "override"
+                        print getHello ' prints <Function: gethello> (i.e. local variable override does NOT work for same-scope-defined methods)
+                    end sub
+                    function getHello()
+                        return "hello"
+                    end function
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics().map(x => {
+                    return {
+                        message: x.message,
+                        range: x.range
+                    };
+                });
+                expect(diagnostics[0]).to.exist.and.to.eql({
+                    message: DiagnosticMessages.localVarShadowedByScopedFunction().message,
+                    range: Range.create(2, 24, 2, 32)
+                });
+            });
+
+            it('detects scope function with same name as built-in function', async () => {
+                await program.addOrReplaceFile({ src: s`${rootDir}/source/main.brs`, dest: s`source/main.brs` }, `
+                    sub main()
+                        print str(12345) ' prints 12345 (i.e. our str() function below is ignored)
+                    end sub
+                    function str(num)
+                        return "override"
+                    end function
+                `);
+                await program.validate();
+                let diagnostics = program.getDiagnostics().map(x => {
+                    return {
+                        message: x.message,
+                        range: x.range
+                    };
+                });
+                expect(diagnostics[0]).to.exist.and.to.eql({
+                    message: DiagnosticMessages.scopeFunctionShadowedByBuiltInFunction().message,
+                    range: Range.create(4, 29, 4, 32)
+                });
+            });
         });
 
         it('detects duplicate callables', async () => {

src/Scope.ts

@@ -10,6 +10,8 @@ import { BsClassValidator } from './validators/ClassValidator';
 import { NamespaceStatement, ParseMode, Statement, NewExpression, FunctionStatement } from './parser';
 import { ClassStatement } from './parser/ClassStatement';
 import { standardizePath as s, util } from './util';
+import { platformCallableMap } from './platformCallables';
+import { FunctionType } from './types/FunctionType';
 
 /**
  * A class to keep track of all declarations within a given scope (like global scope, component scope)
@@ -402,25 +404,41 @@ export class Scope {
         });
 
         //get a list of all callables, indexed by their lower case names
-        let callableContainersByLowerName = util.getCallableContainersByLowerName(callables);
+        let callableContainerMap = util.getCallableContainersByLowerName(callables);
 
         //find all duplicate function declarations
-        this.diagnosticFindDuplicateFunctionDeclarations(callableContainersByLowerName);
+        this.diagnosticFindDuplicateFunctionDeclarations(callableContainerMap);
 
         //enforce a series of checks on the bodies of class methods
         this.validateClasses();
 
         //do many per-file checks
         for (let key in this.files) {
             let scopeFile = this.files[key];
-            this.diagnosticDetectCallsToUnknownFunctions(scopeFile.file, callableContainersByLowerName);
-            this.diagnosticDetectFunctionCallsWithWrongParamCount(scopeFile.file, callableContainersByLowerName);
-            this.diagnosticDetectShadowedLocalVars(scopeFile.file, callableContainersByLowerName);
+            this.diagnosticDetectCallsToUnknownFunctions(scopeFile.file, callableContainerMap);
+            this.diagnosticDetectFunctionCallsWithWrongParamCount(scopeFile.file, callableContainerMap);
+            this.diagnosticDetectShadowedLocalVars(scopeFile.file, callableContainerMap);
+            this.diagnosticDetectFunctionCollisions(scopeFile.file);
         }
 
         this.isValidated = true;
     }
 
+    /**
+     * Find function declarations with the same name as a stdlib function
+     */
+    private diagnosticDetectFunctionCollisions(file: BrsFile | XmlFile) {
+        for (let func of file.callables) {
+            if (platformCallableMap[func.name.toLowerCase()]) {
+                this.diagnostics.push({
+                    ...DiagnosticMessages.scopeFunctionShadowedByBuiltInFunction(),
+                    range: func.nameRange,
+                    file: file
+                });
+            }
+        }
+    }
+
     public getNewExpressions() {
         let result = [] as AugmentedNewExpression[];
         for (let key in this.files) {
@@ -482,23 +500,50 @@ export class Scope {
     /**
      * Detect local variables (function scope) that have the same name as scope calls
      * @param file
-     * @param callablesByLowerName
+     * @param callableContainerMap
      */
-    private diagnosticDetectShadowedLocalVars(file: BrsFile | XmlFile, callablesByLowerName: { [lowerName: string]: CallableContainer[] }) {
+    private diagnosticDetectShadowedLocalVars(file: BrsFile | XmlFile, callableContainerMap: { [lowerName: string]: CallableContainer[] }) {
         //loop through every function scope
         for (let scope of file.functionScopes) {
             //every var declaration in this scope
             for (let varDeclaration of scope.variableDeclarations) {
-                let globalCallableContainer = callablesByLowerName[varDeclaration.name.toLowerCase()];
-                //if we found a collision
-                if (globalCallableContainer && globalCallableContainer.length > 0) {
-                    let globalCallable = globalCallableContainer[0];
+                let lowerVarName = varDeclaration.name.toLowerCase();
+
+                //if the var is a function
+                if (varDeclaration.type instanceof FunctionType) {
+                    //local var function with same name as stdlib function
+                    if (
+                        //has same name as stdlib
+                        platformCallableMap[lowerVarName]
+                    ) {
+                        this.diagnostics.push({
+                            ...DiagnosticMessages.localVarFunctionShadowsParentFunction('stdlib'),
+                            range: varDeclaration.nameRange,
+                            file: file
+                        });
 
+                        //this check needs to come after the stdlib one, because the stdlib functions are included
+                        //in the scope function list
+                    } else if (
+                        //has same name as scope function
+                        callableContainerMap[lowerVarName]
+                    ) {
+                        this.diagnostics.push({
+                            ...DiagnosticMessages.localVarFunctionShadowsParentFunction('scope'),
+                            range: varDeclaration.nameRange,
+                            file: file
+                        });
+                    }
+
+                    //var is not a function
+                } else if (
+                    //is same name as a callable
+                    callableContainerMap[lowerVarName] &&
+                    //is NOT a callable from stdlib (because non-function local vars can have same name as stdlib names)
+                    !platformCallableMap[lowerVarName]
+                ) {
                     this.diagnostics.push({
-                        ...DiagnosticMessages.localVarShadowsGlobalFunction(
-                            varDeclaration.name,
-                            globalCallable.callable.file.pkgPath
-                        ),
+                        ...DiagnosticMessages.localVarShadowedByScopedFunction(),
                         range: varDeclaration.nameRange,
                         file: file
                     });

src/platformCallables.ts

@@ -728,3 +728,12 @@ for (let callable of platformCallables) {
 
 }
 platformFile.callables = platformCallables;
+
+/**
+ * A map of all built-in function names. We use this extensively in scope validation
+ * so keep a single copy in memory to improve performance
+ */
+export const platformCallableMap = platformCallables.reduce((map, x) => {
+    map[x.name.toLowerCase()] = x;
+    return map;
+}, {});