diff --git a/docs/_data/formatters.json b/docs/_data/formatters.json index 83a81e7bf9d..c3541f5d75c 100644 --- a/docs/_data/formatters.json +++ b/docs/_data/formatters.json @@ -1,11 +1,4 @@ [ - { - "formatterName": "applyFixes", - "description": "Automatically fixes lint failures.", - "descriptionDetails": "\nModifies source files and applies fixes for lint failures where possible. Changes\nshould be tested as not all fixes preserve semantics.", - "sample": "\nAll done. Remember to test the changes, as not all fixes preserve semantics.", - "consumer": "machine" - }, { "formatterName": "checkstyle", "description": "Formats errors as through they were Checkstyle output.", diff --git a/docs/formatters/applyFixes/index.html b/docs/formatters/applyFixes/index.html deleted file mode 100644 index b860632660b..00000000000 --- a/docs/formatters/applyFixes/index.html +++ /dev/null @@ -1,14 +0,0 @@ ---- -formatterName: applyFixes -description: Automatically fixes lint failures. -descriptionDetails: |- - - Modifies source files and applies fixes for lint failures where possible. Changes - should be tested as not all fixes preserve semantics. -sample: |- - - All done. Remember to test the changes, as not all fixes preserve semantics. -consumer: machine -layout: formatter -title: 'Formatter: applyFixes' ---- \ No newline at end of file diff --git a/src/formatters/applyFixesFormatter.ts b/src/formatters/applyFixesFormatter.ts deleted file mode 100644 index e012ef40ad4..00000000000 --- a/src/formatters/applyFixesFormatter.ts +++ /dev/null @@ -1,35 +0,0 @@ -import {AbstractFormatter} from "../language/formatter/abstractFormatter"; -import {IFormatterMetadata} from "../language/formatter/formatter"; -import {Fix, RuleFailure} from "../language/rule/rule"; -import * as Utils from "../utils"; -import * as fs from "fs"; - -export class Formatter extends AbstractFormatter { - /* tslint:disable:object-literal-sort-keys */ - public static metadata: IFormatterMetadata = { - formatterName: "applyFixes", - description: "Automatically fixes lint failures.", - descriptionDetails: Utils.dedent` - Modifies source files and applies fixes for lint failures where possible. Changes - should be tested as not all fixes preserve semantics.`, - sample: Utils.dedent` - All done. Remember to test the changes, as not all fixes preserve semantics.`, - consumer: "machine", - }; - /* tslint:enable:object-literal-sort-keys */ - - public format(failures: RuleFailure[]): string { - const files: {[file: string]: boolean} = {}; - failures.map(f => files[f.getFileName()] = true); - const log: string[] = []; - for (const file of Object.keys(files)) { - log.push(`Applying fixes to ${file}`); - let content = fs.readFileSync(file, {encoding: "utf-8"}); - const fixes = failures.filter(f => f.getFileName() === file).map(f => f.getFix()).filter(f => !!f); - content = Fix.applyAll(content, fixes); - fs.writeFileSync(file, content, {encoding: "utf-8"}); - } - log.push("All done. Remember to test the changes, as not all fixes preserve semantics."); - return log.join("\n") + "\n"; - } -} diff --git a/src/formatters/proseFormatter.ts b/src/formatters/proseFormatter.ts index 1331c9248ab..b70024a4fa0 100644 --- a/src/formatters/proseFormatter.ts +++ b/src/formatters/proseFormatter.ts @@ -29,12 +29,30 @@ export class Formatter extends AbstractFormatter { }; /* tslint:enable:object-literal-sort-keys */ - public format(failures: RuleFailure[]): string { - if (failures.length === 0) { + public format(failures: RuleFailure[], fixes?: RuleFailure[]): string { + if (failures.length === 0 && (!fixes || fixes.length === 0)) { return ""; } - const outputLines = failures.map((failure: RuleFailure) => { + let fixLines: string[] = []; + if (fixes) { + let perFileFixes: { [fileName: string]: number } = {}; + for (const fix of fixes) { + if (perFileFixes[fix.getFileName()] == null) { + perFileFixes[fix.getFileName()] = 1; + } else { + perFileFixes[fix.getFileName()]++; + } + } + + Object.keys(perFileFixes).forEach((fixedFile: string) => { + const fixCount = perFileFixes[fixedFile]; + fixLines.push(`Fixed ${fixCount} error(s) in ${fixedFile}`); + }); + fixLines.push(""); // add a blank line between fixes and failures + } + + let errorLines = failures.map((failure: RuleFailure) => { const fileName = failure.getFileName(); const failureString = failure.getFailure(); @@ -44,6 +62,6 @@ export class Formatter extends AbstractFormatter { return `${fileName}${positionTuple}: ${failureString}`; }); - return outputLines.join("\n") + "\n"; + return fixLines.concat(errorLines).join("\n") + "\n"; } } diff --git a/src/language/formatter/formatter.ts b/src/language/formatter/formatter.ts index 50b4de7c9e0..7801a9a510b 100644 --- a/src/language/formatter/formatter.ts +++ b/src/language/formatter/formatter.ts @@ -47,5 +47,10 @@ export interface IFormatterMetadata { export type ConsumerType = "human" | "machine"; export interface IFormatter { - format(failures: RuleFailure[]): string; + /** + * Formats linter results + * @param {RuleFailure[]} failures Linter errors that were not fixed + * @param {RuleFailure[]} fixes Fixed linter errors. Available when the `--fix` argument is used on the command line + */ + format(failures: RuleFailure[], fixes?: RuleFailure[]): string; } diff --git a/src/lint.ts b/src/lint.ts index 35f3a1af219..c96ab319bb5 100644 --- a/src/lint.ts +++ b/src/lint.ts @@ -42,6 +42,7 @@ export var Utils = utils; export interface LintResult { failureCount: number; failures: RuleFailure[]; + fixes?: RuleFailure[]; format: string | Function; output: string; } @@ -55,11 +56,13 @@ export interface ILinterOptionsRaw { export interface ILinterOptions extends ILinterOptionsRaw { configuration: any; + fix: boolean; formatter: string | Function; rulesDirectory: string | string[]; } export interface IMultiLinterOptions { + fix: boolean; formatter?: string | Function; formattersDirectory?: string; rulesDirectory?: string | string[]; diff --git a/src/tslint-cli.ts b/src/tslint-cli.ts index f358dc672f3..cf5f07ec296 100644 --- a/src/tslint-cli.ts +++ b/src/tslint-cli.ts @@ -50,6 +50,9 @@ let processed = optimist alias: "exclude", describe: "exclude globs from path expansion", }, + fix: { + describe: "Fixes linting errors for select rules. This may overwrite linted files", + }, force: { describe: "return status code 0 even if there are lint errors", type: "boolean", @@ -152,6 +155,9 @@ tslint accepts the following commandline options: This option can be supplied multiple times if you need multiple globs to indicate which files to exclude. + --fix: + Fixes linting errors for select rules. This may overwrite linted files. + --force: Return status code 0 even if there are any lint errors. Useful while running as npm script. @@ -220,6 +226,7 @@ const possibleConfigAbsolutePath = argv.c != null ? path.resolve(argv.c) : null; const processFiles = (files: string[], program?: ts.Program) => { const linter = new Linter({ + fix: argv.fix, formatter: argv.t, formattersDirectory: argv.s || "", rulesDirectory: argv.r || "", diff --git a/src/tslint.ts b/src/tslint.ts index 075eb51bc75..da9b53b106d 100644 --- a/src/tslint.ts +++ b/src/tslint.ts @@ -75,6 +75,7 @@ class Linter { return { configuration: configuration || DEFAULT_CONFIG, + fix: false, formatter: formatter || "prose", formattersDirectory, rulesDirectory: arrayify(rulesDirectory).concat(arrayify(configuration.rulesDirectory)), diff --git a/src/tslintMulti.ts b/src/tslintMulti.ts index 9aca4f95af1..2f9ef86232e 100644 --- a/src/tslintMulti.ts +++ b/src/tslintMulti.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { existsSync } from "fs"; +import * as fs from "fs"; import * as ts from "typescript"; import { @@ -30,10 +30,10 @@ import { import { EnableDisableRulesWalker } from "./enableDisableRules"; import { findFormatter } from "./formatterLoader"; import { IFormatter } from "./language/formatter/formatter"; -import { RuleFailure } from "./language/rule/rule"; +import {Fix, IRule, RuleFailure} from "./language/rule/rule"; import { TypedRule } from "./language/rule/typedRule"; -import { getSourceFile } from "./language/utils"; -import { IMultiLinterOptions, IRule, LintResult } from "./lint"; +import * as utils from "./language/utils"; +import { IMultiLinterOptions, LintResult } from "./lint"; import { loadRules } from "./ruleLoader"; import { arrayify } from "./utils"; @@ -49,6 +49,7 @@ class MultiLinter { public static loadConfigurationFromPath = loadConfigurationFromPath; private failures: RuleFailure[] = []; + private fixes: RuleFailure[] = []; /** * Creates a TypeScript program object from a tsconfig.json file path and optional project directory. @@ -65,7 +66,7 @@ class MultiLinter { const { config } = ts.readConfigFile(configFile, ts.sys.readFile); const parseConfigHost = { - fileExists: existsSync, + fileExists: fs.existsSync, readDirectory: ts.sys.readDirectory, useCaseSensitiveFileNames: true, }; @@ -89,54 +90,35 @@ class MultiLinter { } public lint(fileName: string, source?: string, configuration: IConfigurationFile = DEFAULT_CONFIG): void { - let sourceFile: ts.SourceFile; - if (this.program) { - sourceFile = this.program.getSourceFile(fileName); - // check if the program has been type checked - if (sourceFile && !("resolvedModules" in sourceFile)) { - throw new Error("Program must be type checked before linting"); + const enabledRules = this.getEnabledRules(fileName, source, configuration); + let sourceFile = this.getSourceFile(fileName, source); + let hasLinterRun = false; + + if (this.options.fix) { + this.fixes = []; + for (let rule of enabledRules) { + let fileFailures = this.applyRule(rule, sourceFile); + const fixes = fileFailures.map(f => f.getFix()).filter(f => !!f); + source = fs.readFileSync(fileName, { encoding: "utf-8" }); + if (fixes.length > 0) { + this.fixes = this.fixes.concat(fileFailures); + source = Fix.applyAll(source, fixes); + fs.writeFileSync(fileName, source, { encoding: "utf-8" }); + + // reload AST if file is modified + sourceFile = this.getSourceFile(fileName, source); + } + this.failures = this.failures.concat(fileFailures); } - } else { - sourceFile = getSourceFile(fileName, source); + hasLinterRun = true; } - if (sourceFile === undefined) { - throw new Error(`Invalid source file: ${fileName}. Ensure that the files supplied to lint have a .ts, .tsx, or .js extension.`); - } - - // walk the code first to find all the intervals where rules are disabled - const rulesWalker = new EnableDisableRulesWalker(sourceFile, { - disabledIntervals: [], - ruleName: "", - }); - rulesWalker.walk(sourceFile); - const enableDisableRuleMap = rulesWalker.enableDisableRuleMap; - - const rulesDirectories = arrayify(this.options.rulesDirectory) - .concat(arrayify(configuration.rulesDirectory)); - const configurationRules = configuration.rules; - const jsConfiguration = configuration.jsRules; - const isJs = fileName.substr(-3) === ".js"; - let configuredRules: IRule[]; - - if (isJs) { - configuredRules = loadRules(jsConfiguration, enableDisableRuleMap, rulesDirectories, true); - } else { - configuredRules = loadRules(configurationRules, enableDisableRuleMap, rulesDirectories, false); - } - - const enabledRules = configuredRules.filter((r) => r.isEnabled()); - for (let rule of enabledRules) { - let ruleFailures: RuleFailure[] = []; - if (this.program && rule instanceof TypedRule) { - ruleFailures = rule.applyWithProgram(sourceFile, this.program); - } else { - ruleFailures = rule.apply(sourceFile); - } - for (let ruleFailure of ruleFailures) { - if (!this.containsRule(this.failures, ruleFailure)) { - this.failures.push(ruleFailure); - } + // make a 1st pass or make a 2nd pass if there were any fixes because the positions may be off + if (!hasLinterRun || this.fixes.length > 0) { + this.failures = []; + for (let rule of enabledRules) { + const fileFailures = this.applyRule(rule, sourceFile); + this.failures = this.failures.concat(fileFailures); } } } @@ -153,16 +135,71 @@ class MultiLinter { throw new Error(`formatter '${formatterName}' not found`); } - const output = formatter.format(this.failures); + const output = formatter.format(this.failures, this.fixes); return { failureCount: this.failures.length, failures: this.failures, + fixes: this.fixes, format: formatterName, output, }; } + private applyRule(rule: IRule, sourceFile: ts.SourceFile) { + let ruleFailures: RuleFailure[] = []; + if (this.program && rule instanceof TypedRule) { + ruleFailures = rule.applyWithProgram(sourceFile, this.program); + } else { + ruleFailures = rule.apply(sourceFile); + } + let fileFailures: RuleFailure[] = []; + for (let ruleFailure of ruleFailures) { + if (!this.containsRule(this.failures, ruleFailure)) { + fileFailures.push(ruleFailure); + } + } + return fileFailures; + } + + private getEnabledRules(fileName: string, source?: string, configuration: IConfigurationFile = DEFAULT_CONFIG): IRule[] { + const sourceFile = this.getSourceFile(fileName, source); + + // walk the code first to find all the intervals where rules are disabled + const rulesWalker = new EnableDisableRulesWalker(sourceFile, { + disabledIntervals: [], + ruleName: "", + }); + rulesWalker.walk(sourceFile); + const enableDisableRuleMap = rulesWalker.enableDisableRuleMap; + + const rulesDirectories = arrayify(this.options.rulesDirectory) + .concat(arrayify(configuration.rulesDirectory)); + const isJs = fileName.substr(-3) === ".js"; + const configurationRules = isJs ? configuration.jsRules : configuration.rules; + let configuredRules = loadRules(configurationRules, enableDisableRuleMap, rulesDirectories, isJs); + + return configuredRules.filter((r) => r.isEnabled()); + } + + private getSourceFile(fileName: string, source?: string) { + let sourceFile: ts.SourceFile; + if (this.program) { + sourceFile = this.program.getSourceFile(fileName); + // check if the program has been type checked + if (sourceFile && !("resolvedModules" in sourceFile)) { + throw new Error("Program must be type checked before linting"); + } + } else { + sourceFile = utils.getSourceFile(fileName, source); + } + + if (sourceFile === undefined) { + throw new Error(`Invalid source file: ${fileName}. Ensure that the files supplied to lint have a .ts, .tsx, or .js extension.`); + } + return sourceFile; + } + private containsRule(rules: RuleFailure[], rule: RuleFailure) { return rules.some((r) => r.equals(rule)); } diff --git a/test/configurationTests.ts b/test/configurationTests.ts index 2e5ab236c14..c73f0b7cdb8 100644 --- a/test/configurationTests.ts +++ b/test/configurationTests.ts @@ -15,10 +15,9 @@ */ import * as fs from "fs"; -import * as os from "os"; -import * as path from "path"; -import {IConfigurationFile, extendConfigurationFile, loadConfigurationFromPath} from "../src/configuration"; +import { IConfigurationFile, extendConfigurationFile, loadConfigurationFromPath } from "../src/configuration"; +import { createTempFile } from "./utils"; describe("Configuration", () => { it("extendConfigurationFile", () => { @@ -123,16 +122,7 @@ describe("Configuration", () => { let tmpfile: string; beforeEach(() => { - for (let i = 0; i < 5; i++) { - const attempt = path.join(os.tmpdir(), `tslint.test${Math.round(Date.now() * Math.random())}.json`); - if (!fs.existsSync(tmpfile)) { - tmpfile = attempt; - break; - } - } - if (tmpfile === undefined) { - throw new Error("Couldn't create temp file"); - } + tmpfile = createTempFile("json"); }); afterEach(() => { diff --git a/test/executable/executableTests.ts b/test/executable/executableTests.ts index 31d281c2958..81180180fa7 100644 --- a/test/executable/executableTests.ts +++ b/test/executable/executableTests.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { createTempFile } from "../utils"; import * as cp from "child_process"; import * as fs from "fs"; import * as os from "os"; @@ -115,6 +116,22 @@ describe("Executable", function() { }); }); + describe("--fix flag", () => { + it("fixes multiple rules without overwriting each other", (done) => { + const tempFile = createTempFile("ts"); + fs.createReadStream("test/files/multiple-fixes-test/multiple-fixes.test.ts").pipe(fs.createWriteStream(tempFile)); + execCli(["-c", "test/files/multiple-fixes-test/tslint.json", tempFile, "--fix"], + (err, stdout) => { + const content = fs.readFileSync(tempFile, "utf8"); + fs.unlinkSync(tempFile); + assert.strictEqual(content, "import * as y from \"a_long_module\";\nimport * as x from \"b\";\n"); + assert.isNull(err, "process should exit without an error"); + assert.strictEqual(stdout, `Fixed 2 error(s) in ${tempFile}`); + done(); + }); + }); + }); + describe("--force flag", () => { it("exits with code 0 if `--force` flag is passed", (done) => { execCli(["-c", "./test/config/tslint-custom-rules.json", "-r", "./test/files/custom-rules", "--force", "src/tslint.ts"], diff --git a/test/files/multiple-fixes-test/multiple-fixes.test.ts b/test/files/multiple-fixes-test/multiple-fixes.test.ts new file mode 100644 index 00000000000..83849f1a699 --- /dev/null +++ b/test/files/multiple-fixes-test/multiple-fixes.test.ts @@ -0,0 +1,2 @@ +import * as x from "b" +import * as y from "a_long_module"; diff --git a/test/files/multiple-fixes-test/tslint.json b/test/files/multiple-fixes-test/tslint.json new file mode 100644 index 00000000000..d57be419e19 --- /dev/null +++ b/test/files/multiple-fixes-test/tslint.json @@ -0,0 +1,6 @@ +{ + "rules": { + "ordered-imports": [true], + "semicolon": [true, "always"] + } +} diff --git a/test/formatters/proseFormatterTests.ts b/test/formatters/proseFormatterTests.ts index ccf8bc18907..0643e58bfdf 100644 --- a/test/formatters/proseFormatterTests.ts +++ b/test/formatters/proseFormatterTests.ts @@ -47,6 +47,28 @@ describe("Prose Formatter", () => { assert.equal(actualResult, expectedResult); }); + it("formats fixes", () => { + const failures = [ + new RuleFailure(sourceFile, 0, 1, "first failure", "first-name"), + ]; + + const mockFix = { getFileName: () => { return "file2"; } } as any; + + const fixes = [ + new RuleFailure(sourceFile, 0, 1, "first failure", "first-name"), + new RuleFailure(sourceFile, 32, 36, "mid failure", "mid-name"), + mockFix, + ]; + + const expectedResult = + `Fixed 2 error(s) in ${TEST_FILE}\n` + + `Fixed 1 error(s) in file2\n\n` + + `${TEST_FILE}${getPositionString(1, 1)}first failure\n`; + + const actualResult = formatter.format(failures, fixes); + assert.equal(actualResult, expectedResult); + }); + it("handles no failures", () => { const result = formatter.format([]); assert.equal(result, ""); diff --git a/test/utils.ts b/test/utils.ts index f3c80a751ee..81629f064fa 100644 --- a/test/utils.ts +++ b/test/utils.ts @@ -15,6 +15,7 @@ */ import * as fs from "fs"; +import * as os from "os"; import * as path from "path"; import * as ts from "typescript"; @@ -82,3 +83,18 @@ export function assertContainsFailure(haystack: Lint.RuleFailure[], needle: Lint assert(false, "expected " + stringifiedNeedle + " within " + stringifiedHaystack); } } + +export function createTempFile(extension: string) { + let tmpfile: string; + for (let i = 0; i < 5; i++) { + const attempt = path.join(os.tmpdir(), `tslint.test${Math.round(Date.now() * Math.random())}.${extension}`); + if (!fs.existsSync(tmpfile)) { + tmpfile = attempt; + break; + } + } + if (tmpfile === undefined) { + throw new Error("Couldn't create temp file"); + } + return tmpfile; +}