From 51f2920d1df3c31e2baac33b21f3ce8f7abf1063 Mon Sep 17 00:00:00 2001 From: Armano Date: Sun, 20 Jan 2019 02:55:27 +0100 Subject: [PATCH 1/2] feat(parser): validate options passed to parser --- packages/parser/README.md | 6 +- packages/parser/src/parser-options.ts | 23 +++--- packages/parser/src/parser.ts | 90 +++++++++++++++++------- packages/parser/tests/lib/comments.ts | 6 +- packages/parser/tests/lib/jsx.ts | 4 +- packages/parser/tests/lib/parser.ts | 14 +++- packages/parser/tests/lib/tsx.ts | 16 +++-- packages/typescript-estree/src/parser.ts | 8 +-- 8 files changed, 118 insertions(+), 49 deletions(-) diff --git a/packages/parser/README.md b/packages/parser/README.md index 253ed00e4686..dcdd4e3ba919 100644 --- a/packages/parser/README.md +++ b/packages/parser/README.md @@ -32,7 +32,7 @@ By far the most common case will be installing the [@typescript-eslint/eslint-pl The following additional configuration options are available by specifying them in [`parserOptions`](https://eslint.org/docs/user-guide/configuring#specifying-parser-options) in your ESLint configuration file. -- **`jsx`** - default `false`. Enable parsing JSX when `true`. More details can be found [here](https://www.typescriptlang.org/docs/handbook/jsx.html). +- **`ecmaFeatures.jsx`** - default `false`. Enable parsing JSX when `true`. More details can be found [here](https://www.typescriptlang.org/docs/handbook/jsx.html). - It's `false` on `*.ts` files regardless of this option. - It's `true` on `*.tsx` files regardless of this option. @@ -46,7 +46,9 @@ The following additional configuration options are available by specifying them { "parser": "@typescript-eslint/parser", "parserOptions": { - "jsx": true, + "ecmaFeatures": { + "jsx": true + }, "useJSXTextNode": true } } diff --git a/packages/parser/src/parser-options.ts b/packages/parser/src/parser-options.ts index 6bb6b160bca7..95d6e54c182c 100644 --- a/packages/parser/src/parser-options.ts +++ b/packages/parser/src/parser-options.ts @@ -1,17 +1,20 @@ export interface ParserOptions { - useJSXTextNode?: boolean; - loc?: true; - range?: true; - tokens?: true; - filePath?: string; + loc?: boolean; + comment?: boolean; + range?: boolean; + tokens?: boolean; sourceType?: 'script' | 'module'; ecmaVersion?: number; ecmaFeatures?: { globalReturn?: boolean; + jsx?: boolean; }; - /** - * @deprecated We should finalize the work from - * https://github.com/eslint/typescript-eslint-parser#595 - */ - jsx?: boolean; + // ts-estree specific + filePath?: string; + project?: string | string[]; + useJSXTextNode?: boolean; + errorOnUnknownASTType?: boolean; + errorOnTypeScriptSyntacticAndSemanticIssues?: boolean; + tsconfigRootDir?: string; + extraFileExtensions?: string[]; } diff --git a/packages/parser/src/parser.ts b/packages/parser/src/parser.ts index 49aabf26074a..03f26d064962 100644 --- a/packages/parser/src/parser.ts +++ b/packages/parser/src/parser.ts @@ -1,5 +1,9 @@ import traverser from 'eslint/lib/util/traverser'; -import * as typescriptESTree from '@typescript-eslint/typescript-estree'; +import { + AST_NODE_TYPES, + parseAndGenerateServices, + ParserOptions as ParserOptionsTsESTree +} from '@typescript-eslint/typescript-estree'; import { analyzeScope } from './analyze-scope'; import { ParserOptions } from './parser-options'; import { visitorKeys } from './visitor-keys'; @@ -20,52 +24,88 @@ interface ParseForESLintResult { scopeManager: ReturnType; } +function validateBoolean( + value: boolean | undefined, + fallback: boolean = false +): boolean { + if (typeof value !== 'boolean') { + return fallback; + } + return value; +} + +//------------------------------------------------------------------------------ +// Public +//------------------------------------------------------------------------------ + export const version = packageJSON.version; +export const Syntax = Object.freeze(AST_NODE_TYPES); + export function parse(code: string, options?: ParserOptions) { return parseForESLint(code, options).ast; } -export const Syntax = Object.freeze(typescriptESTree.AST_NODE_TYPES); - -export function parseForESLint( +export function parseForESLint( code: string, - options?: T | null + options?: ParserOptions | null ): ParseForESLintResult { - if (typeof options !== 'object' || options === null) { - options = { useJSXTextNode: true } as T; - } else if (typeof options.useJSXTextNode !== 'boolean') { - options = Object.assign({}, options, { useJSXTextNode: true }); - } - if (typeof options.filePath === 'string') { - const tsx = options.filePath.endsWith('.tsx'); - if (tsx || options.filePath.endsWith('.ts')) { - options = Object.assign({}, options, { jsx: tsx }); - } + if (!options || typeof options !== 'object') { + options = {}; } - // https://eslint.org/docs/user-guide/configuring#specifying-parser-options // if sourceType is not provided by default eslint expect that it will be set to "script" - options.sourceType = options.sourceType || 'script'; if (options.sourceType !== 'module' && options.sourceType !== 'script') { options.sourceType = 'script'; } + if (typeof options.ecmaFeatures !== 'object') { + options.ecmaFeatures = {}; + } + + const parserOptions: ParserOptionsTsESTree = { + useJSXTextNode: validateBoolean(options.useJSXTextNode, true), + range: validateBoolean(options.range, true), // TODO: this option is ignored by typescript-estree + loc: validateBoolean(options.loc, true), // TODO: this option is ignored by typescript-estree + tokens: validateBoolean(options.tokens, true), + jsx: validateBoolean(options.ecmaFeatures.jsx), + comment: validateBoolean(options.comment, true), + errorOnUnknownASTType: validateBoolean(options.errorOnUnknownASTType), + errorOnTypeScriptSyntacticAndSemanticIssues: validateBoolean( + options.errorOnTypeScriptSyntacticAndSemanticIssues + ) + }; + + if (options.project) { + parserOptions.project = options.project; + } + + if (typeof options.tsconfigRootDir === 'string') { + parserOptions.tsconfigRootDir = options.tsconfigRootDir; + } + + if (options.extraFileExtensions) { + parserOptions.extraFileExtensions = options.extraFileExtensions; + } + + if (typeof options.filePath === 'string') { + parserOptions.filePath = options.filePath; + + const tsx = options.filePath.endsWith('.tsx'); + if (tsx || options.filePath.endsWith('.ts')) { + parserOptions.jsx = tsx; + } + } - const { ast, services } = typescriptESTree.parseAndGenerateServices( - code, - options - ); + const { ast, services } = parseAndGenerateServices(code, parserOptions); ast.sourceType = options.sourceType; traverser.traverse(ast, { - enter: (node: any) => { + enter(node: any) { switch (node.type) { // Function#body cannot be null in ESTree spec. case 'FunctionExpression': if (!node.body) { - node.type = `TSEmptyBody${ - node.type - }` as typescriptESTree.AST_NODE_TYPES; + node.type = `TSEmptyBody${node.type}` as AST_NODE_TYPES; } break; // no default diff --git a/packages/parser/tests/lib/comments.ts b/packages/parser/tests/lib/comments.ts index bdf3356f3a4b..f2bb2260d651 100644 --- a/packages/parser/tests/lib/comments.ts +++ b/packages/parser/tests/lib/comments.ts @@ -15,8 +15,10 @@ describe('Comments', () => { testFiles.forEach(filename => { const code = fs.readFileSync(filename, 'utf8'); const config: ParserOptions = { - jsx: true, - sourceType: 'module' + sourceType: 'module', + ecmaFeatures: { + jsx: true + } }; it( testUtils.formatSnapshotName(filename, FIXTURES_DIR), diff --git a/packages/parser/tests/lib/jsx.ts b/packages/parser/tests/lib/jsx.ts index 55bb4e5f742f..05adef987f6f 100644 --- a/packages/parser/tests/lib/jsx.ts +++ b/packages/parser/tests/lib/jsx.ts @@ -28,7 +28,9 @@ describe('JSX', () => { const code = fs.readFileSync(filename, 'utf8'); const config = { useJSXTextNode, - jsx: true + ecmaFeatures: { + jsx: true + } }; it( testUtils.formatSnapshotName(filename, fixturesDir), diff --git a/packages/parser/tests/lib/parser.ts b/packages/parser/tests/lib/parser.ts index f5b1f7a9ac4a..b832e957702a 100644 --- a/packages/parser/tests/lib/parser.ts +++ b/packages/parser/tests/lib/parser.ts @@ -1,5 +1,6 @@ import * as typescriptESTree from '@typescript-eslint/typescript-estree'; import { parse, parseForESLint, Syntax } from '../../src/parser'; +import * as scope from '../../src/analyze-scope'; describe('parser', () => { it('parse() should return just the AST from parseForESLint()', () => { @@ -15,11 +16,22 @@ describe('parser', () => { it('parseForESLint() should set the sourceType to script, if an invalid one is provided', () => { const code = 'const valid = true;'; const spy = jest.spyOn(typescriptESTree, 'parseAndGenerateServices'); + const spyScope = jest.spyOn(scope, 'analyzeScope'); parseForESLint(code, { sourceType: 'foo' as any }); expect(spy).toHaveBeenCalledWith(code, { - sourceType: 'script', + comment: true, + errorOnTypeScriptSyntacticAndSemanticIssues: false, + errorOnUnknownASTType: false, + jsx: false, + loc: true, + range: true, + tokens: true, useJSXTextNode: true }); + expect(spyScope).toHaveBeenCalledWith(jasmine.any(Object), { + ecmaFeatures: {}, + sourceType: 'script' + }); }); it('Syntax should contain a frozen object of typescriptESTree.AST_NODE_TYPES', () => { diff --git a/packages/parser/tests/lib/tsx.ts b/packages/parser/tests/lib/tsx.ts index 0872edf307b0..a18278e80458 100644 --- a/packages/parser/tests/lib/tsx.ts +++ b/packages/parser/tests/lib/tsx.ts @@ -17,7 +17,9 @@ describe('TSX', () => { const code = fs.readFileSync(filename, 'utf8'); const config = { useJSXTextNode: true, - jsx: true + ecmaFeatures: { + jsx: true + } }; it( testUtils.formatSnapshotName(filename, FIXTURES_DIR, '.tsx'), @@ -53,7 +55,9 @@ describe('TSX', () => { const config = { parser: '@typescript-eslint/parser', parserOptions: { - jsx: true + ecmaFeatures: { + jsx: true + } } }; const messages = linter.verify(code, config); @@ -85,7 +89,9 @@ describe('TSX', () => { const config = { parser: '@typescript-eslint/parser', parserOptions: { - jsx: true + ecmaFeatures: { + jsx: true + } } }; const messages = linter.verify(code, config, { filename: 'test.ts' }); @@ -117,7 +123,9 @@ describe('TSX', () => { const config = { parser: '@typescript-eslint/parser', parserOptions: { - jsx: false + ecmaFeatures: { + jsx: false + } } }; const messages = linter.verify(code, config, { filename: 'test.tsx' }); diff --git a/packages/typescript-estree/src/parser.ts b/packages/typescript-estree/src/parser.ts index 1a39cfd2f31f..2f6347e0a7a2 100644 --- a/packages/typescript-estree/src/parser.ts +++ b/packages/typescript-estree/src/parser.ts @@ -350,10 +350,7 @@ function generateAST( // Public //------------------------------------------------------------------------------ -export { AST_NODE_TYPES } from './ast-node-types'; -export { version }; - -const version = packageJSON.version; +export const version: string = packageJSON.version; export function parse( code: string, @@ -378,3 +375,6 @@ export function parseAndGenerateServices(code: string, options: ParserOptions) { } }; } + +export { AST_NODE_TYPES } from './ast-node-types'; +export { ParserOptions }; From 390898c4beba91d42d0bac127e4265b5cd49b3bb Mon Sep 17 00:00:00 2001 From: Armano Date: Sun, 20 Jan 2019 03:37:45 +0100 Subject: [PATCH 2/2] feat(parser): remove redundant validation and add tests --- packages/parser/src/parser.ts | 29 +++------------- packages/parser/tests/lib/parser.ts | 54 +++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 31 deletions(-) diff --git a/packages/parser/src/parser.ts b/packages/parser/src/parser.ts index 03f26d064962..2dd32b383625 100644 --- a/packages/parser/src/parser.ts +++ b/packages/parser/src/parser.ts @@ -62,34 +62,13 @@ export function parseForESLint( options.ecmaFeatures = {}; } - const parserOptions: ParserOptionsTsESTree = { + const parserOptions: ParserOptionsTsESTree = {}; + Object.assign(parserOptions, options, { useJSXTextNode: validateBoolean(options.useJSXTextNode, true), - range: validateBoolean(options.range, true), // TODO: this option is ignored by typescript-estree - loc: validateBoolean(options.loc, true), // TODO: this option is ignored by typescript-estree - tokens: validateBoolean(options.tokens, true), - jsx: validateBoolean(options.ecmaFeatures.jsx), - comment: validateBoolean(options.comment, true), - errorOnUnknownASTType: validateBoolean(options.errorOnUnknownASTType), - errorOnTypeScriptSyntacticAndSemanticIssues: validateBoolean( - options.errorOnTypeScriptSyntacticAndSemanticIssues - ) - }; - - if (options.project) { - parserOptions.project = options.project; - } - - if (typeof options.tsconfigRootDir === 'string') { - parserOptions.tsconfigRootDir = options.tsconfigRootDir; - } - - if (options.extraFileExtensions) { - parserOptions.extraFileExtensions = options.extraFileExtensions; - } + jsx: validateBoolean(options.ecmaFeatures.jsx) + }); if (typeof options.filePath === 'string') { - parserOptions.filePath = options.filePath; - const tsx = options.filePath.endsWith('.tsx'); if (tsx || options.filePath.endsWith('.ts')) { parserOptions.jsx = tsx; diff --git a/packages/parser/tests/lib/parser.ts b/packages/parser/tests/lib/parser.ts index b832e957702a..bfc23bac5abb 100644 --- a/packages/parser/tests/lib/parser.ts +++ b/packages/parser/tests/lib/parser.ts @@ -19,13 +19,9 @@ describe('parser', () => { const spyScope = jest.spyOn(scope, 'analyzeScope'); parseForESLint(code, { sourceType: 'foo' as any }); expect(spy).toHaveBeenCalledWith(code, { - comment: true, - errorOnTypeScriptSyntacticAndSemanticIssues: false, - errorOnUnknownASTType: false, + ecmaFeatures: {}, jsx: false, - loc: true, - range: true, - tokens: true, + sourceType: 'script', useJSXTextNode: true }); expect(spyScope).toHaveBeenCalledWith(jasmine.any(Object), { @@ -34,6 +30,52 @@ describe('parser', () => { }); }); + it('parseAndGenerateServices() should be called with options', () => { + const code = 'const valid = true;'; + const spy = jest.spyOn(typescriptESTree, 'parseAndGenerateServices'); + parseForESLint(code, { + loc: false, + comment: false, + range: false, + tokens: false, + sourceType: 'module', + ecmaVersion: 10, + ecmaFeatures: { + globalReturn: false, + jsx: false + }, + // ts-estree specific + filePath: 'test/foo', + project: 'tsconfig.json', + useJSXTextNode: false, + errorOnUnknownASTType: false, + errorOnTypeScriptSyntacticAndSemanticIssues: false, + tsconfigRootDir: '../../', + extraFileExtensions: ['foo'] + }); + expect(spy).toHaveBeenCalledWith(code, { + jsx: false, + loc: false, + comment: false, + range: false, + tokens: false, + sourceType: 'module', + ecmaVersion: 10, + ecmaFeatures: { + globalReturn: false, + jsx: false + }, + // ts-estree specific + filePath: 'test/foo', + project: 'tsconfig.json', + useJSXTextNode: false, + errorOnUnknownASTType: false, + errorOnTypeScriptSyntacticAndSemanticIssues: false, + tsconfigRootDir: '../../', + extraFileExtensions: ['foo'] + }); + }); + it('Syntax should contain a frozen object of typescriptESTree.AST_NODE_TYPES', () => { expect(Syntax).toEqual(typescriptESTree.AST_NODE_TYPES); expect(