From f836dc1d7018b6b2b9cf01e1da65e38cac21db02 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 26 Sep 2016 17:10:52 -0700 Subject: [PATCH] WIP: optimize by checking whole file first --- src/language/languageServiceHost.ts | 41 ++++++++++++----- src/language/rule/typedRule.ts | 2 +- src/rules/noForInArrayRule.ts | 4 +- src/rules/noUnusedVariableRule.ts | 63 ++++++++++++++++----------- src/rules/restrictPlusOperandsRule.ts | 4 +- src/tslintMulti.ts | 8 +++- 6 files changed, 78 insertions(+), 44 deletions(-) diff --git a/src/language/languageServiceHost.ts b/src/language/languageServiceHost.ts index e28e2c5ad20..b79e2a2cfd5 100644 --- a/src/language/languageServiceHost.ts +++ b/src/language/languageServiceHost.ts @@ -19,22 +19,27 @@ import * as ts from "typescript"; import {createCompilerOptions} from "./utils"; -export interface LanguageServiceEditableHost extends ts.LanguageServiceHost { +interface LanguageServiceEditableHost extends ts.LanguageServiceHost { editFile(fileName: string, newContent: string): void; } -export function hostFromProgram(program: ts.Program): LanguageServiceEditableHost { +export function wrapProgram(program: ts.Program): ts.LanguageService { const files: {[name: string]: string} = {}; const fileVersions: {[name: string]: number} = {}; - return { + const host: LanguageServiceEditableHost = { getCompilationSettings: () => program.getCompilerOptions(), getCurrentDirectory: () => program.getCurrentDirectory(), - getDefaultLibFileName: () => "lib.d.ts", - getScriptFileNames: () => program.getRootFileNames(), - getScriptSnapshot: (name: string) => ts.ScriptSnapshot.fromString( - files.hasOwnProperty(name) ? files[name] : - program.getSourceFile(name).getFullText() - ), + getDefaultLibFileName: () => null, + getScriptFileNames: () => program.getSourceFiles().map(sf => sf.fileName), + getScriptSnapshot: (name: string) => { + if (files.hasOwnProperty(name)) { + return ts.ScriptSnapshot.fromString(files[name]); + } + if (!program.getSourceFile(name)) { + return null; + } + return ts.ScriptSnapshot.fromString(program.getSourceFile(name).getFullText()); + }, getScriptVersion: (name: string) => fileVersions.hasOwnProperty(name) ? fileVersions[name] + "" : "1", log: () => { /* */ }, editFile(fileName: string, newContent: string) { @@ -46,6 +51,20 @@ export function hostFromProgram(program: ts.Program): LanguageServiceEditableHos } }, }; + const langSvc = ts.createLanguageService(host, ts.createDocumentRegistry()); + (langSvc as any).editFile = host.editFile; + return langSvc; +} + +export function checkEdit(ls: ts.LanguageService, sf: ts.SourceFile, newText: string) { + (ls as any).editFile(sf.fileName, newText); + const newProgram = ls.getProgram(); + const newSf = newProgram.getSourceFile(sf.fileName); + const newDiags = ts.getPreEmitDiagnostics(newProgram, newSf); + // revert + (ls as any).editFile(sf.fileName, sf.getFullText()); + return newDiags; + } export function createLanguageServiceHost(fileName: string, source: string): ts.LanguageServiceHost { @@ -60,7 +79,7 @@ export function createLanguageServiceHost(fileName: string, source: string): ts. }; } -export function createLanguageService(fileName: string, source: string, program?: ts.Program) { - const languageServiceHost = program ? hostFromProgram(program) : createLanguageServiceHost(fileName, source); +export function createLanguageService(fileName: string, source: string) { + const languageServiceHost = createLanguageServiceHost(fileName, source); return ts.createLanguageService(languageServiceHost); } diff --git a/src/language/rule/typedRule.ts b/src/language/rule/typedRule.ts index 05b5797bb45..026a582a75d 100644 --- a/src/language/rule/typedRule.ts +++ b/src/language/rule/typedRule.ts @@ -26,5 +26,5 @@ export abstract class TypedRule extends AbstractRule { throw new Error(`${this.getOptions().ruleName} requires type checking`); } - public abstract applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[]; + public abstract applyWithProgram(sourceFile: ts.SourceFile, languageService: ts.LanguageService): RuleFailure[]; } diff --git a/src/rules/noForInArrayRule.ts b/src/rules/noForInArrayRule.ts index ebccb4e1a96..51d2c51655e 100644 --- a/src/rules/noForInArrayRule.ts +++ b/src/rules/noForInArrayRule.ts @@ -48,8 +48,8 @@ export class Rule extends Lint.Rules.TypedRule { public static FAILURE_STRING = "for-in loops over arrays are forbidden. Use for-of or array.forEach instead."; - public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] { - const noForInArrayWalker = new NoForInArrayWalker(sourceFile, this.getOptions(), program); + public applyWithProgram(sourceFile: ts.SourceFile, langSvc: Lint.TslintLanguageService): Lint.RuleFailure[] { + const noForInArrayWalker = new NoForInArrayWalker(sourceFile, this.getOptions(), langSvc.getProgram()); return this.applyWithWalker(noForInArrayWalker); } } diff --git a/src/rules/noUnusedVariableRule.ts b/src/rules/noUnusedVariableRule.ts index 9b4333da704..ad6930faf3a 100644 --- a/src/rules/noUnusedVariableRule.ts +++ b/src/rules/noUnusedVariableRule.ts @@ -80,10 +80,8 @@ export class Rule extends Lint.Rules.TypedRule { return this.applyWithProgram(sourceFile, undefined); } - public applyWithProgram(sourceFile: ts.SourceFile, program?: ts.Program): Lint.RuleFailure[] { - let languageServiceHost = program ? Lint.hostFromProgram(program) : - Lint.createLanguageServiceHost(sourceFile.fileName, sourceFile.getFullText()); - return this.applyWithWalker(new NoUnusedVariablesWalker(sourceFile, this.getOptions(), languageServiceHost, program)); + public applyWithProgram(sourceFile: ts.SourceFile, langSvc?: ts.LanguageService): Lint.RuleFailure[] { + return this.applyWithWalker(new NoUnusedVariablesWalker(sourceFile, this.getOptions(), langSvc)); } } @@ -96,16 +94,20 @@ class NoUnusedVariablesWalker extends Lint.RuleWalker { private ignorePattern: RegExp; private isReactUsed: boolean; private reactImport: ts.NamespaceImport; - private languageService: ts.LanguageService; + private dummyLanguageService: boolean; + private possibleFailures: Lint.RuleFailure[] = []; constructor(sourceFile: ts.SourceFile, options: Lint.IOptions, - private languageServiceHost: ts.LanguageServiceHost, private program?: ts.Program) { + private languageService?: ts.LanguageService) { super(sourceFile, options); - this.languageService = ts.createLanguageService(languageServiceHost); this.skipVariableDeclaration = false; this.skipParameterDeclaration = false; this.hasSeenJsxElement = false; this.isReactUsed = false; + if (!languageService) { + this.dummyLanguageService = true; + this.languageService = Lint.createLanguageService(sourceFile.fileName, sourceFile.getFullText()); + } const ignorePatternOption = this.getOptions().filter((option: any) => { return typeof option === "object" && option["ignore-pattern"] != null; @@ -144,9 +146,35 @@ class NoUnusedVariablesWalker extends Lint.RuleWalker { if (!this.isIgnored(nameText)) { const start = this.reactImport.name.getStart(); const msg = Rule.FAILURE_STRING_FACTORY(Rule.FAILURE_TYPE_IMPORT, nameText); - this.addFailure(this.createFailure(start, nameText.length, msg)); + this.possibleFailures.push(this.createFailure(start, nameText.length, msg)); + } + } + + let someFixBrokeIt = false; + // Performance optimization: type-check the whole file before verifying individual fixes + if (this.possibleFailures.some(f => f.hasFix()) && !this.dummyLanguageService) { + let newText = Lint.Fix.applyAll(this.getSourceFile().getFullText(), + this.possibleFailures.map(f => f.getFix()).filter(f => !!f)); + + // If we have the program, we can verify that the fix doesn't introduce failures + if (Lint.checkEdit(this.languageService, this.getSourceFile(), newText).length > 0) { + console.error(`Fixes caused errors in ${this.getSourceFile().fileName}`); + someFixBrokeIt = true; } } + + this.possibleFailures.forEach(f => { + if (!someFixBrokeIt || !f.hasFix()) { + this.addFailure(f); + } else { + let newText = f.getFix().apply(this.getSourceFile().getFullText()); + if (Lint.checkEdit(this.languageService, this.getSourceFile(), newText).length > 0) { + console.error(`Found one of the broken fixes in ${this.getSourceFile().fileName}`); + } else { + this.addFailure(f); + } + } + }); } public visitBindingElement(node: ts.BindingElement) { @@ -427,24 +455,7 @@ class NoUnusedVariablesWalker extends Lint.RuleWalker { if (replacements && replacements.length) { fix = new Lint.Fix("no-unused-variable", replacements); } - // If we have the program, we can verify that the fix doesn't introduce failures - if (this.program) { - const program = this.languageService.getProgram(); - const sf = this.getSourceFile(); - const existingDiags = ts.getPreEmitDiagnostics(program, sf); - if (existingDiags.length > 0) { - throw new Error("Fix existing diagnostics before running no-unused-variable"); - } - (this.languageServiceHost as any).editFile(sf.fileName, fix.apply(sf.getFullText())); - const newProgram = this.languageService.getProgram(); - const newSf = newProgram.getSourceFile(sf.fileName); - const newDiags = ts.getPreEmitDiagnostics(newProgram, newSf); - if (newDiags.length > 0) { - console.error("rejecting fix because it causes errors"); - return; - } - } - this.addFailure(this.createFailure(position, name.length, Rule.FAILURE_STRING_FACTORY(type, name), fix)); + this.possibleFailures.push(this.createFailure(position, name.length, Rule.FAILURE_STRING_FACTORY(type, name), fix)); } private isIgnored(name: string) { diff --git a/src/rules/restrictPlusOperandsRule.ts b/src/rules/restrictPlusOperandsRule.ts index 50cea6b81eb..3f374aec838 100644 --- a/src/rules/restrictPlusOperandsRule.ts +++ b/src/rules/restrictPlusOperandsRule.ts @@ -35,8 +35,8 @@ export class Rule extends Lint.Rules.TypedRule { public static MISMATCHED_TYPES_FAILURE = "Types of values used in '+' operation must match"; public static UNSUPPORTED_TYPE_FAILURE_FACTORY = (type: string) => `cannot add type ${type}`; - public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] { - return this.applyWithWalker(new RestrictPlusOperandsWalker(sourceFile, this.getOptions(), program)); + public applyWithProgram(sourceFile: ts.SourceFile, langSvc: Lint.TslintLanguageService): Lint.RuleFailure[] { + return this.applyWithWalker(new RestrictPlusOperandsWalker(sourceFile, this.getOptions(), langSvc.getProgram())); } } diff --git a/src/tslintMulti.ts b/src/tslintMulti.ts index 471fd636e37..39d5c24529d 100644 --- a/src/tslintMulti.ts +++ b/src/tslintMulti.ts @@ -28,6 +28,7 @@ import { } from "./configuration"; import { EnableDisableRulesWalker } from "./enableDisableRules"; import { findFormatter } from "./formatterLoader"; +import { wrapProgram } from "./language/languageServiceHost"; import { IFormatter } from "./language/formatter/formatter"; import { RuleFailure } from "./language/rule/rule"; import { TypedRule } from "./language/rule/typedRule"; @@ -48,6 +49,7 @@ class MultiLinter { public static loadConfigurationFromPath = loadConfigurationFromPath; private failures: RuleFailure[] = []; + private languageService: ts.LanguageService; /** * Creates a TypeScript program object from a tsconfig.json file path and optional project directory. @@ -84,7 +86,9 @@ class MultiLinter { } constructor(private options: IMultiLinterOptions, private program?: ts.Program) { - // Empty + if (program) { + this.languageService = wrapProgram(program); + } } public lint(fileName: string, source?: string, configuration: any = DEFAULT_CONFIG): void { @@ -119,7 +123,7 @@ class MultiLinter { for (let rule of enabledRules) { let ruleFailures: RuleFailure[] = []; if (this.program && rule instanceof TypedRule) { - ruleFailures = rule.applyWithProgram(sourceFile, this.program); + ruleFailures = rule.applyWithProgram(sourceFile, this.languageService); } else { ruleFailures = rule.apply(sourceFile); }