TSAbstractClassDeclaration are not defined in the scope (no-undef)

[saifelse]

What version of TypeScript are you using? 3.1.3

What version of typescript-eslint-parser are you using? 21.0.2

What code were you trying to parse?

export abstract class Foo {}
export class FooBar extends Foo {}

What did you expect to happen? No lint errors

What happened?

2:29 error 'Foo' is not defined no-undef

While I understand that Typescript already has scope checks that seem to make no-undef unnecessary and can be disabled, it has been useful to ensure that name and length are not accidentally used, see: https://github.com/Microsoft/TypeScript/issues/18433

Potential Fix 1

eslint-scope/lib/referencer.js:Referencer.visitClass adds a definition into the scope for "ClassDeclaration", which does not happen for "TSAbstractClassDeclaration". I think this can be fixed by doing:

TSAbstractClassDeclaration(node) {
    this.currentScope().__define(
        node.id,
        new Definition("ClassName", node.id, node, null, null, null)
    );
    this.ClassDeclaration(node);
}

However this then causes:

1:23 error 'Foo' is already declared in the upper scope no-shadow

Because of this special casing of ClassDeclaration in eslint/lib/rules/no-shadow.js:

/**
 * Checks if a variable of the class name in the class scope of ClassDeclaration.
 *
 * ClassDeclaration creates two variables of its name into its outer scope and its class scope.
 * So we should ignore the variable in the class scope.
 *
 * @param {Object} variable The variable to check.
 * @returns {boolean} Whether or not the variable of the class name in the class scope of ClassDeclaration.
 */
function isDuplicatedClassNameVariable(variable) {
    const block = variable.scope.block;

    return block.type === "ClassDeclaration" && block.id === variable.identifiers[0];
}

From here, I suppose you could just say that no-shadow should be disabled on typescript files, in favor of using tslint's no-shadowed-variables

Potential Fix 2

In typescript-eslint-parser/parser.js, we can simply re-map the abstract class node:

case "TSAbstractClassDeclaration":
    node.type = 'ClassDeclaration';
    break;

which to me seems pretty clean, since as far as eslint is concerned, the abstract keyword doesn't affect anything.

[armano2]

@saifelse i was thinking about making this change directly in estree with additional field "abstract" set to true, to be able to detect that this is an abstract class

[JamesHenry]

This issue has been migrated to the new project here: typescript-eslint/typescript-eslint#20

Thanks!