Add a way to call Tsetse with fixes in the diagnostic text. This adds a way to turn diagnostics into text with an inlined fix, and the related tests. Also sneak in a few tests for imports that I seemed to have forgot about. PiperOrigin-RevId: 268656099
diff --git a/internal/tsetse/failure.ts b/internal/tsetse/failure.ts index 61680ac..317faf4 100644 --- a/internal/tsetse/failure.ts +++ b/internal/tsetse/failure.ts
@@ -19,7 +19,7 @@ * This returns a structure compatible with ts.Diagnostic, but with added * fields, for convenience and to support suggested fixes. */ - toDiagnostic(): ts.Diagnostic&{end: number, fix?: Fix} { + toDiagnostic(): DiagnosticWithFix { return { file: this.sourceFile, start: this.start, @@ -35,15 +35,87 @@ }; } + /** + * Same as toDiagnostic, but include the fix in the message, so that systems + * that don't support displaying suggested fixes can still surface that + * information. This assumes the diagnostic message is going to be presented + * within the context of the problematic code + */ + toDiagnosticWithStringifiedFix(): DiagnosticWithFix { + const diagnostic = this.toDiagnostic(); + if (this.suggestedFix) { + diagnostic.messageText += ' ' + this.fixToReadableStringInContext(); + } + return diagnostic; + } + toString(): string { return `{ sourceFile:${ this.sourceFile ? this.sourceFile.fileName : 'unknown'}, start:${ this.start}, end:${this.end}, fix:${fixToString(this.suggestedFix)} }`; } + + + /** + * Stringifies a `Fix`, in a way that makes sense when presented alongside the + * finding. This is a heuristic, obviously. + */ + fixToReadableStringInContext() { + if (!this.suggestedFix) return ''; // no changes, nothing to state. + const f: Fix = this.suggestedFix; + let fixText = ''; + + for (const c of f.changes) { + // Insertion. + if (c.start === c.end) { + // Try to see if that's an import. + if (c.replacement.indexOf('import') !== -1) { + fixText += `- Add new import: ${c.replacement}\n`; + } else { + // Insertion that's not a full import. This should rarely happen in + // our context, and we don't have a great message for these. + // For instance, this could be the addition of a new symbol in an + // existing import (`import {foo}` becoming `import {foo, bar}`). + fixText += `- Insert ${this.readableRange(c.start, c.end)}: ${ + c.replacement}\n`; + } + } else if (c.start === this.start && c.end === this.end) { + // We assume the replacement is the main part of the fix, so put that + // individual change first in `fixText`. + fixText = `- Replace the full match with: ${c.replacement}\n` + fixText; + } else { + // Fallback case: Use a numerical range to specify a replacement. In + // general, falling through in this case should be avoided, as it's not + // really readable without an IDE (the range can be outside of the + // matched code). + fixText = `- Replace ${this.readableRange(c.start, c.end)} with: ` + + `${c.replacement}\n${fixText}`; + } + } + + return 'Suggested fix:\n' + fixText.trim(); + } + + // TS indexes from 0 both ways, but tooling generally indexes from 1 for both + // lines and columns. The translation is done here. + readableRange(from: number, to: number) { + const lcf = this.sourceFile.getLineAndCharacterOfPosition(from); + const lct = this.sourceFile.getLineAndCharacterOfPosition(to); + if (lcf.line === lct.line) { + if (lcf.character === lct.character) { + return `at line ${lcf.line + 1}, char ${lcf.character + 1}`; + } + return `line ${lcf.line + 1}, from char ${lcf.character + 1} to ${ + lct.character + 1}`; + } else { + return `from line ${lcf.line + 1}, char ${lcf.character + 1} to line ${ + lct.line + 1}, char ${lct.character + 1}`; + } + } } /** - * A Fix is a potential repair to the associated Failure. + * A `Fix` is a potential repair to the associated `Failure`. */ export interface Fix { /** @@ -52,12 +124,26 @@ changes: IndividualChange[], } +/** + * An individual text replacement/insertion in a source file. Used as part of a + * `Fix`. + */ export interface IndividualChange { sourceFile: ts.SourceFile, start: number, end: number, replacement: string } /** - * Stringifies a Fix, replacing the ts.SourceFile with the matching filename. + * A ts.Diagnostic that might include a `Fix`, and with an added `end` field for + * convenience. + */ +export interface DiagnosticWithFix extends ts.Diagnostic { + end: number; + fix?: Fix; +} + +/** + * Stringifies a `Fix`, replacing the `ts.SourceFile` with the matching + * filename. */ export function fixToString(f?: Fix) { if (!f) return 'undefined';
diff --git a/internal/tsetse/tests/ban_conformance_pattern/fixer_test.ts b/internal/tsetse/tests/ban_conformance_pattern/fixer_test.ts index 3b7c85f..24e53ad 100644 --- a/internal/tsetse/tests/ban_conformance_pattern/fixer_test.ts +++ b/internal/tsetse/tests/ban_conformance_pattern/fixer_test.ts
@@ -1,9 +1,9 @@ import 'jasmine'; import * as ts from 'typescript'; -import {Fix} from '../../failure'; +import {Failure, Fix} from '../../failure'; import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule'; -import {buildReplacementFixer, Fixer} from '../../util/fixer'; -import {compileAndCheck, customMatchers} from '../../util/testing/test_support'; +import {buildReplacementFixer, Fixer, maybeAddNamedImport, maybeAddNamespaceImport} from '../../util/fixer'; +import {compile, compileAndCheck, customMatchers} from '../../util/testing/test_support'; const uppercaseFixer: Fixer = { getFixForFlaggedNode(node: ts.Node): Fix { @@ -22,19 +22,19 @@ return {replaceWith: node.getText().toUpperCase()}; }) +// The initial config and source off which we run those checks. +const baseConfig = { + errorMessage: 'found citation', + kind: PatternKind.BANNED_PROPERTY_WRITE, + values: ['HTMLQuoteElement.prototype.cite'], +}; + +const source = `export {};\n` + + `const q = document.createElement('q');\n` + + `q.cite = 'some example string';\n`; + describe('ConformancePatternRule\'s fixer', () => { describe('Generates basic fixes', () => { - const source = `export {};\n` + - `const q = document.createElement('q');\n` + - `q.cite = 'some example string';\n`; - - // The initial config off which we run those checks. - const baseConfig = { - errorMessage: 'found citation', - kind: PatternKind.BANNED_PROPERTY_WRITE, - values: ['HTMLQuoteElement.prototype.cite'], - }; - it('for a single match', () => { const rule = new ConformancePatternRule(baseConfig, uppercaseFixer); const results = compileAndCheck(rule, source); @@ -49,7 +49,6 @@ ]); }); - it('for a single match (alternate fixer)', () => { const rule = new ConformancePatternRule(baseConfig, uppercaseFixerBuilt); const results = compileAndCheck(rule, source); @@ -82,15 +81,117 @@ expect(results[0]).toHaveFixMatching([ {start: 50, end: 80, replacement: `Q.CITE = 'SOME EXAMPLE STRING'`} ]); + expect(results[0].fixToReadableStringInContext()) + .toBe( + `Suggested fix:\n` + + `- Replace the full match with: Q.CITE = 'SOME EXAMPLE STRING'`); expect(results[1]).toHaveFixMatching([{ start: 82, end: 118, replacement: `Q.CITE = 'SOME OTHER EXAMPLE STRING'` }]); + expect(results[1].fixToReadableStringInContext()) + .toBe( + `Suggested fix:\n` + + `- Replace the full match with: Q.CITE = 'SOME OTHER EXAMPLE STRING'`); + }); + }); + + describe('adds imports', () => { + const addNamedImportFixer: Fixer = { + getFixForFlaggedNode(n: ts.Node) { + const ic = + maybeAddNamedImport(n.getSourceFile(), 'foo', './file_1', 'bar'); + if (ic) return {changes: [ic]}; + return; + } + }; + + it('maybeAddNamedImport additions', () => { + const results = compileAndCheck( + new ConformancePatternRule(baseConfig, addNamedImportFixer), source); + + expect(results[0]).toHaveFixMatching([{ + start: 0, + end: 0, + replacement: `import {foo as bar} from './file_1';\n` + }]); + expect(results[0].fixToReadableStringInContext()) + .toBe( + `Suggested fix:\n` + + `- Add new import: import {foo as bar} from './file_1';`); + }); + + it('maybeAddNamedImport already there', () => { + const results = compileAndCheck( + new ConformancePatternRule(baseConfig, addNamedImportFixer), + 'import {foo as bar} from \'./file_1\';\n' + source, + 'export const foo = 1;'); + + expect(results[0]).toHaveNoFix(); + expect(results[0].fixToReadableStringInContext()).toBe(''); + }); + + it('maybeAddNamedImport different name', () => { + const results = compileAndCheck( + new ConformancePatternRule(baseConfig, addNamedImportFixer), + 'import {foo as baz} from \'./file_1\';\n' + source, + 'export const foo = 1;'); + + expect(results[0]).toHaveFixMatching([ + {start: 8, end: 8, replacement: `foo as bar, `} + ]); + expect(results[0].fixToReadableStringInContext()) + .toBe( + `Suggested fix:\n` + + `- Insert at line 1, char 9: foo as bar,`); + }); + + it('maybeAddNamespacedImport', () => { + const addNamespacedImportFixer: Fixer = { + getFixForFlaggedNode(n: ts.Node): Fix | + undefined { + const ic = + maybeAddNamespaceImport(n.getSourceFile(), './file_1', 'foo'); + if (ic) return {changes: [ic]}; + return; + } + }; + const results = compileAndCheck( + new ConformancePatternRule(baseConfig, addNamespacedImportFixer), + source); + + expect(results[0]).toHaveFixMatching([ + {start: 0, end: 0, replacement: `import * as foo from './file_1';\n`} + ]); + }); + }); + + describe('the logic for location->text transforms', () => { + const sourceFile = compile(`let a;\nlet b;\n`) + .getSourceFiles() + .filter(f => f.fileName.indexOf('file_0') !== -1)[0]; + // let a;\nlet b;\n + // 0123456 7890123 Positions + // 1234567 1234567 Expected result in characters + + it('stringifies as expected', () => { + // Only the sourceFile matters here. + const failure = new Failure(sourceFile, NaN, NaN, 'whatever', NaN); + + expect(failure.readableRange(0, 0)).toBe('at line 1, char 1'); + expect(failure.readableRange(1, 1)).toBe('at line 1, char 2'); + expect(failure.readableRange(0, 1)).toBe('line 1, from char 1 to 2'); + expect(failure.readableRange(0, 1)).toBe('line 1, from char 1 to 2'); + expect(failure.readableRange(7, 7)).toBe('at line 2, char 1'); + expect(failure.readableRange(0, 7)) + .toBe('from line 1, char 1 to line 2, char 1'); }); }); }); + + beforeEach(() => { jasmine.addMatchers(customMatchers); });
diff --git a/internal/tsetse/util/testing/test_support.ts b/internal/tsetse/util/testing/test_support.ts index c12e28a..fb2460e 100644 --- a/internal/tsetse/util/testing/test_support.ts +++ b/internal/tsetse/util/testing/test_support.ts
@@ -213,7 +213,22 @@ return {pass: regrets === '', message: regrets}; } }; + }, + + /** + * Asserts that a Failure has no fix. + */ + toHaveNoFix(): jasmine.CustomMatcher { + return { + compare: (actualFailure: Failure) => { + return { + pass: actualFailure.toDiagnostic().fix === undefined, + message: 'This failure should not have a fix.' + }; + } + }; } + }; function expectation(fieldname: string, expectation: any, actual: any) { @@ -232,11 +247,15 @@ messageText?: string, }): void; + /** Checks that a Failure has the expected Fix field. */ toHaveFixMatching(expected: [ {fileName?: string, start?: number, end?: number, replacement?: string} ]): void; toHaveNFailures(expected: Number, config?: Config): void; + + /** Asserts that a Failure has no fix. */ + toHaveNoFix(): void; } } }