Avoid false positive for missing return (#78)

* Avoid false positive for missing return

* Additional tests to ensure we detect return/throws within catch statement as well

Fixes #68

* Bump peer dependency

package.json

@@ -47,7 +47,7 @@
     "@types/node": "^14.6.0",
     "@typescript-eslint/eslint-plugin": "^3.10.1",
     "@typescript-eslint/parser": "^3.10.1",
-    "brighterscript": "^0.57.0",
+    "brighterscript": "^0.59.0",
     "chai": "^4.3.6",
     "coveralls": "^3.1.1",
     "eslint": "^7.7.0",
@@ -58,7 +58,7 @@
     "typescript": "^3.9.7"
   },
   "peerDependencies": {
-    "brighterscript": ">= 0.43 < 1"
+    "brighterscript": ">= 0.59.0 < 1"
   },
   "mocha": {
     "spec": "src/**/*.spec.ts",

src/plugins/trackCodeFlow/index.spec.ts

@@ -195,7 +195,9 @@ describe('trackCodeFlow', () => {
         });
         const actual = fmtDiagnostics(diagnostics);
         const expected = [
+            `05:1001:Cannot find name 'a'`,
             `05:LINT1003:Not all the code paths assign 'a'`,
+            `15:1001:Cannot find name 'b'`,
             `15:LINT1003:Not all the code paths assign 'b'`
         ];
         expect(actual).deep.equal(expected);
@@ -298,7 +300,8 @@ describe('trackCodeFlow', () => {
             `25:LINT2004:Not all code paths return a value`,
             `32:LINT2004:Not all code paths return a value`,
             `39:LINT2004:Not all code paths return a value`,
-            `45:LINT2004:Not all code paths return a value`
+            `45:LINT2004:Not all code paths return a value`,
+            `49:LINT2004:Not all code paths return a value`
         ];
         expect(actual).deep.equal(expected);
     });

src/plugins/trackCodeFlow/index.ts

@@ -1,4 +1,4 @@
-import { BscFile, Scope, BsDiagnostic, CallableContainerMap, BrsFile, OnGetCodeActionsEvent, Statement, EmptyStatement, FunctionExpression, isForEachStatement, isForStatement, isIfStatement, isWhileStatement, Range, createStackedVisitor, isBrsFile, isStatement, isExpression, WalkMode } from 'brighterscript';
+import { BscFile, Scope, BsDiagnostic, CallableContainerMap, BrsFile, OnGetCodeActionsEvent, Statement, EmptyStatement, FunctionExpression, isForEachStatement, isForStatement, isIfStatement, isWhileStatement, Range, createStackedVisitor, isBrsFile, isStatement, isExpression, WalkMode, isTryCatchStatement, isCatchStatement } from 'brighterscript';
 import { PluginContext } from '../../util';
 import { createReturnLinter } from './returnTracking';
 import { createVarLinter, resetVarContext, runDeferredValidation } from './varTracking';
@@ -46,6 +46,7 @@ export interface LintState {
     stack: Statement[];
     blocks: WeakMap<Statement, StatementInfo>;
     ifs?: StatementInfo;
+    trys?: StatementInfo;
     branch?: StatementInfo;
 }
 
@@ -82,6 +83,7 @@ export default class TrackCodeFlow {
                 stack: [],
                 blocks: new WeakMap(),
                 ifs: undefined,
+                trys: undefined,
                 branch: undefined
             };
             let curr: StatementInfo = {
@@ -109,18 +111,23 @@ export default class TrackCodeFlow {
 
                 if (isIfStatement(opened)) {
                     state.ifs = curr;
-                } else if (!curr.parent || isIfStatement(curr.parent)) {
+                } else if (isTryCatchStatement(opened)) {
+                    state.trys = curr;
+                } else if (!curr.parent || isIfStatement(curr.parent) || isTryCatchStatement(curr.parent) || isCatchStatement(curr.parent)) {
                     state.branch = curr;
                 }
                 state.parent = curr;
-
             }, (closed, stack) => {
                 const block = state.blocks.get(closed);
                 state.parent = state.blocks.get(stack[stack.length - 1]);
                 if (isIfStatement(closed)) {
-                    const { ifs, branch } = findBranch(state);
+                    const { ifs, branch } = findIfBranch(state);
                     state.ifs = ifs;
                     state.branch = branch;
+                } else if (isTryCatchStatement(closed)) {
+                    const { trys, branch } = findTryBranch(state);
+                    state.trys = trys;
+                    state.branch = branch;
                 }
                 if (block) {
                     returnLinter.closeBlock(block);
@@ -171,7 +178,7 @@ export default class TrackCodeFlow {
 }
 
 // Find parent if and block where code flow is branched
-function findBranch(state: LintState): { ifs?: StatementInfo; branch?: StatementInfo } {
+function findIfBranch(state: LintState): { ifs?: StatementInfo; branch?: StatementInfo } {
     const { blocks, parent, stack } = state;
     for (let i = stack.length - 2; i >= 0; i--) {
         if (isIfStatement(stack[i])) {
@@ -187,7 +194,24 @@ function findBranch(state: LintState): { ifs?: StatementInfo; branch?: Statement
     };
 }
 
+// Find parent try and block where code flow is branched
+function findTryBranch(state: LintState): { trys?: StatementInfo; branch?: StatementInfo } {
+    const { blocks, parent, stack } = state;
+    for (let i = stack.length - 2; i >= 0; i--) {
+        if (isTryCatchStatement(stack[i])) {
+            return {
+                trys: blocks.get(stack[i]),
+                branch: blocks.get(stack[i + 1])
+            };
+        }
+    }
+    return {
+        trys: undefined,
+        branch: parent
+    };
+}
+
 // `if` and `for/while` are considered as multi-branch
 export function isBranchedStatement(stat: Statement) {
-    return isIfStatement(stat) || isForStatement(stat) || isForEachStatement(stat) || isWhileStatement(stat);
+    return isIfStatement(stat) || isForStatement(stat) || isForEachStatement(stat) || isWhileStatement(stat) || isTryCatchStatement(stat);
 }

src/plugins/trackCodeFlow/returnTracking.ts

@@ -1,4 +1,4 @@
-import { BscFile, FunctionExpression, BsDiagnostic, isCommentStatement, DiagnosticTag, isReturnStatement, isIfStatement, TokenKind, util, ReturnStatement } from 'brighterscript';
+import { BscFile, FunctionExpression, BsDiagnostic, isCommentStatement, DiagnosticTag, isReturnStatement, isIfStatement, isThrowStatement, TokenKind, util, ReturnStatement, ThrowStatement, isTryCatchStatement, isCatchStatement } from 'brighterscript';
 import { LintState, StatementInfo } from '.';
 import { PluginContext } from '../../util';
 
@@ -7,6 +7,10 @@ interface ReturnInfo {
     hasValue: boolean;
 }
 
+interface ThrowInfo {
+    stat: ThrowStatement;
+}
+
 enum ReturnLintError {
     UnreachableCode = 'LINT2001',
     ReturnValueUnexpected = 'LINT2002',
@@ -26,6 +30,7 @@ export function createReturnLinter(
 ) {
     const { severity } = lintContext;
     const returns: ReturnInfo[] = [];
+    const throws: ThrowInfo[] = [];
 
     function visitStatement(curr: StatementInfo) {
         const { parent } = state;
@@ -41,13 +46,21 @@ export function createReturnLinter(
                 });
             }
         } else if (isReturnStatement(curr.stat)) {
-            const { ifs, branch, parent } = state;
+            const { ifs, trys, branch, parent } = state;
             returns.push({
                 stat: curr.stat,
                 hasValue: !!curr.stat.value && !isCommentStatement(curr.stat.value)
             });
             // flag parent branch to return
-            const returnBlock = ifs ? branch : parent;
+            const returnBlock = (ifs || trys) ? branch : parent;
+            if (returnBlock && parent?.branches === 1) {
+                returnBlock.returns = true;
+            }
+        } else if (isThrowStatement(curr.stat)) {
+            const { ifs, trys, branch, parent } = state;
+            throws.push({ stat: curr.stat });
+            // flag parent branch to 'return'
+            const returnBlock = (ifs || trys) ? branch : parent;
             if (returnBlock && parent?.branches === 1) {
                 returnBlock.returns = true;
             }
@@ -58,14 +71,18 @@ export function createReturnLinter(
         const { parent } = state;
         if (!parent) {
             finalize(closed);
-        } else if (isIfStatement(closed.stat)) {
+        } else if (isIfStatement(closed.stat) || isTryCatchStatement(closed.stat) || isCatchStatement(closed.stat)) {
             if (closed.branches === 0) {
                 parent.returns = true;
                 parent.branches--;
             }
         } else if (closed.returns) {
             if (isIfStatement(parent.stat)) {
                 parent.branches--;
+            } else if (isTryCatchStatement(parent.stat)) {
+                parent.branches--;
+            } else if (isCatchStatement(parent.stat)) {
+                parent.branches--;
             }
         }
     }
@@ -108,7 +125,7 @@ export function createReturnLinter(
 
         // Function doesn't consistently return,
         // or doesn't return at all but has an explicit type
-        if ((requiresReturnValue || hasReturnedValue) && (missingBranches || returns.length === 0)) {
+        if ((requiresReturnValue || hasReturnedValue) && (missingBranches || (returns.length + throws.length) === 0)) {
             diagnostics.push({
                 severity: consistentReturn,
                 code: ReturnLintError.UnsafeReturnValue,

src/plugins/trackCodeFlow/varTracking.ts

@@ -1,4 +1,4 @@
-import { BscFile, FunctionExpression, BsDiagnostic, Range, isForStatement, isForEachStatement, isIfStatement, isAssignmentStatement, Expression, isVariableExpression, isBinaryExpression, TokenKind, Scope, CallableContainerMap, DiagnosticSeverity, isLiteralInvalid, isWhileStatement, isClassMethodStatement, isBrsFile, isCatchStatement, isLabelStatement, isGotoStatement, NamespacedVariableNameExpression, ParseMode } from 'brighterscript';
+import { BscFile, FunctionExpression, BsDiagnostic, Range, isForStatement, isForEachStatement, isIfStatement, isAssignmentStatement, isNamespaceStatement, NamespaceStatement, Expression, isVariableExpression, isBinaryExpression, TokenKind, Scope, CallableContainerMap, DiagnosticSeverity, isLiteralInvalid, isWhileStatement, isClassMethodStatement, isBrsFile, isCatchStatement, isLabelStatement, isGotoStatement, NamespacedVariableNameExpression, ParseMode } from 'brighterscript';
 import { LintState, StatementInfo, NarrowingInfo, VarInfo, VarRestriction } from '.';
 import { PluginContext } from '../../util';
 
@@ -291,7 +291,7 @@ export function createVarLinter(
                     kind: ValidationKind.UninitializedVar,
                     name: name,
                     range: expr.range,
-                    namespace: expr.namespaceName
+                    namespace: expr.findAncestor<NamespaceStatement>(isNamespaceStatement)?.nameExpression
                 });
                 return;
             } else {

test/project1/source/assign-all-paths.brs

@@ -10,7 +10,7 @@ sub error2()
     a = Rnd(10)
     if a > 0
         b = 0
-    elseif a > 1
+    else if a > 1
         b = 1
     end if
     print "b"; b 'error
@@ -56,7 +56,7 @@ sub error6()
     if a > 5
         b = a
     end if
-    if b <> invalid  ' opposite of narrowed exit
+    if b <> invalid ' opposite of narrowed exit
         return
     end if
     print "b"; b ' error
@@ -102,7 +102,7 @@ sub ok2()
     a = Rnd(10)
     if a > 0
         b = 0
-    elseif a > 1
+    else if a > 1
         b = 1
     else
         b = 2
@@ -155,7 +155,7 @@ sub ok6()
     a = Rnd(10)
     if a > 0
         b = 0
-    elseif a > 1
+    else if a > 1
         b = 1
     else
         ' dead branch

test/project1/source/consistent-return.brs

@@ -36,7 +36,7 @@ sub error7() as string ' error
 end sub
 
 sub error8()
-    some(function () 'error
+    some(function() 'error
         if true then return 0
     end function)
 end sub
@@ -46,6 +46,15 @@ sub error9()
     end sub)
 end sub
 
+function error10() as integer
+    a = 1
+    if a > 0
+        throw "something wrong"
+    else
+        ' missing return or throw here
+    end if
+end function
+
 sub ok1()
     return
 end sub
@@ -83,6 +92,40 @@ function ok6() as void
     return ' error
 end function
 
+function ok7() as integer
+    a = 1
+    if a > 0
+        throw "something wrong"
+    else
+        return 0
+    end if
+end function
+
+function ok8() as integer
+    throw "Not yet implemented"
+end function
+
+function ok9() as integer
+    try
+        a = 0
+        b = 100 / a
+        return b
+    catch err
+        throw "Not yet implemented"
+    end try
+end function
+
+function ok10() as integer
+    throw "Not yet implemented"
+end function
+
+function ok11() as void
+    a = 1
+    if a > 0
+        throw "something wrong"
+    end if
+end function
+
 sub some(o)
     print o
 end sub