lint for used-but-not-declared and declared-but-not-used variables in algorithms

[bakkot]

Based on #482. Fixes #129.

Given tc39/ecma262#2896 and tc39/ecma262@5f759fa, this passes on ecma262 except for a few genuinely unused declarations, which I'll fix as part of merging this.

This is pretty conservative (e.g. it assumes any variable referred to in a previous paragraph in the clause is defined, to handle stuff like this), but seems to work well. In particular it handles abstract closures correctly (variables must be captured from the outer scope, captures count as using variables from the outer scope).

[michaelficarra]

aww, I was looking forward to seeing how you did this

[bakkot]

It's not that hard - you know exactly the step being replaced, so you can swap out that step in the original and re-run this algorithm, discarding any warnings which don't apply to the replacement section - but it just doesn't come up enough to be worth worrying about.

[michaelficarra]

break here so you don't try to declare the loop target or anything mentioned in the guard

[michaelficarra]

We should also probably have a test for that.

[bakkot]

We don't want to declare anything else, but we do want to count references on the rest of the line as uses, and just putting a continue here would skip that. What test are you imagining continueing here would fix?

[michaelficarra]

1. For each integer _a_ of _b_ such that _c_ relates to _a_ in some way, do

[bakkot]

That case is guarded explicitly:

ecmarkup/src/lint/rules/variable-use-def.ts

Lines 293 to 297 in 23e5a62

	if (
	// "of"/"in" guard is to distinguish "an integer X such that" from "an integer X in Y such that" - latter should not declare Y
	(isSuchThat && cur.name === 'text' && !/(?: of | in )/.test(cur.contents)) ||
	(isBe && cur.name === 'text' && /\blet (?:each of )?$/i.test(cur.contents))
	) {

.

[bakkot]

There's a test for it too:

ecmarkup/test/lint-variable-use-def.js

Lines 102 to 116 in 23e5a62

	it('"there exists _x_ in _y_ such that" declares _x_ but not _y_"', async () => {
	await assertLint(
	positioned`
	<emu-alg>
	1. If there exists an integer _x_ in ${M}_y_ such that _x_ < 10, return *true*.
	1. Return *false*.
	</emu-alg>
	`,
	{
	ruleId: 'use-before-def',
	nodeType: 'emu-alg',
	message: 'could not find a preceding declaration for "y"',
	}
	);
	});

Though not using the "for each" style; I can add one for that case, if you want.

[michaelficarra]

Yes, please.

[bakkot]

Done.

[michaelficarra]

I'd kind of rather get rid of these. We can leave this support for now, but can you open an issue on 262 that lists these cases so we can review them for phrasing?

[bakkot]

Not sure about an issue on 262, but here's a list for you.

• This one line in JSON.stringify which should just get fixed
• Module Record GetBindingValue step 3.a
• Number::toString step 5
• Number.prototype.toExponential step 10
• Number.prototype.toPrecision step 10

In Temporal there is also ParseTimeZoneOffsetString step 3.

[jmdyck]

(Note that PR #2288 takes care of Module Record GetBindingValue step 3.a.)

[michaelficarra]

Suggested change

	it('"such that" counts as a declaration"', async () => {
	it('"such that" counts as a use"', async () => {

[bakkot]

Nope, it's a declaration. For example

While _finalizationRegistry_.[[Cells]] contains a Record _cell_ such that _cell_.[[WeakRefTarget]] is ~empty~, [...]

declares _cell_.