Conditional compile code flow - var tracking (#132)

* Refactored var tracking diagnostic messages

* Added var tracking for #if statements

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,

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';
@@ -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
             });
         }
     }
@@ -246,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);
@@ -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
                     });
                 }

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