From 7930b4bab4a1928d01be948300ee14ce3bfd781d Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Mon, 4 May 2020 06:12:27 -0400 Subject: [PATCH] Refined the stdlib function collision diagnostics. Fixes #70. --- src/DiagnosticMessages.ts | 14 +++- src/Program.ts | 1 + src/Scope.spec.ts | 132 +++++++++++++++++++++++++++++++++----- src/Scope.ts | 75 +++++++++++++++++----- src/platformCallables.ts | 9 +++ 5 files changed, 199 insertions(+), 32 deletions(-) diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts index c755cb34e..16e7810bd 100644 --- a/src/DiagnosticMessages.ts +++ b/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 }) }; diff --git a/src/Program.ts b/src/Program.ts index 502815e83..4b7cd349a 100644 --- a/src/Program.ts +++ b/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( diff --git a/src/Scope.spec.ts b/src/Scope.spec.ts index 1545c3e81..d31aa9b00 100644 --- a/src/Scope.spec.ts +++ b/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 (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 () => { diff --git a/src/Scope.ts b/src/Scope.ts index 40f67cd96..fdb08fee1 100644 --- a/src/Scope.ts +++ b/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,10 +404,10 @@ 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(); @@ -413,14 +415,30 @@ export class Scope { //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 }); diff --git a/src/platformCallables.ts b/src/platformCallables.ts index bd878a857..7d57e11a2 100644 --- a/src/platformCallables.ts +++ b/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; +}, {});