From 6f7441dd803004492bc96b79292c80889c3a0ebd Mon Sep 17 00:00:00 2001 From: zoehneto Date: Thu, 22 Mar 2018 15:32:07 +0100 Subject: [PATCH 1/5] Extract createProgram from applyTypes to allow sharing with instrument --- package.json | 2 +- src/apply-types.ts | 35 ++--------------- ...-types.spec.ts => compiler-helper.spec.ts} | 10 ++--- src/compiler-helper.ts | 38 +++++++++++++++++++ 4 files changed, 47 insertions(+), 38 deletions(-) rename src/{apply-types.spec.ts => compiler-helper.spec.ts} (76%) create mode 100644 src/compiler-helper.ts diff --git a/package.json b/package.json index 8e28522..38d1c28 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "license": "MIT", "scripts": { "build": "rimraf dist && tsc && copyfiles -u 1 src/type-collector-snippet.ts dist", - "format": "prettier --write src/**/*.ts packages/**/src/**.ts", + "format": "prettier --write src/*.ts src/**/*.ts packages/**/src/**.ts", "precommit": "lint-staged", "prepublish": "npm run build", "lint": "tslint -p .", diff --git a/src/apply-types.ts b/src/apply-types.ts index b813187..d05defb 100644 --- a/src/apply-types.ts +++ b/src/apply-types.ts @@ -2,6 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as ts from 'typescript'; +import { getProgram, ICompilerOptions } from './compiler-helper'; import { IExtraOptions } from './instrument'; import { applyReplacements, Replacement } from './replacement'; import { ISourceLocation } from './type-collector-snippet'; @@ -10,28 +11,13 @@ export type ICollectedTypeInfo = Array< [string, number, Array<[string | undefined, ISourceLocation | undefined]>, IExtraOptions] >; -export interface IApplyTypesOptions { +export interface IApplyTypesOptions extends ICompilerOptions { /** * A prefix that will be added in front of each type applied. You can use a javascript comment * to mark the automatically added types. The prefix will be added after the colon character, * just before the actual type. */ prefix?: string; - - /** - * If given, all the file paths in the collected type info will be resolved relative to this directory. - */ - rootDir?: string; - - /** - * Path to your project's tsconfig file - */ - tsConfig?: string; - - // You probably never need to touch these two - they are used by the integration tests to setup - // a virtual file system for TS: - tsConfigHost?: ts.ParseConfigHost; - tsCompilerHost?: ts.CompilerHost; } function findType(program?: ts.Program, typeName?: string, sourcePos?: ISourceLocation) { @@ -88,22 +74,7 @@ export function applyTypesToFile( export function applyTypes(typeInfo: ICollectedTypeInfo, options: IApplyTypesOptions = {}) { const files: { [key: string]: typeof typeInfo } = {}; - let program: ts.Program | undefined; - if (options.tsConfig) { - const configHost = options.tsConfigHost || ts.sys; - const { config, error } = ts.readConfigFile(options.tsConfig, configHost.readFile); - if (error) { - throw new Error(`Error while reading ${options.tsConfig}: ${error.messageText}`); - } - - const parsed = ts.parseJsonConfigFileContent(config, configHost, options.rootDir || ''); - if (parsed.errors.length) { - const errors = parsed.errors.map((e) => e.messageText).join(', '); - throw new Error(`Error while parsing ${options.tsConfig}: ${errors}`); - } - - program = ts.createProgram(parsed.fileNames, parsed.options, options.tsCompilerHost); - } + const program: ts.Program | undefined = getProgram(options); for (const entry of typeInfo) { const file = entry[0]; if (!files[file]) { diff --git a/src/apply-types.spec.ts b/src/compiler-helper.spec.ts similarity index 76% rename from src/apply-types.spec.ts rename to src/compiler-helper.spec.ts index 8191498..bb6b362 100644 --- a/src/apply-types.spec.ts +++ b/src/compiler-helper.spec.ts @@ -1,9 +1,9 @@ import * as ts from 'typescript'; -import { applyTypes } from './apply-types'; +import { getProgram } from './compiler-helper'; -describe('applyTypes', () => { +describe('getProgram', () => { it('should throw an error if given non-existing tsconfig.json file', () => { - expect(() => applyTypes([], { tsConfig: 'not-found-file.json' })).toThrowError( + expect(() => getProgram({ tsConfig: 'not-found-file.json' })).toThrowError( `Error while reading not-found-file.json: The specified path does not exist: 'not-found-file.json'.`, ); }); @@ -13,7 +13,7 @@ describe('applyTypes', () => { ...ts.sys, readFile: jest.fn(() => ''), }; - expect(() => applyTypes([], { tsConfig: 'tsconfig.bad.json', tsConfigHost })).toThrowError( + expect(() => getProgram({ tsConfig: 'tsconfig.bad.json', tsConfigHost })).toThrowError( `Error while reading tsconfig.bad.json: '{' expected.`, ); expect(tsConfigHost.readFile).toHaveBeenCalledWith('tsconfig.bad.json'); @@ -24,7 +24,7 @@ describe('applyTypes', () => { ...ts.sys, readFile: jest.fn(() => '{ "include": 123 }'), }; - expect(() => applyTypes([], { tsConfig: 'tsconfig.invalid.json', tsConfigHost })).toThrowError( + expect(() => getProgram({ tsConfig: 'tsconfig.invalid.json', tsConfigHost })).toThrowError( `Error while parsing tsconfig.invalid.json: Compiler option 'include' requires a value of type Array.`, ); expect(tsConfigHost.readFile).toHaveBeenCalledWith('tsconfig.invalid.json'); diff --git a/src/compiler-helper.ts b/src/compiler-helper.ts new file mode 100644 index 0000000..9f085fa --- /dev/null +++ b/src/compiler-helper.ts @@ -0,0 +1,38 @@ +import * as ts from 'typescript'; + +export interface ICompilerOptions { + /** + * If given, all the file paths in the collected type info will be resolved relative to this directory. + */ + rootDir?: string; + + /** + * Path to your project's tsconfig file + */ + tsConfig?: string; + + // You probably never need to touch these two - they are used by the integration tests to setup + // a virtual file system for TS: + tsConfigHost?: ts.ParseConfigHost; + tsCompilerHost?: ts.CompilerHost; +} + +export function getProgram(options: ICompilerOptions) { + let program: ts.Program | undefined; + if (options.tsConfig) { + const configHost = options.tsConfigHost || ts.sys; + const { config, error } = ts.readConfigFile(options.tsConfig, configHost.readFile); + if (error) { + throw new Error(`Error while reading ${options.tsConfig}: ${error.messageText}`); + } + + const parsed = ts.parseJsonConfigFileContent(config, configHost, options.rootDir || ''); + if (parsed.errors.length) { + const errors = parsed.errors.map((e) => e.messageText).join(', '); + throw new Error(`Error while parsing ${options.tsConfig}: ${errors}`); + } + + program = ts.createProgram(parsed.fileNames, parsed.options, options.tsCompilerHost); + } + return program; +} From a1fc41552f87f80be7eef82cd46dcf7920d9a055 Mon Sep 17 00:00:00 2001 From: zoehneto Date: Thu, 22 Mar 2018 19:15:27 +0100 Subject: [PATCH 2/5] Initial implementation, checking for implicit this doesn't work correctly yet --- src/apply-types.ts | 6 +++ src/instrument.ts | 46 +++++++++++++++++-- src/integration.spec.ts | 86 +++++++++++++++++++++++++++++------ src/type-collector-snippet.ts | 6 ++- 4 files changed, 125 insertions(+), 19 deletions(-) diff --git a/src/apply-types.ts b/src/apply-types.ts index d05defb..cd83bf8 100644 --- a/src/apply-types.ts +++ b/src/apply-types.ts @@ -67,7 +67,13 @@ export function applyTypesToFile( replacements.push(Replacement.insert(opts.parens[0], '(')); suffix = ')'; } + if (opts && opts.comma) { + suffix = ', '; + } replacements.push(Replacement.insert(pos, ': ' + prefix + sortedTypes.join('|') + suffix)); + if (opts && opts.thisType) { + replacements.push(Replacement.insert(pos, 'this')); + } } return applyReplacements(source, replacements); } diff --git a/src/instrument.ts b/src/instrument.ts index 6581965..0a424d1 100644 --- a/src/instrument.ts +++ b/src/instrument.ts @@ -1,14 +1,18 @@ import * as ts from 'typescript'; +import { getProgram, ICompilerOptions } from './compiler-helper'; import { applyReplacements, Replacement } from './replacement'; -export interface IInstrumentOptions { +export interface IInstrumentOptions extends ICompilerOptions { instrumentCallExpressions: boolean; + instrumentImplicitThis: boolean; } export interface IExtraOptions { arrow?: boolean; parens?: [number, number]; + thisType?: boolean; + comma?: boolean; } function hasParensAroundArguments(node: ts.FunctionLike) { @@ -25,9 +29,41 @@ function hasParensAroundArguments(node: ts.FunctionLike) { } } -function visit(node: ts.Node, replacements: Replacement[], fileName: string, options: IInstrumentOptions) { +function visit( + node: ts.Node, + replacements: Replacement[], + fileName: string, + options: IInstrumentOptions, + program?: ts.Program, +) { const isArrow = ts.isArrowFunction(node); if (ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node) || ts.isArrowFunction(node)) { + if (node.body) { + // 'this' type inference requires at least typescript 2.5.0 + const needsThisInstrumentation = + options.instrumentImplicitThis && + program && + (program.getTypeChecker() as any).tryGetThisTypeAt && + (program.getTypeChecker() as any).tryGetThisTypeAt(node) === undefined && + node.body.getText().includes('this'); + if (needsThisInstrumentation) { + const opts: IExtraOptions = { thisType: true }; + if (node.parameters.length > 0) { + opts.comma = true; + } + const params = [ + JSON.stringify('this'), + 'this', + node.parameters.pos, + JSON.stringify(fileName), + JSON.stringify(opts), + ]; + const instrumentExpr = `$_$twiz(${params.join(',')})`; + + replacements.push(Replacement.insert(node.body.getStart() + 1, `${instrumentExpr};`)); + } + } + const isShortArrow = ts.isArrowFunction(node) && !ts.isBlock(node.body); for (const param of node.parameters) { if (!param.type && !param.initializer && node.body) { @@ -105,7 +141,7 @@ function visit(node: ts.Node, replacements: Replacement[], fileName: string, opt } } - node.forEachChild((child) => visit(child, replacements, fileName, options)); + node.forEachChild((child) => visit(child, replacements, fileName, options, program)); } const declaration = ` @@ -118,11 +154,13 @@ const declaration = ` export function instrument(source: string, fileName: string, options?: IInstrumentOptions) { const instrumentOptions: IInstrumentOptions = { instrumentCallExpressions: false, + instrumentImplicitThis: false, ...options, }; + const program: ts.Program | undefined = getProgram(instrumentOptions); const sourceFile = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true); const replacements = [] as Replacement[]; - visit(sourceFile, replacements, fileName, instrumentOptions); + visit(sourceFile, replacements, fileName, instrumentOptions, program); if (replacements.length) { replacements.push(Replacement.insert(0, declaration)); } diff --git a/src/integration.spec.ts b/src/integration.spec.ts index 21ce668..70a6c6a 100644 --- a/src/integration.spec.ts +++ b/src/integration.spec.ts @@ -13,19 +13,7 @@ jest.doMock('fs', () => mockFs); import { applyTypes, getTypeCollectorSnippet, IApplyTypesOptions, instrument } from './index'; function typeWiz(input: string, typeCheck = false, options?: IApplyTypesOptions) { - // Step 1: instrument the source - const instrumented = instrument(input, 'c:\\test.ts', { instrumentCallExpressions: true }); - - // Step 2: compile + add the type collector - const compiled = typeCheck ? transpileSource(instrumented, 'test.ts') : ts.transpile(instrumented); - - // Step 3: evaluate the code, and collect the runtime type information - const collectedTypes = vm.runInNewContext(getTypeCollectorSnippet() + compiled + ';$_$twiz.get();'); - - // Step 4: put the collected typed into the code - mockFs.readFileSync.mockReturnValue(input); - mockFs.writeFileSync.mockImplementationOnce(() => 0); - + // setup options to allow using the TypeChecker if (options && options.tsConfig) { options.tsCompilerHost = virtualCompilerHost(input, 'c:/test.ts'); options.tsConfigHost = { @@ -49,6 +37,23 @@ function typeWiz(input: string, typeCheck = false, options?: IApplyTypesOptions) }; } + // Step 1: instrument the source + const instrumented = instrument(input, 'c:\\test.ts', { + instrumentCallExpressions: true, + instrumentImplicitThis: true, + ...options, + }); + + // Step 2: compile + add the type collector + const compiled = typeCheck ? transpileSource(instrumented, 'test.ts') : ts.transpile(instrumented); + + // Step 3: evaluate the code, and collect the runtime type information + const collectedTypes = vm.runInNewContext(getTypeCollectorSnippet() + compiled + ';$_$twiz.get();'); + + // Step 4: put the collected typed into the code + mockFs.readFileSync.mockReturnValue(input); + mockFs.writeFileSync.mockImplementationOnce(() => 0); + applyTypes(collectedTypes, options); if (options && options.tsConfig) { @@ -70,6 +75,61 @@ beforeEach(() => { }); describe('function parameters', () => { + it('should find `this` type', () => { + const input = ` + class Greeter { + text = "Hello World"; + sayGreeting = greet; + } + function greet() { + return this.text; + } + const greeter = new Greeter(); + greeter.sayGreeting(); + `; + + expect(typeWiz(input, true, { tsConfig: 'tsconfig.integration.json' })).toBe(` + class Greeter { + text = "Hello World"; + sayGreeting = greet; + } + function greet(this: Greeter) { + return this.text; + } + const greeter = new Greeter(); + greeter.sayGreeting(); + `); + }); + + it('should not add `this` type', () => { + const input = ` + class Greeter { + text; + constructor(){ + this.text = "Hello World"; + } + sayGreeting() { + return this.text; + } + } + const greeter = new Greeter(); + greeter.sayGreeting(); + `; + expect(typeWiz(input, true, { tsConfig: 'tsconfig.integration.json' })).toBe(` + class Greeter { + text; + constructor(){ + this.text = "Hello World"; + } + sayGreeting() { + return this.text; + } + } + const greeter = new Greeter(); + greeter.sayGreeting(); + `); + }); + it('should infer `string` type for a simple function', () => { const input = ` function greet(c) { diff --git a/src/type-collector-snippet.ts b/src/type-collector-snippet.ts index c10b5af..216c421 100644 --- a/src/type-collector-snippet.ts +++ b/src/type-collector-snippet.ts @@ -1,3 +1,5 @@ +import { ICollectedTypeInfo } from './apply-types'; + class NestError extends Error {} export type ISourceLocation = [string, number]; /* filename, offset */ @@ -5,7 +7,7 @@ export type ISourceLocation = [string, number]; /* filename, offset */ interface IKey { filename: string; pos: number; - opts: any; + opts: ICollectedTypeInfo; } export function getTypeName(value: any, nest = 0): string | null { @@ -77,7 +79,7 @@ export function getTypeName(value: any, nest = 0): string | null { const logs: { [key: string]: Set } = {}; const trackedObjects = new WeakMap(); -export function $_$twiz(name: string, value: any, pos: number, filename: string, opts: any) { +export function $_$twiz(name: string, value: any, pos: number, filename: string, opts: ICollectedTypeInfo) { const objectDeclaration = trackedObjects.get(value); const index = JSON.stringify({ filename, pos, opts } as IKey); try { From f0242e4593e43f89c274e1a8ff91e9e91e8f2423 Mon Sep 17 00:00:00 2001 From: Tom Z?hner Date: Fri, 23 Mar 2018 19:34:05 +0100 Subject: [PATCH 3/5] Check implicit this with diagnostic --- src/instrument.ts | 42 ++++++++++++++++++++++++++++++++++------- src/integration.spec.ts | 3 ++- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/instrument.ts b/src/instrument.ts index 0a424d1..cae701e 100644 --- a/src/instrument.ts +++ b/src/instrument.ts @@ -35,17 +35,40 @@ function visit( fileName: string, options: IInstrumentOptions, program?: ts.Program, + semanticDiagnostics?: ReadonlyArray, ) { const isArrow = ts.isArrowFunction(node); if (ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node) || ts.isArrowFunction(node)) { if (node.body) { - // 'this' type inference requires at least typescript 2.5.0 const needsThisInstrumentation = options.instrumentImplicitThis && program && - (program.getTypeChecker() as any).tryGetThisTypeAt && - (program.getTypeChecker() as any).tryGetThisTypeAt(node) === undefined && - node.body.getText().includes('this'); + semanticDiagnostics && + semanticDiagnostics.find((diagnostic) => { + if ( + diagnostic.code === 2683 && + diagnostic.file && + diagnostic.file.fileName === node.getSourceFile().fileName && + diagnostic.start + ) { + if (ts.isBlock(node)) { + const body = node.body as ts.FunctionBody; + return ( + body.statements.find((statement) => { + return ( + diagnostic.start !== undefined && + statement.pos <= diagnostic.start && + diagnostic.start <= statement.end + ); + }) !== undefined + ); + } else { + const body = node.body as ts.Expression; + return body.pos <= diagnostic.start && diagnostic.start <= body.end; + } + } + return false; + }) !== undefined; if (needsThisInstrumentation) { const opts: IExtraOptions = { thisType: true }; if (node.parameters.length > 0) { @@ -141,7 +164,7 @@ function visit( } } - node.forEachChild((child) => visit(child, replacements, fileName, options, program)); + node.forEachChild((child) => visit(child, replacements, fileName, options, program, semanticDiagnostics)); } const declaration = ` @@ -158,9 +181,14 @@ export function instrument(source: string, fileName: string, options?: IInstrume ...options, }; const program: ts.Program | undefined = getProgram(instrumentOptions); - const sourceFile = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true); + const sourceFile = program + ? program.getSourceFile(fileName) + : ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true); const replacements = [] as Replacement[]; - visit(sourceFile, replacements, fileName, instrumentOptions, program); + if (sourceFile) { + const semanticDiagnostics = program ? program.getSemanticDiagnostics(sourceFile) : undefined; + visit(sourceFile, replacements, fileName, instrumentOptions, program, semanticDiagnostics); + } if (replacements.length) { replacements.push(Replacement.insert(0, declaration)); } diff --git a/src/integration.spec.ts b/src/integration.spec.ts index 70a6c6a..49977c0 100644 --- a/src/integration.spec.ts +++ b/src/integration.spec.ts @@ -28,6 +28,7 @@ function typeWiz(input: string, typeCheck = false, options?: IApplyTypesOptions) readFile: jest.fn(() => JSON.stringify({ compilerOptions: { + noImplicitThis: true, target: 'es2015', }, include: ['test.ts'], @@ -117,7 +118,7 @@ describe('function parameters', () => { `; expect(typeWiz(input, true, { tsConfig: 'tsconfig.integration.json' })).toBe(` class Greeter { - text; + text: string; constructor(){ this.text = "Hello World"; } From ed0f4e189d9563bbf6717d184e5ae47471d105c4 Mon Sep 17 00:00:00 2001 From: Tom Z?hner Date: Fri, 23 Mar 2018 20:05:35 +0100 Subject: [PATCH 4/5] Fix body detection and improve test coverage --- src/instrument.ts | 2 +- src/integration.spec.ts | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/instrument.ts b/src/instrument.ts index cae701e..6201f01 100644 --- a/src/instrument.ts +++ b/src/instrument.ts @@ -51,7 +51,7 @@ function visit( diagnostic.file.fileName === node.getSourceFile().fileName && diagnostic.start ) { - if (ts.isBlock(node)) { + if (node.body && ts.isBlock(node.body)) { const body = node.body as ts.FunctionBody; return ( body.statements.find((statement) => { diff --git a/src/integration.spec.ts b/src/integration.spec.ts index 49977c0..ad6a6ee 100644 --- a/src/integration.spec.ts +++ b/src/integration.spec.ts @@ -76,7 +76,7 @@ beforeEach(() => { }); describe('function parameters', () => { - it('should find `this` type', () => { + it('should add `this` type', () => { const input = ` class Greeter { text = "Hello World"; @@ -102,6 +102,32 @@ describe('function parameters', () => { `); }); + it('should add `this` type before parameter', () => { + const input = ` + class Greeter { + text = "Hello World: "; + sayGreeting = greet; + } + function greet(name) { + return this.text + name; + } + const greeter = new Greeter(); + greeter.sayGreeting('user'); + `; + + expect(typeWiz(input, true, { tsConfig: 'tsconfig.integration.json' })).toBe(` + class Greeter { + text = "Hello World: "; + sayGreeting = greet; + } + function greet(this: Greeter, name: string) { + return this.text + name; + } + const greeter = new Greeter(); + greeter.sayGreeting('user'); + `); + }); + it('should not add `this` type', () => { const input = ` class Greeter { From 641f3e5fbefaae8e8529d9c3edbf3a7498385f7b Mon Sep 17 00:00:00 2001 From: Tom Z?hner Date: Sat, 24 Mar 2018 10:15:34 +0100 Subject: [PATCH 5/5] Address review comments --- src/apply-types.ts | 12 ++++-------- src/integration.spec.ts | 2 +- src/type-collector-snippet.ts | 6 +++++- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/apply-types.ts b/src/apply-types.ts index cd83bf8..af683a1 100644 --- a/src/apply-types.ts +++ b/src/apply-types.ts @@ -3,13 +3,8 @@ import * as path from 'path'; import * as ts from 'typescript'; import { getProgram, ICompilerOptions } from './compiler-helper'; -import { IExtraOptions } from './instrument'; import { applyReplacements, Replacement } from './replacement'; -import { ISourceLocation } from './type-collector-snippet'; - -export type ICollectedTypeInfo = Array< - [string, number, Array<[string | undefined, ISourceLocation | undefined]>, IExtraOptions] ->; +import { ICollectedTypeInfo, ISourceLocation } from './type-collector-snippet'; export interface IApplyTypesOptions extends ICompilerOptions { /** @@ -62,6 +57,7 @@ export function applyTypesToFile( continue; } + let thisPrefix = ''; let suffix = ''; if (opts && opts.parens) { replacements.push(Replacement.insert(opts.parens[0], '(')); @@ -70,10 +66,10 @@ export function applyTypesToFile( if (opts && opts.comma) { suffix = ', '; } - replacements.push(Replacement.insert(pos, ': ' + prefix + sortedTypes.join('|') + suffix)); if (opts && opts.thisType) { - replacements.push(Replacement.insert(pos, 'this')); + thisPrefix = 'this'; } + replacements.push(Replacement.insert(pos, thisPrefix + ': ' + prefix + sortedTypes.join('|') + suffix)); } return applyReplacements(source, replacements); } diff --git a/src/integration.spec.ts b/src/integration.spec.ts index ad6a6ee..9fb4dd6 100644 --- a/src/integration.spec.ts +++ b/src/integration.spec.ts @@ -128,7 +128,7 @@ describe('function parameters', () => { `); }); - it('should not add `this` type', () => { + it('should not add `this` type when it can be inferred', () => { const input = ` class Greeter { text; diff --git a/src/type-collector-snippet.ts b/src/type-collector-snippet.ts index 216c421..c6a5cf4 100644 --- a/src/type-collector-snippet.ts +++ b/src/type-collector-snippet.ts @@ -1,9 +1,13 @@ -import { ICollectedTypeInfo } from './apply-types'; +import { IExtraOptions } from './instrument'; class NestError extends Error {} export type ISourceLocation = [string, number]; /* filename, offset */ +export type ICollectedTypeInfo = Array< + [string, number, Array<[string | undefined, ISourceLocation | undefined]>, IExtraOptions] +>; + interface IKey { filename: string; pos: number;