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

Issue:
The `assertion-arguments` rule should validate message argument type
<!-- Issuehunt Badges -->
[<img alt="Issuehunt badges" src="https://img.shields.io/badge/IssueHunt-%2460%20Funded-%2300A156.svg" />](https://issuehunt.io/r/avajs/eslint-plugin-ava/issues/167)
<!-- /Issuehunt Badges -->


This currently passes `t.deepEqual({}, {}, {});`, which can make for some hard to debug issues. @gajus [reported](https://gitter.im/avajs/ava?at=5880e744e836bf70106558a4) such issue on Gitter.

We should ensure that if `message` is specified, it's a string. We could implement inline literal validation first, as that's pretty easy.

We should also detect the type if it's a variable defined in the same scope:

```js
const message = 'foo';

...

t.deepEqual({}, {}, message);
```

@jfmengels Many rules could use some kind of naive type inference. Maybe [`eslint-ast-utils`](https://github.com/jfmengels/eslint-ast-utils) could add something like that?

---

> *Note: This issue has a bounty, so it's expected that you are an experienced programmer and that you give it your best effort if you intend to tackle this. Don't forget, if applicable, to add tests, docs (double-check for typos). And don't be sloppy. Review your own diff multiple times and try to find ways to improve and simplify your code. Instead of asking too many questions, present solutions. The point of an issue bounty is to reduce my workload, not give me more. Include a 🦄 in your PR description to indicate that you've read this. Thanks for helping out 🙌* - @sindresorhus


<!-- Issuehunt content -->

---

<details>
<summary>
<b>IssueHunt Summary</b>
</summary>


### Backers (Total: $60.00)

- [<img src='https://avatars3.githubusercontent.com/u/44827199?v=4' alt='issuehunt' width=24 height=24> issuehunt](https://issuehunt.io/u/issuehunt) ($60.00)

### Submitted pull Requests
- [#314 Add check for the assertion message to be a string](https://issuehunt.io/r/avajs/eslint-plugin-ava/pull/314)
---

#### [Become a backer now!](https://issuehunt.io/r/avajs/eslint-plugin-ava/issues/167)
#### [Or submit a pull request to get the deposits!](https://issuehunt.io/r/avajs/eslint-plugin-ava/issues/167)
### Tips

- Checkout the [Issuehunt explorer](https://issuehunt.io/r/avajs/eslint-plugin-ava/) to discover more funded issues.
- Need some help from other developers? [Add your repositories](https://issuehunt.io/r/new) on IssueHunt to raise funds.
</details>
<!-- /Issuehunt content-->

Pull request description:
Ensure the assertion message is a string
This will check for the presence of the extra argument (which should be the `message`) and verify if it's a string.
It will try to lookup variable and its value in the scope, but non-linear flow could break it.

```js
let message;
function woops () {
	message = null;
}
message = 'ok';
woops();
t.is('same', 'same', message); // <= this will not raise an error
```

Should we cover this ?

Fixes #167


<!-- Issuehunt content -->

---

<details>
<summary>
<b>IssueHunt Summary</b>
</summary>

### Referenced issues

This pull request has been submitted to:
- [#167: The `assertion-arguments` rule should validate message argument type](https://issuehunt.io/repos/49722492/issues/167)
---
</details>
<!-- /Issuehunt content-->

Patch:
diff --git a/docs/rules/assertion-arguments.md b/docs/rules/assertion-arguments.md
index 686f2ff..24c5a5b 100644
--- a/docs/rules/assertion-arguments.md
+++ b/docs/rules/assertion-arguments.md
@@ -16,6 +16,7 @@ const test = require('ava');
 test(t => {
 	t.is(value); // Not enough arguments
 	t.is(value, expected, message, extra); // Too many arguments
+	t.is(value, expected, false); // Assertion message is not a string
 });
 
 /* eslint ava/assertion-arguments: ["error", {"message": "always"}] */
diff --git a/rules/assertion-arguments.js b/rules/assertion-arguments.js
index 5c050fd..0f6bbaa 100644
--- a/rules/assertion-arguments.js
+++ b/rules/assertion-arguments.js
@@ -1,6 +1,6 @@
 'use strict';
 const {visitIf} = require('enhance-visitors');
-const {getStaticValue, isOpeningParenToken, isCommaToken} = require('eslint-utils');
+const {getStaticValue, isOpeningParenToken, isCommaToken, findVariable} = require('eslint-utils');
 const util = require('../util');
 const createAvaRule = require('../create-ava-rule');
 
@@ -203,6 +203,13 @@ function noComments(sourceCode, ...nodes) {
 	});
 }
 
+function isString(node) {
+	const {type} = node;
+	return type === 'TemplateLiteral' ||
+		type === 'TaggedTemplateExpression' ||
+		(type === 'Literal' && typeof node.value === 'string');
+}
+
 const create = context => {
 	const ava = createAvaRule();
 	const options = context.options[0] || {};
@@ -271,6 +278,24 @@ const create = context => {
 
 				checkArgumentOrder({node, assertion: firstNonSkipMember, context});
 			}
+
+			if (gottenArgs === nArgs.max && nArgs.min !== nArgs.max) {
+				let lastArg = node.arguments[node.arguments.length - 1];
+
+				if (lastArg.type === 'Identifier') {
+					const variable = findVariable(context.getScope(), lastArg);
+					let value;
+					for (const ref of variable.references) {
+						value = ref.writeExpr || value;
+					}
+
+					lastArg = value;
+				}
+
+				if (!isString(lastArg)) {
+					report(node, 'Assertion message should be a string.');
+				}
+			}
 		})
 	});
 };
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/assertion-arguments.js b/test/assertion-arguments.js
index 51074f3..4b51147 100644
--- a/test/assertion-arguments.js
+++ b/test/assertion-arguments.js
@@ -19,6 +19,7 @@ const outOfOrderError = (line, column, endLine, endColumn) => ({
 	message: 'Expected values should come after actual values.',
 	line, column, endLine, endColumn
 });
+const messageIsNotStringError = 'Assertion message should be a string.';
 
 const header = 'const test = require(\'ava\');';
 
@@ -331,7 +332,11 @@ ruleTester.run('assertion-arguments', rule, {
 		testCase(false, 't.regex(\'static\', new RegExp(\'[dynamic]+\'));'),
 		testCase(false, 't.regex(dynamic, /[static]/);'),
 		testCase(false, 't.notRegex(\'static\', new RegExp(\'[dynamic]+\'));'),
-		testCase(false, 't.notRegex(dynamic, /[static]/);')
+		testCase(false, 't.notRegex(dynamic, /[static]/);'),
+
+		// Lookup message type
+		testCase(false, 'const message = \'ok\'; t.assert(true, message);'),
+		testCase(false, 'const message = \'ok\'; t.is(42, 42, message);')
 	],
 	invalid: [
 		// Not enough arguments
@@ -538,6 +543,12 @@ ruleTester.run('assertion-arguments', rule, {
 				outOfOrderError(1, 13, 1, 23 + expression.length),
 				{output: `t.deepEqual(${expression}, 'static');`}
 			)
-		)
+		),
+
+		// Message is not string
+		testCase(false, 't.assert(true, true);', messageIsNotStringError),
+		testCase(false, 't.deepEqual({}, {}, 42);', messageIsNotStringError),
+		testCase(false, 't.fail({});', messageIsNotStringError),
+		testCase(false, 'let message = "ok"; message = false; t.assert(true, message);', messageIsNotStringError)
 	]
 });
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
 		}
 	]
 });