Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(typescript-estree): added allowInvalidAST option to not throw on invalid tokens #6247

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion docs/architecture/TypeScript-ESTree.mdx
Expand Up @@ -48,6 +48,16 @@ interface ParseOptions {
*/
debugLevel?: boolean | ('typescript-eslint' | 'eslint' | 'typescript')[];

/**
* Causes the parser to an error if it receives an invalid AST from TypeScript.
* This case only usually occurs when attempting to lint invalid code.
*
* @remarks
* This is because TypeScript reports some syntax issues as semantic diagnostics.
* See https://github.com/typescript-eslint/typescript-eslint/issues/1852.
*/
errorOnInvalidAST?: boolean;

/**
* Cause the parser to error if it encounters an unknown AST node type (useful for testing).
* This case only usually occurs when TypeScript releases new features.
Expand Down Expand Up @@ -99,7 +109,7 @@ interface ParseOptions {

const PARSE_DEFAULT_OPTIONS: ParseOptions = {
comment: false,
errorOnUnknownASTType: false,

filePath: 'estree.ts', // or 'estree.tsx', if you pass jsx: true
jsx: false,
loc: false,
Expand Down
4 changes: 2 additions & 2 deletions packages/parser/tests/lib/parser.ts
Expand Up @@ -35,7 +35,7 @@ describe('parser', () => {
// ts-estree specific
filePath: 'isolated-file.src.ts',
project: 'tsconfig.json',
errorOnUnknownASTType: false,

errorOnTypeScriptSyntacticAndSemanticIssues: false,
tsconfigRootDir: 'tests/fixtures/services',
extraFileExtensions: ['.foo'],
Expand Down Expand Up @@ -89,7 +89,7 @@ describe('parser', () => {
// ts-estree specific
filePath: 'isolated-file.src.ts',
project: 'tsconfig.json',
errorOnUnknownASTType: false,

errorOnTypeScriptSyntacticAndSemanticIssues: false,
tsconfigRootDir: 'tests/fixtures/services',
extraFileExtensions: ['.foo'],
Expand Down
1 change: 1 addition & 0 deletions packages/typescript-estree/src/ast-converter.ts
Expand Up @@ -26,6 +26,7 @@ export function astConverter(
* Recursively convert the TypeScript AST into an ESTree-compatible AST
*/
const instance = new Converter(ast, {
errorOnInvalidAST: parseSettings.errorOnInvalidAST || false,
errorOnUnknownASTType: parseSettings.errorOnUnknownASTType || false,
shouldPreserveNodeMaps,
});
Expand Down
46 changes: 43 additions & 3 deletions packages/typescript-estree/src/convert.ts
Expand Up @@ -24,6 +24,7 @@ import {
isESTreeClassMember,
isOptional,
isThisInTypeQuery,
nodeHasIllegalDecorators,
unescapeStringLiteralText,
} from './node-utils';
import type {
Expand All @@ -37,8 +38,9 @@ import { AST_NODE_TYPES } from './ts-estree';
const SyntaxKind = ts.SyntaxKind;

interface ConverterOptions {
errorOnUnknownASTType: boolean;
shouldPreserveNodeMaps: boolean;
errorOnInvalidAST?: boolean;
errorOnUnknownASTType?: boolean;
shouldPreserveNodeMaps?: boolean;
}

/**
Expand Down Expand Up @@ -76,7 +78,7 @@ export class Converter {
* @param options additional options for the conversion
* @returns the converted ESTreeNode
*/
constructor(ast: ts.SourceFile, options: ConverterOptions) {
constructor(ast: ts.SourceFile, options: ConverterOptions = {}) {
this.ast = ast;
this.options = { ...options };
}
Expand Down Expand Up @@ -238,6 +240,22 @@ export class Converter {
return this.converter(child, parent, this.inTypeMode, false);
}

/**
* Converts a TypeScript child node into an ESTree node, throwing an error
* if the child is not defined and options.errorOnInvalidAST is true.
* @returns The converted ESTree node.
*/
private convertChildStrict(
child: ts.Node | undefined,
parent: ts.Node,
message: string,
): any | null {
if (!child && this.options.errorOnInvalidAST) {
throw createError(this.ast, parent.pos, message);
}
return this.converter(child, parent, this.inTypeMode, false);
}

/**
* Converts a TypeScript node into an ESTree node.
* @param child the child ts.Node
Expand Down Expand Up @@ -983,6 +1001,8 @@ export class Converter {
}

case SyntaxKind.VariableStatement: {
this.#checkIllegalDecorators(node);

const result = this.createNode<TSESTree.VariableDeclaration>(node, {
type: AST_NODE_TYPES.VariableDeclaration,
declarations: node.declarationList.declarations.map(el =>
Expand All @@ -991,6 +1011,14 @@ export class Converter {
kind: getDeclarationKind(node.declarationList),
});

if (this.options.errorOnInvalidAST && !result.declarations.length) {
throw createError(
this.ast,
node.pos,
'A variable declaration list must have at least one variable declarator.',
);
}

/**
* Semantically, decorators are not allowed on variable declarations,
* Pre 4.8 TS would include them in the AST, so we did as well.
Expand Down Expand Up @@ -2449,6 +2477,8 @@ export class Converter {
return this.convertChild(node.expression, parent);

case SyntaxKind.TypeAliasDeclaration: {
this.#checkIllegalDecorators(node);

const result = this.createNode<TSESTree.TSTypeAliasDeclaration>(node, {
type: AST_NODE_TYPES.TSTypeAliasDeclaration,
id: this.convertChild(node.name),
Expand Down Expand Up @@ -2605,6 +2635,8 @@ export class Converter {
}

case SyntaxKind.InterfaceDeclaration: {
this.#checkIllegalDecorators(node);

const interfaceHeritageClauses = node.heritageClauses ?? [];
const result = this.createNode<TSESTree.TSInterfaceDeclaration>(node, {
type: AST_NODE_TYPES.TSInterfaceDeclaration,
Expand Down Expand Up @@ -2695,6 +2727,8 @@ export class Converter {
}

case SyntaxKind.EnumDeclaration: {
this.#checkIllegalDecorators(node);

const result = this.createNode<TSESTree.TSEnumDeclaration>(node, {
type: AST_NODE_TYPES.TSEnumDeclaration,
id: this.convertChild(node.name),
Expand Down Expand Up @@ -2922,4 +2956,10 @@ export class Converter {
return this.deeplyCopy(node);
}
}

#checkIllegalDecorators(node: ts.Node): void {
if (this.options.errorOnInvalidAST && nodeHasIllegalDecorators(node)) {
throw createError(this.ast, node.pos, 'Decorators are not valid here.');
}
}
}
7 changes: 7 additions & 0 deletions packages/typescript-estree/src/node-utils.ts
Expand Up @@ -628,6 +628,13 @@ export function createError(
return new TSError(message, ast.fileName, start, loc.line + 1, loc.character);
}

export function nodeHasIllegalDecorators(node: ts.Node): boolean {
return !!(
'illegalDecorators' in node &&
(node.illegalDecorators as unknown[] | undefined)?.length
);
}

/**
* @param n the TSNode
* @param ast the TS AST
Expand Down
Expand Up @@ -41,6 +41,7 @@ export function createParseSettings(
: Array.isArray(options.debugLevel)
? new Set(options.debugLevel)
: new Set(),
errorOnInvalidAST: options.errorOnInvalidAST === true,
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
errorOnTypeScriptSyntacticAndSemanticIssues: false,
errorOnUnknownASTType: options.errorOnUnknownASTType === true,
EXPERIMENTAL_useSourceOfProjectReferenceRedirect:
Expand Down
5 changes: 5 additions & 0 deletions packages/typescript-estree/src/parseSettings/index.ts
Expand Up @@ -41,6 +41,11 @@ export interface MutableParseSettings {
*/
debugLevel: Set<DebugModule>;

/**
* Whether to throw an error when a required node child does not exist.
*/
errorOnInvalidAST: boolean;

/**
* Whether to error if TypeScript reports a semantic or syntactic error diagnostic.
*/
Expand Down
10 changes: 10 additions & 0 deletions packages/typescript-estree/src/parser-options.ts
Expand Up @@ -25,6 +25,16 @@ interface ParseOptions {
*/
debugLevel?: DebugLevel;

/**
* Causes the parser to an error if it sees that a required node child does not exist.
* This case only usually occurs when attempting to lint invalid code.
*
* @remarks
* This is because TypeScript reports some syntax issues as semantic diagnostics.
* See https://github.com/typescript-eslint/typescript-eslint/issues/1852.
*/
errorOnInvalidAST?: boolean;

/**
* Cause the parser to error if it encounters an unknown AST node type (useful for testing).
* This case only usually occurs when TypeScript releases new features.
Expand Down
87 changes: 58 additions & 29 deletions packages/typescript-estree/tests/lib/convert.test.ts
Expand Up @@ -26,20 +26,14 @@ describe('convert', () => {

ts.forEachChild(ast, fakeUnknownKind);

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);
expect(instance.convertProgram()).toMatchSnapshot();
});

it('deeplyCopy should convert node with decorators correctly', () => {
const ast = convertCode('@test class foo {}');

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);

expect(
instance['deeplyCopy'](ast.statements[0] as ts.ClassDeclaration),
Expand All @@ -49,10 +43,7 @@ describe('convert', () => {
it('deeplyCopy should convert node with type parameters correctly', () => {
const ast = convertCode('class foo<T> {}');

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);

expect(
instance['deeplyCopy'](ast.statements[0] as ts.ClassDeclaration),
Expand All @@ -62,10 +53,7 @@ describe('convert', () => {
it('deeplyCopy should convert node with type arguments correctly', () => {
const ast = convertCode('new foo<T>()');

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);

expect(
instance['deeplyCopy'](
Expand All @@ -78,10 +66,7 @@ describe('convert', () => {
it('deeplyCopy should convert array of nodes', () => {
const ast = convertCode('new foo<T>()');

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);
expect(instance['deeplyCopy'](ast)).toMatchSnapshot();
});

Expand All @@ -90,7 +75,6 @@ describe('convert', () => {

const instance = new Converter(ast, {
errorOnUnknownASTType: true,
shouldPreserveNodeMaps: false,
});

expect(() => instance['deeplyCopy'](ast)).toThrow(
Expand All @@ -107,7 +91,6 @@ describe('convert', () => {
`);

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: true,
});
instance.convertProgram();
Expand Down Expand Up @@ -140,7 +123,6 @@ describe('convert', () => {
const ast = convertCode(`<a.b.c.d.e></a.b.c.d.e>`);

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: true,
});
instance.convertProgram();
Expand Down Expand Up @@ -172,7 +154,6 @@ describe('convert', () => {
const ast = convertCode(`export function foo () {}`);

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: true,
});
const program = instance.convertProgram();
Expand Down Expand Up @@ -203,7 +184,6 @@ describe('convert', () => {
it('should correctly create node with range and loc set', () => {
const ast = convertCode('');
const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: true,
});

Expand Down Expand Up @@ -252,13 +232,62 @@ describe('convert', () => {
for (const code of jsDocCode) {
const ast = convertCode(code);

const instance = new Converter(ast, {
errorOnUnknownASTType: false,
shouldPreserveNodeMaps: false,
});
const instance = new Converter(ast);
expect(() => instance.convertProgram()).toThrow(
'JSDoc types can only be used inside documentation comments.',
);
}
});

describe('errorOnInvalidAST', () => {
function generateTest(title: string, code: string, error: string): void {
it(`does not throw an error for ${title} when errorOnInvalidAST is false`, () => {
const ast = convertCode(code);

const instance = new Converter(ast);

expect(() => instance.convertProgram()).not.toThrow();
});

it(`throws an error for ${title} when errorOnInvalidAST is true`, () => {
const ast = convertCode(code);

const instance = new Converter(ast, {
errorOnInvalidAST: true,
});

expect(() => instance.convertProgram()).toThrow(error);
});
}

generateTest(
'a decorator on an enum declaration',
'@decl let value;',
'Decorators are not valid here.',
);

generateTest(
'a decorator on an interface declaration',
'@decl interface _ {};',
'Decorators are not valid here.',
);

generateTest(
'a decorator on a type alias declaration',
'@decl type _ = {};',
'Decorators are not valid here.',
);

generateTest(
'a decorator on a variable statement',
'@decl let value;',
'Decorators are not valid here.',
);

generateTest(
'a variable declaration with no variables',
'const;',
'A variable declaration list must have at least one variable declarator.',
);
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
});
});
2 changes: 1 addition & 1 deletion packages/website/src/components/linter/config.ts
Expand Up @@ -7,7 +7,7 @@ export const parseSettings: ParseSettings = {
comments: [],
DEPRECATED__createDefaultProgram: false,
debugLevel: new Set(),
errorOnUnknownASTType: false,

extraFileExtensions: [],
filePath: '',
jsx: false,
Expand Down