Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds diagnostic for when a variables type changes #130

Open
wants to merge 2 commits into
base: v1
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export type BsLintConfig = Pick<BsConfig, 'project' | 'rootDir' | 'files' | 'cwd
'no-assocarray-component-field-type'?: RuleSeverity;
'no-array-component-field-type'?: RuleSeverity;
'name-shadowing'?: RuleSeverity;
'type-reassignment'?: RuleSeverity;
};
globals?: string[];
ignores?: string[];
Expand Down Expand Up @@ -88,6 +89,7 @@ export interface BsLintRules {
noAssocarrayComponentFieldType: BsLintSeverity;
noArrayComponentFieldType: BsLintSeverity;
nameShadowing: BsLintSeverity;
typeReassignment: BsLintSeverity;
}

export { Linter };
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/codeStyle/diagnosticMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,5 +222,11 @@ export const messages = {
code: CodeStyleError.NameShadowing,
severity: severity,
source: 'bslint'
}),
typeReassignment: (varName: string, previousType: string, newType: string, severity: DiagnosticSeverity) => ({
message: `${ST} Reassignment of the type of '${varName}' from ${previousType} to ${newType}`,
code: CodeStyleError.NameShadowing,
severity: severity,
source: 'bslint'
})
};
32 changes: 32 additions & 0 deletions src/plugins/codeStyle/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,38 @@ 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',
'53:LINT3026:Strictness: Reassignment of the type of \'obj\' from integer to roAssociativeArray'
]);
});

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 = [
Expand Down
48 changes: 46 additions & 2 deletions src/plugins/codeStyle/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ import {
ExtraSymbolData,
OnScopeValidateEvent,
InternalWalkMode,
isCallableType
isCallableType,
AssignmentStatement,
isFunctionExpression,
isDynamicType
} from 'brighterscript';
import { RuleAAComma } from '../..';
import { addFixesToEvent } from '../../textEdit';
Expand Down Expand Up @@ -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) => {
Expand All @@ -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 });
Expand Down Expand Up @@ -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<FunctionExpression>(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
});
}
}
}
}
}

/**
Expand Down
6 changes: 4 additions & 2 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
};
}

Expand Down Expand Up @@ -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'])
};
}

Expand Down
51 changes: 51 additions & 0 deletions test/project1/source/type-reassignment-custom.bs
Original file line number Diff line number Diff line change
@@ -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
54 changes: 54 additions & 0 deletions test/project1/source/type-reassignment.brs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
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

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