lint for redeclarations (#541)

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<string, { kind: VarKind; used: boolean; node: HasLocation | null }>;
+ declare strictScopes: Set<string>[];
  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 <del>_x_</del><ins>_y_</ins>, 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<UnderscoreNode> = 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 {

test/expr-parser.js

@@ -99,9 +99,9 @@ describe('expression parsing', () => {
  await assertLintFree(`
  <emu-alg>
  1. Let _x_ be a new Record { [[Foo]]: 0, <!-- comment --> [[Bar]]: 1 }.
- 1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1</ins> }.
- 1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1, [[Baz]]: 2</ins> }.
- 1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1,</ins> [[Baz]]: 2 }.
+ 1. Set _x_ to a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1</ins> }.
+ 1. Set _x_ to a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1, [[Baz]]: 2</ins> }.
+ 1. Set _x_ to a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1,</ins> [[Baz]]: 2 }.
  1. Use _x_.
  </emu-alg>
  `);

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`
+ <emu-alg>
+ 1. Let _x_ be 0.
+ 1. Let ${M}_x_ be 1.
+ 1. Return _x_.
+ </emu-alg>
+ </emu-clause>
+ `,
+ {
+ ruleId: 're-declaration',
+ nodeType: 'emu-alg',
+ message: '"x" is already declared',
+ }
+ );
+ });
+
+ it('redeclaration at an inner level is an error', async () => {
+ await assertLint(
+ positioned`
+ <emu-alg>
+ 1. Let _x_ be 0.
+ 1. Repeat,
+ 1. Let ${M}_x_ be 1.
+ 1. Return _x_.
+ </emu-alg>
+ </emu-clause>
+ `,
+ {
+ ruleId: 're-declaration',
+ nodeType: 'emu-alg',
+ message: '"x" is already declared',
+ }
+ );
+ });
+
+ it('multi-line if-else does not count as redeclaration', async () => {
+ await assertLintFree(
+ `
+ <emu-alg>
+ 1. If condition, then
+ 1. Let _result_ be 0.
+ 1. Else,
+ 1. Let _result_ be 1.
+ 1. Return _result_.
+ </emu-alg>
+ `
+ );
+ });
+
+ it('single-line if-else does not count as redeclaration', async () => {
+ await assertLintFree(
+ `
+ <emu-alg>
+ 1. If condition, let _result_ be 0.
+ 1. Else, let _result_ be 1.
+ 1. Return _result_.
+ </emu-alg>
+ `
+ );
+ });
+
+ it('[declared] annotation can be redeclared', async () => {
+ await assertLintFree(
+ `
+ <emu-alg>
+ 1. [declared="var"] NOTE: Something about _var_.
+ 1. Let _var_ be 0.
+ 1. Return _var_.
+ </emu-alg>
+ `
+ );
+ });
+
+ it('variables mentioned in the premable can be redeclared', async () => {
+ await assertLintFree(
+ `
+ <emu-clause id="example">
+ <h1>Example</h1>
+ <p>In the following algorithm, _var_ is a variable.</p>
+ <emu-alg>
+ 1. Let _var_ be 0.
+ 1. Return _var_.
+ </emu-alg>
+ </emu-clause>
+ `
+ );
+ });
+});

test/typecheck.js

@@ -131,12 +131,12 @@ describe('typechecking completions', () => {
  </dl>
  <emu-alg>
  1. Let _a_ be Completion(ExampleAlg()).
- 1. Let _a_ be Completion(<emu-meta suppress-effects="user-code">ExampleAlg()</emu-meta>).
+ 1. Set _a_ to Completion(<emu-meta suppress-effects="user-code">ExampleAlg()</emu-meta>).
  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.
  </emu-alg>