Catch & fix out-of-order assertion arguments

[ninevra]

This PR adds the capability to detect and fix many cases where the user has called an assertion function with the expected value first and the actual value second. Closes #159. This is a breaking change.

If the first argument to t.deepEqual(), t.is(), t.not(), t.notDeepEqual(), t.throws(), or t.throwsAsync() is a static expression, such as {a: [1, 2, 3]}.['a'], and the second argument is a non-static expression, then the assertion-arguments rule reports an error.

Similarly, if the first argument to t.assert(), t.false(), t.falsy(), t.true(), or t.truthy() is a binary relation (e.g. 1 < 2), the left side of the relation is a static expression, and the right side of the relation is a non-static expression, then the assertion-arguments rule reports an error.

These errors are usually fixable; the autofix swaps the first and second arguments to the assertion. For the single-argument assertions, the relational operator is inverted if appropriate. They are not fixable if the autofix would change the relationship between either argument and any comment.

This should be a sound means of detecting expected arguments because supplying a static actual argument asserts nothing about the code under test. That is, t.deepEqual({a: 1}, expected); tests nothing.

Static values are computed using eslint-utils' getStaticValue() function, which seems quite comprehensive. eslint-utils was already in the dependency graph of eslint-plugin-ava; this PR lifts it to top-level.

The PR incidentally adds assertion-arguments support for t.throwsAsync(), t.notThrowsAsync(), and t.assert(). t.throwsAsync() and t.assert() are compatible with the new functionality.

Support for t.like() is blocked by PR #306. Merging #306 will enable t.like() support, and this branch can then add tests for t.like() support.

Potentially interesting design decisions:

• Extend assertion-arguments rather than adding a new rule, and don't provide an option to disable the check. This is per the previous discussion in Rule proposal: Put actual values first and expected values second in assertion #159.
• Do support single-argument assertions when the argument is a binary relation. This is motivated by power-assert, per the proposal in Rule proposal: Put actual values first and expected values second in assertion #159.
• Don't autofix cases where any comments are present around the target values. For example, t.is(/* left comment */ 'static', dynamic /* right comment */) produces an error, but does not provide a fix.
  • Moving text around in the presence of comments is dangerous and complex.
    • Line comments (// comment) must not be placed before any other tokens.
    • Flow type cast comments must be moved with their associated value, or else they'll end up casting the other argument.
    • Eslint directive comments must not be moved, or else suffer various kinds of missense. /* eslint-disable */ and /* eslint-enable */ are particularly problematic.
    • Other directive comments might have arbitrary behavior; enumerating, detecting, and handling them all is impossible.
• Don't consider function-valued expressions to be static. This is useful, though not strictly necessary, in supporting t.throws() and t.throwsAsync(), where the thrower (analogous to actual) is often a function. In t.throws(() => {}, TypeError); the left argument is static-valued (technically, all function expressions which don't close over dynamic values are static-valued) and the right is non-static. There is a mistake here, in that () => {} tests nothing, but the arguments are clearly not out of order. Currently, eslint-utils#getStaticValue() doesn't compute static-valued functions, but I've added a type-check in case a future minor version of it does.
• Don't consider well-known identifiers (undefined, NaN, Infinity) to be static. These are non-configurable, non-writable data properties of the global object, and can be shadowed in nested scopes by non-static values.

Especially questionable design decisions:

• Do consider expressions with side effects (e.g. a = 1 or void a()) to be static.
  • The principle that static-valued expressions shouldn't be passed as actual arguments is still sound here, but expected arguments also shouldn't have side effects, so something weirder than out-of-order arguments might be going on in these cases.
  • Detection of side effects is available in eslint-utils, but it either has false positives (reporting side effects on pure static MemberExpresssions) or false negatives (reporting void ident.prop as pure, even though ident.prop could be a getter or proxy trap), depending on configuration.

Potential further improvements:

• Track variable identifiers back to their declaration to find a static value.
  • Only non-shadowed non-configurable immutable globals and non-shadowed const-declared immutable values could feasibly be proven static.
  • This could enable detection of many common, likely static values, such as undefined and const expected = 'static'; t.is(expected, actual);.
  • This functionality is offered by eslint-utils; however, its implementation has multiple major issues.
    • The algorithm used by eslint-utils trusts the user not to redefine globals like undefined or RegExp. If the user does this, some identifiers will be erroneously treated as static, and reports might be issued in error.
    • The algorithm used by eslint-utils treats mutable values as static whenever the initial declaration is static, and doesn't check for future modifications to the value. So, const dynamic = {a: 1}; dynamic.b = foo(); is erroneously reported as static. This could cause reports to be issued in error.
    • For some reason, enabling scope resolution in eslint-utils breaks the p-queue integration test; linting p-queue's test/test.ts thereafter errors with Expected `index` to be a number. Occurred while linting <...>/p-queue/test/test.ts:672. I have not been able to identify the cause.

🦄

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #159: Rule proposal: Put actual values first and expected values second in assertion

---

[novemberborn]

@ninevra wow, a lot of effort must have gone into this! I wasn't online much this weekend so I haven't been able to look at this yet, sorry.

[novemberborn]

@ninevra the code looks great to me… but I'm actually not all that familiar with our ESLint plugin. @sindresorhus would you be able to have a look?

I'm happy with the design decisions. Thank you for calling them out so explicitly.

I believe this is a breaking change? That's fine, we may as well make ESLint 7 the minimal version, but if you could confirm that'd be great 😄

[ninevra]

Rebased on v10.5.0 and pushed tests for t.like() support.

@novemberborn, I agree this is a breaking change. It changes the behavior of an existing rule in a manner that is always enabled and could result in new reports without any change to the user's codebase. Users updating could see new reports, possibly breaking their CI; therefore, this should be a major version.

If shipping this in a minor version is desired, the new behavior could be put behind an option or moved to an entirely new rule (and defaulted to off or warn in the recommended config). I implemented this as a breaking change because that appeared to be the consensus in #159 (comment).

[novemberborn]

I agree this is a breaking change.

Thank for clarifying. I'll make sure we ship it as such.

[novemberborn]

@sindresorhus would you have time to take a look at this?

[sindresorhus]

Track variable identifiers back to their declaration to find a static value.

Is this something you intend to work on in this PR?

The algorithm used by eslint-utils trusts the user not to redefine globals like undefined or RegExp. If the user does this, some identifiers will be erroneously treated as static, and reports might be issued in error.

I think that's totally fine and pragmatic. This is an extreme edge-case nobody should be doing. It's not worth caring about.

The algorithm used by eslint-utils treats mutable values as static whenever the initial declaration is static, and doesn't check for future modifications to the value. So, const dynamic = {a: 1}; dynamic.b = foo(); is erroneously reported as static. This could cause reports to be issued in error.

You should open an issue on eslint-utils about that.

For some reason, enabling scope resolution in eslint-utils breaks the p-queue integration test; linting p-queue's test/test.ts thereafter errors with Expected index to be a number. Occurred while linting <...>/p-queue/test/test.ts:672. I have not been able to identify the cause.

I guess it's an incompatibility with the TypeScript parser.

[ninevra]

Track variable identifiers back to their declaration to find a static value.

Is this something you intend to work on in this PR?

I can if you think it's warranted. I hadn't planned to work on it in this PR, because ideally the solution would involve upstreaming fixes for typescript parser compatibility and safer variable resolution in eslint-utils, which could take an unknown/uncontrollable amount of time. After that turning on the feature would be a one- or two-line change. If you want me to add this functionality now, I can copy eslint-utils' implementation into this package's utilities, work on fixing its issues, and try to upstream those changes afterwards.

The algorithm used by eslint-utils treats mutable values as static whenever the initial declaration is static, and doesn't check for future modifications to the value. So, const dynamic = {a: 1}; dynamic.b = foo(); is erroneously reported as static. This could cause reports to be issued in error.

You should open an issue on eslint-utils about that.

I've already opened such an issue at mysticatea/eslint-utils#10.

[sindresorhus]

I think that could be useful, yes, but it's not worth blocking this PR. It can be done later on when the upstream changes are done.

[sindresorhus]

Thanks for your great work on this, @ninevra 👍🏻

[novemberborn]

Great!

When we land #310 we can ship the next major version.

[ninevra]

I've isolated the cause of the p-queue integration test failure. The cause was my use of the token.start and token.end properties, which are present in the AST produced by espree but not in that produced by @typescript-eslint/parser. Changing to token.range[0] and token.range[1] as @sindresorhus requested fixed the issue.

Turning on variable resolution only appeared to trigger the problem because p-queue contains an example of mysticatea/eslint-utils#10 (eslint-utils falsely identifying a value as static).

detailed explanation

p-queue test/test.ts lines 648-689:

test('.add() - throttled 10, concurrency 5', async t => {
	const result: number[] = [];

	const queue = new PQueue({
		concurrency: 5,
		intervalCap: 10,
		interval: 1000,
		autoStart: false
	});

	const firstValue = [...new Array(5).keys()];
	const secondValue = [...new Array(10).keys()];
	const thirdValue = [...new Array(13).keys()];
	thirdValue.forEach(async value => queue.add(async () => {
		await delay(300);
		result.push(value);
	}));

	queue.start();

	t.deepEqual(result, []);

	(async () => {
		await delay(400);
		t.deepEqual(result, firstValue);
		t.is(queue.pending, 5);
	})();

	(async () => {
		await delay(700);
		t.deepEqual(result, secondValue);
	})();

	(async () => {
		await delay(1200);
		t.is(queue.pending, 3);
		t.deepEqual(result, secondValue);
	})();

	await delay(1400);
	t.deepEqual(result, thirdValue);
});

With scope resolution off, eslint-utils considers result, firstValue, secondValue, thirdValue to be dynamic, since they're identifiers. With scope resolution on, eslint-utils still considers firstValue etc dynamic (because it doesn't simulate new Array()), but incorrectly believes result is static because its initializer was static (const result: number[] = [];), not seeing the later mutation of its value (result.push(value);). This causes the rule to change t.deepEqual(result, firstValue); to t.deepEqual(firstValue, result);, and in so doing, to run the bugged code.

TL;DR: there was no typescript compatibility error and this issue is already fixed in master.

The only remaining issue blocking identifier resolution is mysticatea/eslint-utils#10.