From b3181a89e8cc0820c85653868dfaf59b8d167cbf Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Fri, 13 Sep 2024 09:25:33 -0300 Subject: [PATCH 1/2] Refactored var tracking diagnostic messages --- src/plugins/trackCodeFlow/varTracking.ts | 62 ++++++++++++++++++------ 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/src/plugins/trackCodeFlow/varTracking.ts b/src/plugins/trackCodeFlow/varTracking.ts index bed2201..3faece8 100644 --- a/src/plugins/trackCodeFlow/varTracking.ts +++ b/src/plugins/trackCodeFlow/varTracking.ts @@ -17,6 +17,45 @@ enum ValidationKind { Unsafe = 'Unsafe' } + +export const VarTrackingMessages = { + varCasing: (curr: VarInfo, name: { text: string; location: Location }) => ({ + severity: DiagnosticSeverity.Warning, + source: 'bslint', + code: VarLintError.CaseMismatch, + message: `Variable '${name.text}' was previously set with a different casing as '${curr.name}'`, + data: { + name: curr.name, + location: name.location + } + }), + unsafeIterator: (name: string) => ({ + severity: DiagnosticSeverity.Error, + source: 'bslint', + code: VarLintError.UnsafeIteratorVar, + message: `Using iterator variable '${name}' outside loop` + }), + unusedVariable: (name: string) => ({ + severity: DiagnosticSeverity.Warning, + source: 'bslint', + code: VarLintError.UnusedVariable, + message: `Variable '${name}' is set but value is never used` + }), + unsafeInitialization: (name: string) => ({ + severity: DiagnosticSeverity.Error, + source: 'bslint', + code: VarLintError.UnsafeInitialization, + message: `Not all the code paths assign '${name}'` + }), + uninitializedVariable: (name: string, scopeName: string) => ({ + severity: DiagnosticSeverity.Error, + source: 'bslint', + code: VarLintError.UninitializedVar, + message: `Using uninitialised variable '${name}' when this file is included in scope '${scopeName}'` + }) +}; + + interface ValidationInfo { kind: ValidationKind; name: string; @@ -60,14 +99,9 @@ export function createVarLinter( function verifyVarCasing(curr: VarInfo, name: { text: string; location: Location }) { if (curr && curr.name !== name.text) { diagnostics.push({ + ...VarTrackingMessages.varCasing(curr, name), severity: severity.caseSensitivity, - code: VarLintError.CaseMismatch, - message: `Variable '${name.text}' was previously set with a different casing as '${curr.name}'`, - location: name.location, - data: { - name: curr.name, - location: name.location - } + location: name.location }); } } @@ -302,16 +336,14 @@ export function createVarLinter( if (local.isUnsafe && !findSafeLocal(name)) { if (local.restriction) { diagnostics.push({ + ...VarTrackingMessages.unsafeIterator(name), severity: severity.unsafeIterators, - code: VarLintError.UnsafeIteratorVar, - message: `Using iterator variable '${name}' outside loop`, location: expr.location }); } else if (!isNarrowing(local, expr, parent, curr)) { diagnostics.push({ - severity: severity.unsafePathLoop, - code: VarLintError.UnsafeInitialization, - message: `Not all the code paths assign '${name}'`, + ...VarTrackingMessages.unsafeInitialization(name), + severity: severity.unsafePathLoop, // should this be severity.assignAllPath? location: expr.location }); } @@ -353,9 +385,8 @@ export function createVarLinter( locals.forEach(local => { if (!local.isUsed && !local.restriction) { diagnostics.push({ + ...VarTrackingMessages.unusedVariable(local.name), severity: severity.unusedVariable, - code: VarLintError.UnusedVariable, - message: `Variable '${local.name}' is set but value is never used`, location: local.location }); } @@ -430,9 +461,8 @@ function deferredVarLinter( case ValidationKind.UninitializedVar: if (!hasCallable) { diagnostics.push({ + ...VarTrackingMessages.uninitializedVariable(name, scope.name), severity: DiagnosticSeverity.Error, - code: VarLintError.UninitializedVar, - message: `Using uninitialised variable '${name}' when this file is included in scope '${scope.name}'`, location: location }); } From 5f98714c713c7c6155aa3018824f2ce30355e756 Mon Sep 17 00:00:00 2001 From: Mark Pearce Date: Fri, 13 Sep 2024 16:21:25 -0300 Subject: [PATCH 2/2] Added var tracking for #if statements --- src/plugins/trackCodeFlow/index.spec.ts | 22 ++++++ src/plugins/trackCodeFlow/varTracking.ts | 4 +- ...sign-all-paths-conditional-compilation.brs | 77 +++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 test/project1/source/assign-all-paths-conditional-compilation.brs diff --git a/src/plugins/trackCodeFlow/index.spec.ts b/src/plugins/trackCodeFlow/index.spec.ts index 8fd0398..8139e8c 100644 --- a/src/plugins/trackCodeFlow/index.spec.ts +++ b/src/plugins/trackCodeFlow/index.spec.ts @@ -230,6 +230,28 @@ describe('trackCodeFlow', () => { expect(actual).deep.equal(expected); }); + it('implements assign-all-paths with conditional compilation', async () => { + const diagnostics = await linter.run({ + ...project1, + files: ['source/assign-all-paths-conditional-compilation.brs'], + rules: { + 'assign-all-paths': 'error', + 'consistent-return': 'off', + 'unused-variable': 'off' + }, + diagnosticFilters: [1001, 1090] + } as any); + const actual = fmtDiagnostics(diagnostics); + const expected = [ + `15:LINT1003:Not all the code paths assign 'a'`, + `23:LINT1003:Not all the code paths assign 'a'`, + `42:LINT1003:Not all the code paths assign 'a'`, + `65:LINT1003:Not all the code paths assign 'a'`, + `76:LINT1003:Not all the code paths assign 'a'` + ]; + expect(actual).deep.equal(expected); + }); + it('report errors for classes', async () => { const diagnostics = await linter.run({ ...project1, diff --git a/src/plugins/trackCodeFlow/varTracking.ts b/src/plugins/trackCodeFlow/varTracking.ts index 3faece8..5465d46 100644 --- a/src/plugins/trackCodeFlow/varTracking.ts +++ b/src/plugins/trackCodeFlow/varTracking.ts @@ -1,4 +1,4 @@ -import { BscFile, FunctionExpression, BsDiagnostic, Range, isForStatement, isForEachStatement, isIfStatement, isAssignmentStatement, isNamespaceStatement, NamespaceStatement, Expression, isVariableExpression, isBinaryExpression, TokenKind, Scope, CallableContainerMap, DiagnosticSeverity, isLiteralInvalid, isWhileStatement, isCatchStatement, isLabelStatement, isGotoStatement, ParseMode, util, isMethodStatement, isTryCatchStatement } from 'brighterscript'; +import { BscFile, FunctionExpression, BsDiagnostic, Range, isForStatement, isForEachStatement, isIfStatement, isAssignmentStatement, isNamespaceStatement, NamespaceStatement, Expression, isVariableExpression, isBinaryExpression, TokenKind, Scope, CallableContainerMap, DiagnosticSeverity, isLiteralInvalid, isWhileStatement, isCatchStatement, isLabelStatement, isGotoStatement, ParseMode, util, isMethodStatement, isTryCatchStatement, isConditionalCompileStatement } from 'brighterscript'; import { LintState, StatementInfo, NarrowingInfo, VarInfo, VarRestriction } from '.'; import { PluginContext } from '../../util'; import { Location } from 'vscode-languageserver-types'; @@ -280,7 +280,7 @@ export function createVarLinter( if (!parent.locals) { parent.locals = locals; } else { - const isParentBranched = isIfStatement(parent.stat) || isTryCatchStatement(parent.stat); + const isParentBranched = isIfStatement(parent.stat) || isTryCatchStatement(parent.stat) || isConditionalCompileStatement(parent.stat); const isLoop = isForStatement(closed.stat) || isForEachStatement(closed.stat) || isWhileStatement(closed.stat); locals.forEach((local, name) => { const parentLocal = parent.locals.get(name); diff --git a/test/project1/source/assign-all-paths-conditional-compilation.brs b/test/project1/source/assign-all-paths-conditional-compilation.brs new file mode 100644 index 0000000..443bffe --- /dev/null +++ b/test/project1/source/assign-all-paths-conditional-compilation.brs @@ -0,0 +1,77 @@ +function assignPathsCC() as dynamic + #if true + a = 1 + #else + a = 2 + #end if + return a ' no error +end function + + +function assignPathsCC1() as dynamic + #if true + a = 1 + #end if + return a ' not really an error because #else block will never be run, but displays one anyway. Maybe fix this +end function + + +function assignPathsCC2() as dynamic + #if SOME_VAR + a = 1 + #end if + return a ' error +end function + + +function assignPathsCC3() as dynamic + #if SOME_VAR + a = 1 + #else + a = 2 + #end if + return a ' no error +end function + +function assignPathsCC4() as dynamic + #if SOME_VAR + b = 1 + #else + a = 2 + #end if + return a ' error +end function + + +function assignPathsCC5() as dynamic + #if SOME_VAR + a = 1 + #else if SOME_VAR2 + a = 2 + #else + a = 3 + #end if + return a ' No error +end function + +function assignPathsCC6() as dynamic + #if SOME_VAR + a = 1 + #else if SOME_VAR2 + a = 2 + #else + ' missing assignment + #end if + return a ' error +end function + +function assignPathsCC7() as dynamic + #if SOME_VAR + a = 1 + #else if SOME_VAR2 + ' missing assignment + #else + a = 2 + #end if + return a ' error +end function \ No newline at end of file