Repository: avajs/eslint-plugin-ava (js)

Issue:
Edge-case crashes & false-positives in no-statement-after-end with unreachable statements
The path analysis logic in `no-statement-after-end` doesn't properly handle unreachable statements, sometimes causing crashes or false-positives.

In `eslint` (tested in v6 and v7), `CodePathSegment`s end when encountering e.g. `return` or `throw`. Unreachable statements after that point don't appear to be in any `CodePathSegment`. This can cause `no-statement-after-end` to crash when trying to analyze unreachable statements in the global scope or to treat unreachable statements in one path as reachable statements in the next outermost path.

Example of a crash:
```javascript
const test = require('ava');

throw new Error('make some unreachable code');

1; // <-- triggers a crash
```

Example of a false positive:
```javascript
const test = require('ava');

test.cb(t => {
  function newCodePath() {
    throw new Error('make some unreachable code');
    // Now we're in the outer scope, according to the rule
    t.end();
  }

  // Now the rule thinks we've ended.
  1; // <-- triggers a report
});
```

This came up while writing tests for avajs/ava#1424.

This is unlikely to affect many users, since triggering it requires very odd and/or obviously incorrect code.

I'm working on a PR to fix this presently.

Pull request description:
Fix crash, false-positive in no-statement-after-end
Fixes #315 by changing the way `no-statement-after-end` records `CodePathSegment`s. The stack is used only to hold the in-progress segment in the outer `CodePath`s. When `CodePath`s begin or end, the current segment is pushed onto or popped from the stack. When `CodePathSegment`s begin or end, the current segment is set or unset.

When in unreachable code at the end of a path, the current segment is therefore undefined, rather than erroneously taken from the next-outermost path.

Checks whether the current segment is set before operating on it.

(Interestingly, this rule doesn't catch statements after `t.end()` in unreachable code, e.g.:
```javascript
throw new Error();
t.end();
1; // <-- does not trigger a report
```
This behavior predates this PR.)

While I believe this fixes the issue, I don't fully understand how this rule ought to handle its edge-cases nor why the codepath analysis system works this way. Please review carefully?

Patch:
diff --git a/rules/no-statement-after-end.js b/rules/no-statement-after-end.js
index 90ef126..58678cc 100644
--- a/rules/no-statement-after-end.js
+++ b/rules/no-statement-after-end.js
@@ -21,10 +21,19 @@ const create = context => {
 
 	let currentSegmentInfo;
 
+	function pathStart() {
+		if (currentSegmentInfo !== undefined) {
+			segmentInfoStack.push(currentSegmentInfo);
+			currentSegmentInfo = undefined;
+		}
+	}
+
+	function pathEnd() {
+		currentSegmentInfo = segmentInfoStack.pop();
+	}
+
 	function segmentStart(segment) {
 		// A new CodePathSegment has started, create an "info" object to track this segments state.
-		segmentInfoStack.push(currentSegmentInfo);
-
 		currentSegmentInfo = {
 			ended: false,
 			prev: segment.prevSegments.map(previousSegment => segmentInfoMap.get(previousSegment.id))
@@ -34,12 +43,14 @@ const create = context => {
 	}
 
 	function segmentEnd() {
-		currentSegmentInfo = segmentInfoStack.pop();
+		currentSegmentInfo = undefined;
 	}
 
 	function checkForEndExpression(node) {
 		if (isEndExpression(node)) {
-			currentSegmentInfo.ended = true;
+			if (currentSegmentInfo !== undefined) {
+				currentSegmentInfo.ended = true;
+			}
 		}
 	}
 
@@ -48,6 +59,12 @@ const create = context => {
 			return;
 		}
 
+		// If there is no current segment (this occurs in unreachable code), then we
+		// can't check whether `t.end()` was called
+		if (currentSegmentInfo === undefined) {
+			return;
+		}
+
 		const ended = [currentSegmentInfo]
 			.concat(currentSegmentInfo.prev)
 			.filter(info => info.ended);
@@ -85,6 +102,8 @@ const create = context => {
 				checkStatement(node);
 			}
 		},
+		onCodePathStart: pathStart,
+		onCodePathEnd: pathEnd,
 		onCodePathSegmentStart: segmentStart,
 		onCodePathSegmentEnd: segmentEnd,
 		CallExpression: checkForEndExpression


Test patch:
diff --git a/test/no-statement-after-end.js b/test/no-statement-after-end.js
index 2230803..2aef50f 100644
--- a/test/no-statement-after-end.js
+++ b/test/no-statement-after-end.js
@@ -30,7 +30,22 @@ ruleTester.run('no-statement-after-end', rule, {
 		cbTest('return t.end();'),
 		cbTest('t.end(); return;'),
 		// Valid because it is not a test file (no header)
-		cbTest('t.end(); t.is(1, 1);', false)
+		cbTest('t.end(); t.is(1, 1);', false),
+		`
+		const test = require('ava');
+
+		throw new Error();
+
+		1;
+		`,
+		cbTest(`
+			function newCodePath() {
+				throw new Error('make some unreachable code');
+				t.end();
+			}
+
+			1;
+		`)
 	],
 	invalid: [
 		{
@@ -48,6 +63,20 @@ ruleTester.run('no-statement-after-end', rule, {
 		{
 			code: cbTest('if (t.context.a === 1) { t.end(); }\nt.is(1, 1); t.end();'),
 			errors
+		},
+		{
+			code: cbTest(`
+				function newCodePath() {
+					// ...
+				}
+				t.end();
+				1;
+			`),
+			errors
+		},
+		{
+			code: cbTest('t.end(); function newCodePath() {} 1;'),
+			errors
 		}
 	]
 });