diff --git a/src/DiagnosticMessages.ts b/src/DiagnosticMessages.ts index 3db691f46..6419fd34c 100644 --- a/src/DiagnosticMessages.ts +++ b/src/DiagnosticMessages.ts @@ -729,6 +729,11 @@ export let DiagnosticMessages = { message: `'${typeText}' cannot be used as a type`, code: 1140, severity: DiagnosticSeverity.Error + }), + argumentTypeMismatch: (actualTypeString: string, expectedTypeString: string) => ({ + message: `Argument of type '${actualTypeString}' is not compatible with parameter of type '${expectedTypeString}'`, + code: 1141, + severity: DiagnosticSeverity.Error }) }; diff --git a/src/Scope.ts b/src/Scope.ts index 4653b23c1..454b5bd85 100644 --- a/src/Scope.ts +++ b/src/Scope.ts @@ -752,7 +752,10 @@ export class Scope { //do many per-file checks this.enumerateBrsFiles((file) => { - this.diagnosticDetectFunctionCallsWithWrongParamCount(file, callableContainerMap); + if (!this.program.options.enableTypeValidation) { + //TODO: this is replaced by ScopeValidator.validateFunctionCalls() when enableTypeValidation is true + this.diagnosticDetectFunctionCallsWithWrongParamCount(file, callableContainerMap); + } this.diagnosticDetectShadowedLocalVars(file, callableContainerMap); this.diagnosticDetectFunctionCollisions(file); this.detectVariableNamespaceCollisions(file); diff --git a/src/bscPlugin/validation/BrsFileValidator.ts b/src/bscPlugin/validation/BrsFileValidator.ts index 1edb8db1a..18be4ac6d 100644 --- a/src/bscPlugin/validation/BrsFileValidator.ts +++ b/src/bscPlugin/validation/BrsFileValidator.ts @@ -48,8 +48,15 @@ export class BrsFileValidator { const visitor = createVisitor({ MethodStatement: (node) => { //add the `super` symbol to class methods - //Todo: get the actual type of the parent class - node.func.body.symbolTable.addSymbol('super', undefined, DynamicType.instance, SymbolTypeFlag.runtime); + let superType: BscType = DynamicType.instance; + if (isClassStatement(node.parent) && node.parent.hasParentClass()) { + if (this.event.program.options.enableTypeValidation) { + const parentClassType = node.parent.parentClassName.getType({ flags: SymbolTypeFlag.typetime }); + const methodName = node.getName(ParseMode.BrighterScript); + superType = parentClassType.getMemberType(methodName, { flags: SymbolTypeFlag.runtime }); + } + node.func.body.symbolTable.addSymbol('super', undefined, superType, SymbolTypeFlag.runtime); + } }, CallfuncExpression: (node) => { if (node.args.length > 5) { @@ -98,7 +105,7 @@ export class BrsFileValidator { }, FunctionStatement: (node) => { this.validateDeclarationLocations(node, 'function', () => util.createBoundingRange(node.func.functionType, node.name)); - const funcType = node.getType({ flags: SymbolTypeFlag.typetime }); + const funcType = this.getTypeFromNode(node, { flags: SymbolTypeFlag.typetime }); if (node.name?.text) { node.parent.getSymbolTable().addSymbol( @@ -137,7 +144,7 @@ export class BrsFileValidator { FunctionParameterExpression: (node) => { const paramName = node.name?.text; const symbolTable = node.getSymbolTable(); - const nodeType = node.getType({ flags: SymbolTypeFlag.typetime }); + const nodeType = this.getTypeFromNode(node, { flags: SymbolTypeFlag.typetime }); symbolTable?.addSymbol(paramName, node.name.range, nodeType, SymbolTypeFlag.runtime); }, InterfaceStatement: (node) => { diff --git a/src/bscPlugin/validation/ScopeValidator.spec.ts b/src/bscPlugin/validation/ScopeValidator.spec.ts new file mode 100644 index 000000000..d622e78a1 --- /dev/null +++ b/src/bscPlugin/validation/ScopeValidator.spec.ts @@ -0,0 +1,509 @@ +import * as sinonImport from 'sinon'; +import { DiagnosticMessages } from '../../DiagnosticMessages'; +import { Program } from '../../Program'; +import { expectDiagnostics, expectZeroDiagnostics } from '../../testHelpers.spec'; +import { expect } from 'chai'; + +describe('ScopeValidator', () => { + + let sinon = sinonImport.createSandbox(); + let rootDir = process.cwd(); + let program: Program; + beforeEach(() => { + program = new Program({ + rootDir: rootDir + }); + program.createSourceScope(); + }); + afterEach(() => { + sinon.restore(); + program.dispose(); + }); + + describe('function call validation with enableTypeValidation', () => { + + beforeEach(() => { + program.options.enableTypeValidation = true; + }); + + describe('mismatchArgumentCount', () => { + it('detects calling functions with too many arguments', () => { + program.setFile('source/file.brs', ` + sub a() + end sub + sub b() + a(1) + end sub + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.mismatchArgumentCount(0, 1).message + ]); + }); + + it('detects calling class constructors with too many arguments', () => { + program.setFile('source/main.bs', ` + function noop0() + end function + + function noop1(p1) + end function + + sub main() + noop0(1) + noop1(1,2) + noop1() + end sub + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.mismatchArgumentCount(0, 1), + DiagnosticMessages.mismatchArgumentCount(1, 2), + DiagnosticMessages.mismatchArgumentCount(1, 0) + ]); + }); + + it('detects calling functions with too few arguments', () => { + program.setFile('source/file.brs', ` + sub a(name) + end sub + sub b() + a() + end sub + `); + program.validate(); + expectDiagnostics(program, [ + DiagnosticMessages.mismatchArgumentCount(1, 0) + ]); + }); + + it('allows skipping optional parameter', () => { + program.setFile('source/file.brs', ` + sub a(name="Bob") + end sub + sub b() + a() + end sub + `); + program.validate(); + //should have an error + expectZeroDiagnostics(program); + }); + + it('shows expected parameter range in error message', () => { + program.setFile('source/file.brs', ` + sub a(age, name="Bob") + end sub + sub b() + a() + end sub + `); + program.validate(); + //should have an error + expectDiagnostics(program, [ + DiagnosticMessages.mismatchArgumentCount('1-2', 0) + ]); + }); + + it('handles expressions as arguments to a function', () => { + program.setFile('source/file.brs', ` + sub a(age, name="Bob") + end sub + sub b() + a("cat" + "dog" + "mouse") + end sub + `); + program.validate(); + expectZeroDiagnostics(program); + }); + + it('Catches extra arguments for expressions as arguments to a function', () => { + program.setFile('source/file.brs', ` + sub a(age) + end sub + sub b() + a(m.lib.movies[0], 1) + end sub + `); + program.validate(); + //should have an error + expectDiagnostics(program, [ + DiagnosticMessages.mismatchArgumentCount(1, 2) + ]); + }); + }); + + describe('argumentTypeMismatch', () => { + it('Catches argument type mismatches on function calls', () => { + program.setFile('source/file.brs', ` + sub a(age as integer) + end sub + sub b() + a("hello") + end sub + `); + program.validate(); + //should have an error + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('string', 'integer').message + ); + }); + + it('Catches argument type mismatches on function calls for functions defined in another file', () => { + program.setFile('source/file.brs', ` + sub a(age as integer) + end sub + `); + program.setFile('source/file2.brs', ` + sub b() + a("hello") + foo = "foo" + a(foo) + end sub + `); + program.validate(); + //should have an error + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('string', 'integer').message + ); + }); + + it('catches argument type mismatches on function calls within namespaces', () => { + program.setFile('source/file.bs', ` + namespace Name.Space + sub a(param as integer) + print param + end sub + + sub b() + a("hello") + foo = "foo" + a(foo) + end sub + end namespace + `); + program.validate(); + //should have an error + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('string', 'integer').message + ); + }); + + it('catches argument type mismatches on function calls as arguments', () => { + program.setFile('source/file1.bs', ` + sub a(param as string) + print param + end sub + + function getNum() as integer + return 1 + end function + + sub b() + a(getNum()) + end sub + `); + program.validate(); + //should have an error + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('integer', 'string').message + ); + }); + + + it('catches argument type mismatches on function calls within namespaces across files', () => { + program.setFile('source/file1.bs', ` + namespace Name.Space + function getNum() as integer + return 1 + end function + + function getStr() as string + return "hello" + end function + end namespace + `); + program.setFile('source/file2.bs', ` + namespace Name.Space + sub needsInt(param as integer) + print param + end sub + + sub someFunc() + needsInt(getStr()) + needsInt(getNum()) + end sub + end namespace + `); + program.validate(); + //should have an error + expect(program.getDiagnostics().length).to.equal(1); + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('string', 'integer').message + ); + }); + + it('correctly validates correct parameters that are class members', () => { + program.setFile('source/main.bs', ` + class PiHolder + pi = 3.14 + function getPi() as float + return m.pi + end function + end class + + sub takesFloat(fl as float) + end sub + + sub someFunc() + holder = new PiHolder() + takesFloat(holder.pi) + takesFloat(holder.getPI()) + end sub`); + program.validate(); + //should have no error + expectZeroDiagnostics(program); + }); + + it('correctly validates wrong parameters that are class members', () => { + program.setFile('source/main.bs', ` + class PiHolder + pi = 3.14 + name = "hello" + function getPi() as float + return m.pi + end function + end class + + sub takesFloat(fl as float) + end sub + + sub someFunc() + holder = new PiHolder() + takesFloat(holder.name) + takesFloat(Str(holder.getPI())) + end sub`); + program.validate(); + //should have error: holder.name is string + expect(program.getDiagnostics().length).to.equal(2); + expect(program.getDiagnostics().map(x => x.message)).to.include( + DiagnosticMessages.argumentTypeMismatch('string', 'float').message + ); + }); + + it('correctly validates correct parameters that are interface members', () => { + program.setFile('source/main.bs', ` + interface IPerson + height as float + name as string + function getWeight() as float + function getAddress() as string + end interface + + sub takesFloat(fl as float) + end sub + + sub someFunc(person as IPerson) + takesFloat(person.height) + takesFloat(person.getWeight()) + end sub`); + program.validate(); + //should have no error + expectZeroDiagnostics(program); + }); + + it('correctly validates wrong parameters that are interface members', () => { + program.setFile('source/main.bs', ` + interface IPerson + isAlive as boolean + function getAddress() as string + end interface + + sub takesFloat(fl as float) + end sub + + sub someFunc(person as IPerson) + takesFloat(person.isAlive) + takesFloat(person.getAddress()) + end sub + `); + program.validate(); + //should have 2 errors: person.name is string (not float) and person.getAddress() is object (not float) + expectDiagnostics(program, [ + DiagnosticMessages.argumentTypeMismatch('boolean', 'float').message, + DiagnosticMessages.argumentTypeMismatch('string', 'float').message + ]); + }); + + it('`as object` param allows all types', () => { + program.setFile('source/main.bs', ` + sub takesObject(obj as Object) + end sub + + sub main() + takesObject(true) + takesObject(1) + takesObject(1.2) + takesObject(1.2#) + takesObject("text") + takesObject({}) + takesObject([]) + end sub + `); + program.validate(); + expectZeroDiagnostics(program); + }); + + it('allows conversions for arguments', () => { + program.setFile('source/main.bs', ` + sub takesFloat(fl as float) + end sub + + sub someFunc() + takesFloat(1) + end sub`); + program.validate(); + //should have no error + expectZeroDiagnostics(program); + }); + + it('allows subclasses as arguments', () => { + program.setFile('source/main.bs', ` + + class Animal + end class + + class Dog extends Animal + end class + + class Retriever extends Dog + end class + + class Lab extends Retriever + end class + + sub takesAnimal(thing as Animal) + end sub + + sub someFunc() + fido = new Lab() + takesAnimal(fido) + end sub`); + program.validate(); + //should have no error + expectZeroDiagnostics(program); + }); + + it('allows subclasses from namespaces as arguments', () => { + program.setFile('source/main.bs', ` + + class Outside + end class + + class ChildOutExtendsInside extends NS.Inside + end class + + namespace NS + class Inside + end class + + class ChildInExtendsOutside extends Outside + end class + + class ChildInExtendsInside extends Inside + sub methodTakesInside(i as Inside) + end sub + end class + + sub takesInside(klass as Inside) + end sub + + sub testFuncInNamespace() + takesOutside(new Outside()) + takesOutside(new NS.ChildInExtendsOutside()) + + ' These call NS.takesInside + takesInside(new NS.Inside()) + takesInside(new Inside()) + takesInside(new NS.ChildInExtendsInside()) + takesInside(new ChildInExtendsInside()) + takesInside(new ChildOutExtendsInside()) + + child = new ChildInExtendsInside() + child.methodTakesInside(new Inside()) + child.methodTakesInside(new ChildInExtendsInside()) + child.methodTakesInside(new ChildOutExtendsInside()) + end sub + + end namespace + + sub takesOutside(klass as Outside) + end sub + + sub takesInside(klass as NS.Inside) + end sub + + sub testFunc() + takesOutside(new Outside()) + takesOutside(new NS.ChildInExtendsOutside()) + + takesInside(new NS.Inside()) + takesInside(new NS.ChildInExtendsInside()) + takesInside(new ChildOutExtendsInside()) + + NS.takesInside(new NS.Inside()) + NS.takesInside(new NS.ChildInExtendsInside()) + NS.takesInside(new ChildOutExtendsInside()) + + child = new NS.ChildInExtendsInside() + child.methodTakesInside(new NS.Inside()) + child.methodTakesInside(new NS.ChildInExtendsInside()) + child.methodTakesInside(new ChildOutExtendsInside()) + end sub`); + program.validate(); + //should have no error + expectZeroDiagnostics(program); + }); + + it('respects union types', () => { + program.setFile('source/main.bs', ` + sub takesStringOrKlass(p as string or Klass) + end sub + + class Klass + end class + + sub someFunc() + myKlass = new Klass() + takesStringOrKlass("test") + takesStringOrKlass(myKlass) + takesStringOrKlass(1) + end sub`); + program.validate(); + //should have error when passed an integer + expect(program.getDiagnostics().length).to.equal(1); + expectDiagnostics(program, [ + DiagnosticMessages.argumentTypeMismatch('integer', 'string or Klass').message + ]); + }); + + + it('validates functions assigned to variables', () => { + program.setFile('source/main.bs', ` + sub someFunc() + myFunc = function(i as integer, s as string) + print i+1 + print s.len() + end function + myFunc("hello", 2) + end sub`); + program.validate(); + //should have error when passed incorrect types + expectDiagnostics(program, [ + DiagnosticMessages.argumentTypeMismatch('string', 'integer').message, + DiagnosticMessages.argumentTypeMismatch('integer', 'string').message + ]); + }); + }); + }); +}); diff --git a/src/bscPlugin/validation/ScopeValidator.ts b/src/bscPlugin/validation/ScopeValidator.ts index 77674f2f2..c3b78ab05 100644 --- a/src/bscPlugin/validation/ScopeValidator.ts +++ b/src/bscPlugin/validation/ScopeValidator.ts @@ -1,5 +1,5 @@ import { URI } from 'vscode-uri'; -import { isBinaryExpression, isBrsFile, isLiteralExpression, isNamespaceStatement, isTypeExpression, isXmlScope } from '../../astUtils/reflection'; +import { isBinaryExpression, isBrsFile, isFunctionType, isLiteralExpression, isNamespaceStatement, isTypeExpression, isXmlScope } from '../../astUtils/reflection'; import { Cache } from '../../Cache'; import { DiagnosticMessages } from '../../DiagnosticMessages'; import type { BrsFile } from '../../files/BrsFile'; @@ -52,6 +52,9 @@ export class ScopeValidator { if (isBrsFile(file)) { this.iterateFileExpressions(file); this.validateCreateObjectCalls(file); + if (this.event.program.options.enableTypeValidation) { + this.validateFunctionCalls(file); + } } }); } @@ -392,6 +395,63 @@ export class ScopeValidator { this.event.scope.addDiagnostics(diagnostics); } + /** + * Detect calls to functions with the incorrect number of parameters, or wrong types of arguments + */ + private validateFunctionCalls(file: BscFile) { + const diagnostics: BsDiagnostic[] = []; + + //validate all function calls + for (let expCall of file.functionCalls) { + const funcType = expCall.expression?.callee?.getType({ flags: SymbolTypeFlag.runtime }); + if (funcType?.isResolvable() && isFunctionType(funcType)) { + funcType.setName(expCall.name); + + //get min/max parameter count for callable + let minParams = 0; + let maxParams = 0; + for (let param of funcType.params) { + maxParams++; + //optional parameters must come last, so we can assume that minParams won't increase once we hit + //the first isOptional + if (param.isOptional !== true) { + minParams++; + } + } + let expCallArgCount = expCall.args.length; + if (expCall.args.length > maxParams || expCall.args.length < minParams) { + let minMaxParamsText = minParams === maxParams ? maxParams : `${minParams}-${maxParams}`; + diagnostics.push({ + ...DiagnosticMessages.mismatchArgumentCount(minMaxParamsText, expCallArgCount), + range: expCall.nameRange, + //TODO detect end of expression call + file: file + }); + } + let paramIndex = 0; + for (let arg of expCall.args) { + const argType = arg.expression.getType({ flags: SymbolTypeFlag.runtime }); + + const paramType = funcType.params[paramIndex]?.type; + if (!paramType) { + // unable to find a paramType -- maybe there are more args than params + break; + } + if (!paramType?.isTypeCompatible(argType)) { + diagnostics.push({ + ...DiagnosticMessages.argumentTypeMismatch(argType.toString(), paramType.toString()), + range: arg.expression.range, + //TODO detect end of expression call + file: file + }); + } + paramIndex++; + } + } + } + this.event.scope.addDiagnostics(diagnostics); + } + /** * Adds a diagnostic to the first scope for this key. Prevents duplicate diagnostics * for diagnostics where scope isn't important. (i.e. CreateObject validations) diff --git a/src/files/BrsFile.ts b/src/files/BrsFile.ts index 901e3918c..807544ed4 100644 --- a/src/files/BrsFile.ts +++ b/src/files/BrsFile.ts @@ -717,7 +717,8 @@ export class BrsFile { name: functionName, nameRange: util.createRange(callee.range.start.line, columnIndexBegin, callee.range.start.line, columnIndexEnd), //TODO keep track of parameters - args: args + args: args, + expression: expression }; this.functionCalls.push(functionCall); } diff --git a/src/interfaces.ts b/src/interfaces.ts index a5b3f4d00..56b9811b2 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -15,6 +15,7 @@ import type { BscType } from './types/BscType'; import type { AstEditor } from './astUtils/AstEditor'; import type { Token } from './lexer/Token'; import type { SymbolTypeFlag } from './SymbolTable'; +import type { CallExpression } from './parser/Expression'; export interface BsDiagnostic extends Diagnostic { file: BscFile; @@ -68,6 +69,7 @@ export interface FunctionCall { * The full range of this function call (from the start of the function name to its closing paren) */ range: Range; + expression: CallExpression; functionScope: FunctionScope; file: File; name: string; diff --git a/src/types/ObjectType.ts b/src/types/ObjectType.ts index 989dc6e38..875aec41d 100644 --- a/src/types/ObjectType.ts +++ b/src/types/ObjectType.ts @@ -1,5 +1,5 @@ import { SymbolTypeFlag } from '../SymbolTable'; -import { isDynamicType, isInheritableType, isObjectType, isUnionType } from '../astUtils/reflection'; +import { isObjectType } from '../astUtils/reflection'; import type { GetTypeOptions } from '../interfaces'; import { BscType } from './BscType'; import { BscTypeKind } from './BscTypeKind'; @@ -15,15 +15,8 @@ export class ObjectType extends BscType { public readonly kind = BscTypeKind.ObjectType; public isTypeCompatible(targetType: BscType) { - if (isUnionType(targetType)) { - return targetType.checkAllMemberTypes((type) => type.isTypeCompatible(this)); - } else if (isObjectType(targetType) || - isDynamicType(targetType) || - isInheritableType(targetType) - ) { - return true; - } - return false; + //Brightscript allows anything passed "as object", so as long as a type is provided, this is true + return !!targetType; } public toString() {