diff --git a/src/lint/rules/variable-use-def.ts b/src/lint/rules/variable-use-def.ts index b2addcc9..3bef1a6f 100644 --- a/src/lint/rules/variable-use-def.ts +++ b/src/lint/rules/variable-use-def.ts @@ -9,6 +9,26 @@ import type { Reporter } from '../algorithm-error-reporter-type'; import { Seq, walk as walkExpr } from '../../expr-parser'; import { offsetToLineAndColumn } from '../../utils'; +/* +Ecmaspeak scope rules are a bit weird. +Variables can be declared across branches and used subsequently, as in + +1. If condition, then + 1. Let _var_ be true. +1. Else, + 1. Let _var_ be false. +1. Use _var_. + +So it mostly behaves like `var`-declared variables in JS. + +(Exception: loop variables can't be used outside the loop.) + +But for readability reasons, we don't want to allow redeclaration or shadowing. + +The `Scope` class tracks names as if they are `var`-scoped, and the `strictScopes` property is +used to track names as if they are `let`-scoped, so we can warn on redeclaration. +*/ + type HasLocation = { location: { start: { line: number; column: number } } }; type VarKind = | 'parameter' @@ -19,14 +39,17 @@ type VarKind = | 'attribute declaration'; class Scope { declare vars: Map; + declare strictScopes: Set[]; declare report: Reporter; constructor(report: Reporter) { this.vars = new Map(); + this.strictScopes = [new Set()]; + this.report = report; + // TODO remove this when regex state objects become less dumb for (const name of ['captures', 'input', 'startIndex', 'endIndex']) { - this.declare(name, null); + this.declare(name, null, undefined, true); } - this.report = report; } declared(name: string): boolean { @@ -38,11 +61,32 @@ class Scope { return this.vars.get(name)!.used; } - declare(name: string, nameNode: HasLocation | null, kind: VarKind = 'variable'): void { + declare( + name: string, + nameNode: HasLocation | null, + kind: VarKind = 'variable', + mayBeShadowed: boolean = false + ): void { if (this.declared(name)) { - return; + if (!mayBeShadowed) { + for (const scope of this.strictScopes) { + if (scope.has(name)) { + this.report({ + ruleId: 're-declaration', + message: `${JSON.stringify(name)} is already declared`, + line: nameNode!.location.start.line, + column: nameNode!.location.start.column, + }); + return; + } + } + } + } else { + this.vars.set(name, { kind, used: false, node: nameNode }); + } + if (!mayBeShadowed) { + this.strictScopes[this.strictScopes.length - 1].add(name); } - this.vars.set(name, { kind, used: false, node: nameNode }); } undeclare(name: string) { @@ -100,7 +144,7 @@ export function checkVariableUsage( ) { // `__` is for _x__y_, which has textContent `_x__y_` for (const name of preceding.textContent.matchAll(/(?<=\b|_)_([a-zA-Z0-9]+)_(?=\b|_)/g)) { - scope.declare(name[1], null); + scope.declare(name[1], null, undefined, true); } } preceding = previousOrParent(preceding, parentClause); @@ -145,6 +189,7 @@ function walkAlgorithm( } return; } + scope.strictScopes.push(new Set()); stepLoop: for (const step of steps.contents) { const loopVars: Set = new Set(); @@ -175,7 +220,12 @@ function walkAlgorithm( column, }); } else { - scope.declare(name, { location: { start: { line, column } } }, 'attribute declaration'); + scope.declare( + name, + { location: { start: { line, column } } }, + 'attribute declaration', + true + ); } } } @@ -327,8 +377,12 @@ function walkAlgorithm( (isSuchThat && cur.name === 'text' && !/(?: of | in )/.test(cur.contents)) || (isBe && cur.name === 'text' && /\blet (?:each of )?$/i.test(cur.contents)) ) { + const conditional = + expr.items[0].name === 'text' && + /^(If|Else|Otherwise)\b/.test(expr.items[0].contents); + for (const v of varsDeclaredHere) { - scope.declare(v.contents, v); + scope.declare(v.contents, v, 'variable', conditional); declaredThisLine.add(v); } } @@ -353,6 +407,7 @@ function walkAlgorithm( scope.undeclare(decl.contents); } } + scope.strictScopes.pop(); } function isVariable(node: UnderscoreNode | { name: string } | null): node is UnderscoreNode { diff --git a/test/expr-parser.js b/test/expr-parser.js index 56cd57b0..c9825d91 100644 --- a/test/expr-parser.js +++ b/test/expr-parser.js @@ -99,9 +99,9 @@ describe('expression parsing', () => { await assertLintFree(` 1. Let _x_ be a new Record { [[Foo]]: 0, [[Bar]]: 1 }. - 1. Let _x_ be a new Record { [[Foo]]: 0, [[Bar]]: 1 }. - 1. Let _x_ be a new Record { [[Foo]]: 0, [[Bar]]: 1, [[Baz]]: 2 }. - 1. Let _x_ be a new Record { [[Foo]]: 0, [[Bar]]: 1, [[Baz]]: 2 }. + 1. Set _x_ to a new Record { [[Foo]]: 0, [[Bar]]: 1 }. + 1. Set _x_ to a new Record { [[Foo]]: 0, [[Bar]]: 1, [[Baz]]: 2 }. + 1. Set _x_ to a new Record { [[Foo]]: 0, [[Bar]]: 1, [[Baz]]: 2 }. 1. Use _x_. `); diff --git a/test/lint-variable-use-def.js b/test/lint-variable-use-def.js index 18007e90..72b9703b 100644 --- a/test/lint-variable-use-def.js +++ b/test/lint-variable-use-def.js @@ -497,3 +497,95 @@ describe('variables are declared and used appropriately', () => { }); }); }); + +describe('variables cannot be redeclared', () => { + it('redeclaration at the same level is an error', async () => { + await assertLint( + positioned` + + 1. Let _x_ be 0. + 1. Let ${M}_x_ be 1. + 1. Return _x_. + + + `, + { + ruleId: 're-declaration', + nodeType: 'emu-alg', + message: '"x" is already declared', + } + ); + }); + + it('redeclaration at an inner level is an error', async () => { + await assertLint( + positioned` + + 1. Let _x_ be 0. + 1. Repeat, + 1. Let ${M}_x_ be 1. + 1. Return _x_. + + + `, + { + ruleId: 're-declaration', + nodeType: 'emu-alg', + message: '"x" is already declared', + } + ); + }); + + it('multi-line if-else does not count as redeclaration', async () => { + await assertLintFree( + ` + + 1. If condition, then + 1. Let _result_ be 0. + 1. Else, + 1. Let _result_ be 1. + 1. Return _result_. + + ` + ); + }); + + it('single-line if-else does not count as redeclaration', async () => { + await assertLintFree( + ` + + 1. If condition, let _result_ be 0. + 1. Else, let _result_ be 1. + 1. Return _result_. + + ` + ); + }); + + it('[declared] annotation can be redeclared', async () => { + await assertLintFree( + ` + + 1. [declared="var"] NOTE: Something about _var_. + 1. Let _var_ be 0. + 1. Return _var_. + + ` + ); + }); + + it('variables mentioned in the premable can be redeclared', async () => { + await assertLintFree( + ` + +

Example

+

In the following algorithm, _var_ is a variable.

+ + 1. Let _var_ be 0. + 1. Return _var_. + +
+ ` + ); + }); +}); diff --git a/test/typecheck.js b/test/typecheck.js index 0d9ed66f..c2d2a7fe 100644 --- a/test/typecheck.js +++ b/test/typecheck.js @@ -131,12 +131,12 @@ describe('typechecking completions', () => { 1. Let _a_ be Completion(ExampleAlg()). - 1. Let _a_ be Completion(ExampleAlg()). + 1. Set _a_ to Completion(ExampleAlg()). 1. Set _a_ to ! ExampleAlg(). 1. Return ? ExampleAlg(). 1. Let _foo_ be 0. - 1. Let _a_ be Completion(ExampleSDO of _foo_). - 1. Let _a_ be Completion(ExampleSDO of _foo_ with argument 0). + 1. Set _a_ to Completion(ExampleSDO of _foo_). + 1. Set _a_ to Completion(ExampleSDO of _foo_ with argument 0). 1. If ? ExampleSDO of _foo_ is *true*, then 1. Something.