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;
}
}
}
