From 5692ee857190e67d55a82f35be56f7ad7f169036 Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Mon, 9 Sep 2024 16:42:14 -0300 Subject: [PATCH 1/2] Adds diagnostic for when a variables type changes --- README.md | 4 ++ src/index.ts | 2 + src/plugins/codeStyle/diagnosticMessages.ts | 6 +++ src/plugins/codeStyle/index.spec.ts | 31 +++++++++++ src/plugins/codeStyle/index.ts | 48 ++++++++++++++++- src/util.ts | 6 ++- .../source/type-reassignment-custom.bs | 51 +++++++++++++++++++ test/project1/source/type-reassignment.brs | 40 +++++++++++++++ 8 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 test/project1/source/type-reassignment-custom.bs create mode 100644 test/project1/source/type-reassignment.brs diff --git a/README.md b/README.md index 7e27449..32c7c04 100644 --- a/README.md +++ b/README.md @@ -265,6 +265,10 @@ Default rules: - `args`: enforce arguments type annotations - `off`: do not validate (**default**) +- `name-shadowing`: enforces that no two items have the same name (`error | warn | info | off`) + +- `type-reassignment`: enforces that a variable is not used to reference multiple types (`error | warn | info | off`) + ### Code flow rules Valid values for the rules severity are: `error | warn | info | off`. diff --git a/src/index.ts b/src/index.ts index b2767b7..ad6a015 100644 --- a/src/index.ts +++ b/src/index.ts @@ -52,6 +52,7 @@ export type BsLintConfig = Pick ({ + message: `${ST} Reassignment of the type of '${varName}' from ${previousType} to ${newType}`, + code: CodeStyleError.NameShadowing, + severity: severity, + source: 'bslint' }) }; diff --git a/src/plugins/codeStyle/index.spec.ts b/src/plugins/codeStyle/index.spec.ts index c619df8..2dfbbfe 100644 --- a/src/plugins/codeStyle/index.spec.ts +++ b/src/plugins/codeStyle/index.spec.ts @@ -998,6 +998,37 @@ describe('codeStyle', () => { }); }); + describe('type-reassignment', () => { + it('detects type reassignment', async () => { + const diagnostics = await linter.run({ + ...project1, + files: ['source/type-reassignment.brs'], + rules: { + 'type-reassignment': 'error' + } + }); + expectDiagnosticsFmt(diagnostics, [ + '12:LINT3026:Strictness: Reassignment of the type of \'param\' from string to integer', + '18:LINT3026:Strictness: Reassignment of the type of \'value\' from integer to string', + '27:LINT3026:Strictness: Reassignment of the type of \'value\' from integer to dynamic' + ]); + }); + + it('allows type reassignment with custom types', async () => { + const diagnostics = await linter.run({ + ...project1, + files: ['source/type-reassignment-custom.bs'], + rules: { + 'type-reassignment': 'error' + } + }); + expectDiagnosticsFmt(diagnostics, [ + '30:LINT3026:Strictness: Reassignment of the type of \'arg\' from Iface1 to roAssociativeArray', + '44:LINT3026:Strictness: Reassignment of the type of \'arg\' from Child to Parent' + ]); + }); + }); + describe('fix', () => { // Filenames (without the extension) that we want to copy with a "-temp" suffix const tmpFileNames = [ diff --git a/src/plugins/codeStyle/index.ts b/src/plugins/codeStyle/index.ts index 2e01e46..6e3c772 100644 --- a/src/plugins/codeStyle/index.ts +++ b/src/plugins/codeStyle/index.ts @@ -27,7 +27,10 @@ import { ExtraSymbolData, OnScopeValidateEvent, InternalWalkMode, - isCallableType + isCallableType, + AssignmentStatement, + isFunctionExpression, + isDynamicType } from 'brighterscript'; import { RuleAAComma } from '../..'; import { addFixesToEvent } from '../../textEdit'; @@ -231,7 +234,7 @@ export default class CodeStyle implements CompilerPlugin { validateBrsFileInScope(file: BrsFile) { const diagnostics: (BsDiagnostic)[] = []; const { severity } = this.lintContext; - const { nameShadowing } = severity; + const { nameShadowing, typeReassignment } = severity; file.ast.walk(createVisitor({ NamespaceStatement: (nsStmt) => { @@ -248,6 +251,9 @@ export default class CodeStyle implements CompilerPlugin { }, EnumStatement: (enumStmt) => { this.validateNameShadowing(file, enumStmt, enumStmt.tokens.name, nameShadowing, diagnostics); + }, + AssignmentStatement: (assignStmt) => { + this.validateTypeReassignment(file, assignStmt, typeReassignment, diagnostics); } // eslint-disable-next-line no-bitwise }), { walkMode: WalkMode.visitStatementsRecursive | InternalWalkMode.visitFalseConditionalCompilationBlocks }); @@ -447,6 +453,44 @@ export default class CodeStyle implements CompilerPlugin { relatedInformation: relatedInformation }); } + + validateTypeReassignment(file: BrsFile, assignStmt: AssignmentStatement, severity: DiagnosticSeverity, diagnostics: (BsDiagnostic)[]) { + const functionExpression = assignStmt.findAncestor(isFunctionExpression); + if (!functionExpression) { + return; + } + const bodyTable = functionExpression.body?.getSymbolTable(); + const rhsType = assignStmt.value?.getType({ flags: SymbolTypeFlag.runtime }); + if (!rhsType.isResolvable()) { + return; + } + const varName = assignStmt.tokens.name.text; + let previousType = functionExpression.getSymbolTable().getSymbolType(varName, { flags: SymbolTypeFlag.runtime }); + + if (!previousType) { + // check for last previous assignment + const symbols = bodyTable.getSymbol(varName, SymbolTypeFlag.runtime); + for (const symbol of symbols) { + if (util.comparePosition(symbol.data?.definingNode?.location?.range?.start, assignStmt.location.range.start) < 0) { + previousType = symbol.type; + } else { + break; + } + } + } + + if (previousType?.isResolvable()) { + // is this different? + if (!isDynamicType(previousType)) { + if (isDynamicType(rhsType) || !previousType.isTypeCompatible(rhsType)) { + diagnostics.push({ + ...messages.typeReassignment(varName, previousType.toString(), rhsType.toString(), severity), + location: assignStmt.location + }); + } + } + } + } } /** diff --git a/src/util.ts b/src/util.ts index 4969825..9ebd0e8 100644 --- a/src/util.ts +++ b/src/util.ts @@ -29,7 +29,8 @@ export function getDefaultRules(): BsLintConfig['rules'] { 'no-print': 'off', 'no-assocarray-component-field-type': 'off', 'no-array-component-field-type': 'off', - 'name-shadowing': 'off' + 'name-shadowing': 'off', + 'type-reassignment': 'off' }; } @@ -168,7 +169,8 @@ function rulesToSeverity(rules: BsLintConfig['rules']) { colorCertCompliant: rules['color-cert'], noAssocarrayComponentFieldType: ruleToSeverity(rules['no-assocarray-component-field-type']), noArrayComponentFieldType: ruleToSeverity(rules['no-array-component-field-type']), - nameShadowing: ruleToSeverity(rules['name-shadowing']) + nameShadowing: ruleToSeverity(rules['name-shadowing']), + typeReassignment: ruleToSeverity(rules['type-reassignment']) }; } diff --git a/test/project1/source/type-reassignment-custom.bs b/test/project1/source/type-reassignment-custom.bs new file mode 100644 index 0000000..85486e5 --- /dev/null +++ b/test/project1/source/type-reassignment-custom.bs @@ -0,0 +1,51 @@ +interface Iface1 + a as string +end interface + +interface Iface2 + a as string + optional b as string +end interface + +interface OtherFace1 + a as integer +end interface + +function convertToIface1(arg as Iface2) as Iface1 + arg = {a: arg.a} ' no diagnostic + return arg +end function + +function convertToIface2(arg as Iface1) as Iface2 + arg = {a: arg.a, b: "hello"} ' no diagnostic + return arg +end function + +function convertToIface2ViaTypeCast(arg as Iface1) + arg = arg as Iface2 ' no diagnostic + return arg +end function + +function convertToToOtherFace1(arg as Iface1) + arg as OtherFace1 = {a: 1} ' diagnostic + return arg +end function + + +class Parent + a as string +end class + +class Child extends Parent + b as string +end class + +function convertToParent(arg as Child) + arg = new Parent() ' diagnostic + return arg +end function + +function convertToChild(arg as Parent) + arg = new Child() ' no diagnostic - Child is a subclass of Parent + return arg +end function diff --git a/test/project1/source/type-reassignment.brs b/test/project1/source/type-reassignment.brs new file mode 100644 index 0000000..73c90de --- /dev/null +++ b/test/project1/source/type-reassignment.brs @@ -0,0 +1,40 @@ +sub checkTypesNoProblem(param as string) ' no diagnostic + param = "Hello " + param + num = 1234 + num = 6.7 + bool = true + if num > 3 + bool = false + end if +end sub + +sub checkTypesChangeFromParam(param as string) + param = 1 ' was string. now integer +end sub + + +sub checkTypesInFunc() + value = 1 + value = "hello" ' was string. now integer +end sub + +function getDynamic() + return 1 +end function + +sub checkTypesDefinedToDynamic() + value = 1 + value = getDynamic() ' was integer. now dynamic +end sub + +sub checkTypesNumberChange() ' no diagnostic + param = 1 + param = 3.14 +end sub + + +sub checkTypesDynamicToDefined() ' no diagnostic + value = getDynamic() + value = 1 ' was dynamic. now integer +end sub + From 18ae56e97c7eded02aaf89cc102fd9b25d2771bc Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Tue, 10 Sep 2024 08:33:28 -0300 Subject: [PATCH 2/2] Some more tests around object type --- src/plugins/codeStyle/index.spec.ts | 3 ++- test/project1/source/type-reassignment.brs | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/plugins/codeStyle/index.spec.ts b/src/plugins/codeStyle/index.spec.ts index 2dfbbfe..0245a9c 100644 --- a/src/plugins/codeStyle/index.spec.ts +++ b/src/plugins/codeStyle/index.spec.ts @@ -1010,7 +1010,8 @@ describe('codeStyle', () => { expectDiagnosticsFmt(diagnostics, [ '12:LINT3026:Strictness: Reassignment of the type of \'param\' from string to integer', '18:LINT3026:Strictness: Reassignment of the type of \'value\' from integer to string', - '27:LINT3026:Strictness: Reassignment of the type of \'value\' from integer to dynamic' + '27:LINT3026:Strictness: Reassignment of the type of \'value\' from integer to dynamic', + '53:LINT3026:Strictness: Reassignment of the type of \'obj\' from integer to roAssociativeArray' ]); }); diff --git a/test/project1/source/type-reassignment.brs b/test/project1/source/type-reassignment.brs index 73c90de..27097e1 100644 --- a/test/project1/source/type-reassignment.brs +++ b/test/project1/source/type-reassignment.brs @@ -38,3 +38,17 @@ sub checkTypesDynamicToDefined() ' no diagnostic value = 1 ' was dynamic. now integer end sub +sub checkTypesObject(obj as object) + if obj = invalid + obj = createObject("roAssociativeArray") + end if + obj.foo = "bar" +end sub + +sub checkTypesObjectToPrimitive(obj as object) + obj = 3 ' This is allowed because primitive types can be boxed as objects +end sub + +sub checkTypesPrimitiveToObject(obj as integer) + obj = createObject("roAssociativeArray") ' was integer. now associativearray +end sub