From b2dd610e92ca9f68f24f589f727459b586fc5d43 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Tue, 6 Mar 2018 12:19:19 -0800 Subject: [PATCH] Fix ignore message indentation (#22340) * Update baselines for user tests * Add explicit indentation * Fix https://github.com/Microsoft/TypeScript/issues/21355: Format `// @ts-ignore` added by quick fix * Extract check to a separate function * Consolidate checking for valid insert location * Code review comments * Do not return makeChange --- .../codefixes/disableJsDiagnostics.ts | 77 +++++++++++-------- src/services/textChanges.ts | 8 +- .../codeFixDisableJsDiagnosticsInFile6.ts | 8 +- .../codeFixDisableJsDiagnosticsInFile7.ts | 8 +- .../codeFixDisableJsDiagnosticsInFile8.ts | 10 +-- 5 files changed, 62 insertions(+), 49 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 091ee07a9c..57778d7352 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -9,60 +9,69 @@ namespace ts.codefix { registerCodeFix({ errorCodes, getCodeActions(context) { - const { sourceFile, program, span } = context; + const { sourceFile, program, span, host, formatContext } = context; if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions())) { return undefined; } - const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); + const fixes: CodeFixAction[] = [ + { + description: getLocaleSpecificMessage(Diagnostics.Disable_checking_for_this_file), + changes: [createFileTextChanges(sourceFile.fileName, [ + createTextChange(sourceFile.checkJsDirective + ? createTextSpanFromBounds(sourceFile.checkJsDirective.pos, sourceFile.checkJsDirective.end) + : createTextSpan(0, 0), `// @ts-nocheck${getNewLineOrDefaultFromHost(host, formatContext.options)}`), + ])], + // fixId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file. + fixId: undefined, + }]; - return [{ - description: getLocaleSpecificMessage(Diagnostics.Ignore_this_error_message), - changes: [createFileTextChanges(sourceFile.fileName, [getIgnoreCommentLocationForLocation(sourceFile, span.start, newLineCharacter).change])], - fixId, - }, - { - description: getLocaleSpecificMessage(Diagnostics.Disable_checking_for_this_file), - changes: [createFileTextChanges(sourceFile.fileName, [ - createTextChange(sourceFile.checkJsDirective ? createTextSpanFromBounds(sourceFile.checkJsDirective.pos, sourceFile.checkJsDirective.end) : createTextSpan(0, 0), `// @ts-nocheck${newLineCharacter}`), - ])], - // fixId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file. - fixId: undefined, - }]; + if (isValidSuppressLocation(sourceFile, span.start)) { + fixes.unshift({ + description: getLocaleSpecificMessage(Diagnostics.Ignore_this_error_message), + changes: textChanges.ChangeTracker.with(context, t => makeChange(t, sourceFile, span.start)), + fixId, + }); + } + + return fixes; }, fixIds: [fixId], getAllCodeActions: context => { - const seenLines = createMap(); // Only need to add `// @ts-ignore` for a line once. - return codeFixAllWithTextChanges(context, errorCodes, (changes, err) => { - if (err.start !== undefined) { - const { lineNumber, change } = getIgnoreCommentLocationForLocation(err.file!, err.start, getNewLineOrDefaultFromHost(context.host, context.formatContext.options)); - if (addToSeen(seenLines, lineNumber)) { - changes.push(change); - } + const seenLines = createMap(); + return codeFixAll(context, errorCodes, (changes, diag) => { + if (isValidSuppressLocation(diag.file!, diag.start!)) { + makeChange(changes, diag.file!, diag.start!, seenLines); } }); }, }); - function getIgnoreCommentLocationForLocation(sourceFile: SourceFile, position: number, newLineCharacter: string): { lineNumber: number, change: TextChange } { + function isValidSuppressLocation(sourceFile: SourceFile, position: number) { + return !isInComment(sourceFile, position) && !isInString(sourceFile, position) && !isInTemplateString(sourceFile, position); + } + + function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, seenLines?: Map) { const { line: lineNumber } = getLineAndCharacterOfPosition(sourceFile, position); + + // Only need to add `// @ts-ignore` for a line once. + if (seenLines && !addToSeen(seenLines, lineNumber)) { + return; + } + const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); // First try to see if we can put the '// @ts-ignore' on the previous line. // We need to make sure that we are not in the middle of a string literal or a comment. - // We also want to check if the previous line holds a comment for a node on the next line - // if so, we do not want to separate the node from its comment if we can. - if (!isInComment(sourceFile, startPosition) && !isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition)) { - const token = getTouchingToken(sourceFile, startPosition, /*includeJsDocComment*/ false); - const tokenLeadingComments = getLeadingCommentRangesOfNode(token, sourceFile); - if (!tokenLeadingComments || !tokenLeadingComments.length || tokenLeadingComments[0].pos >= startPosition) { - return { lineNumber, change: createTextChangeFromStartLength(startPosition, 0, `// @ts-ignore${newLineCharacter}`) }; - } - } + // If so, we do not want to separate the node from its comment if we can. + // Otherwise, add an extra new line immediately before the error span. + const insertAtLineStart = isValidSuppressLocation(sourceFile, startPosition); - // If all fails, add an extra new line immediately before the error span. - return { lineNumber, change: createTextChangeFromStartLength(position, 0, `${position === startPosition ? "" : newLineCharacter}// @ts-ignore${newLineCharacter}`) }; + const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position, /*includeJsDocComment*/ false); + const clone = setStartsOnNewLine(getSynthesizedDeepClone(token), true); + addSyntheticLeadingComment(clone, SyntaxKind.SingleLineCommentTrivia, " @ts-ignore"); + changes.replaceNode(sourceFile, token, clone, { preserveLeadingWhitespace: true, prefix: insertAtLineStart ? undefined : changes.newLineCharacter }); } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 133fd8f96b..0e75158c4d 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -94,6 +94,10 @@ namespace ts.textChanges { * Text of inserted node will be formatted with this delta, otherwise delta will be inferred from the new node kind */ delta?: number; + /** + * Do not trim leading white spaces in the edit range + */ + preserveLeadingWhitespace?: boolean; } enum ChangeKind { @@ -212,7 +216,7 @@ namespace ts.textChanges { } /** Public for tests only. Other callers should use `ChangeTracker.with`. */ - constructor(private readonly newLineCharacter: string, private readonly formatContext: formatting.FormatContext) {} + constructor(public readonly newLineCharacter: string, private readonly formatContext: formatting.FormatContext) {} public deleteRange(sourceFile: SourceFile, range: TextRange) { this.changes.push({ kind: ChangeKind.Remove, sourceFile, range }); @@ -628,7 +632,7 @@ namespace ts.textChanges { ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(newLineCharacter) : format(change.node); // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line - const noIndent = (options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, ""); + const noIndent = (options.preserveLeadingWhitespace || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, ""); return (options.prefix || "") + noIndent + (options.suffix || ""); } diff --git a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts index 5b93f4953d..952628c6ca 100644 --- a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts +++ b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts @@ -7,11 +7,11 @@ // @Filename: a.js ////var x = 0; //// -////function f(_a) { -//// [|f(x());|] -////} +////function f(_a) {[| +//// f(x()); +////|]} // Disable checking for next line -verify.rangeAfterCodeFix(`// @ts-ignore +verify.rangeAfterCodeFix(` // @ts-ignore f(x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0); diff --git a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts index 767fa98430..11f027d4f3 100644 --- a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts +++ b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts @@ -7,11 +7,11 @@ // @Filename: a.js ////var x = 0; //// -////function f(_a) { -//// [|x();|] -////} +////function f(_a) {[| +//// x(); +////|]} // Disable checking for next line -verify.rangeAfterCodeFix(`// @ts-ignore +verify.rangeAfterCodeFix(`// @ts-ignore x();`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0); diff --git a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts index 3df5864f19..334b275c87 100644 --- a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts +++ b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts @@ -7,13 +7,13 @@ // @Filename: a.js ////var x = 0; //// -////function f(_a) { +////function f(_a) {[| //// /** comment for f */ -//// [|f(x());|] -////} +//// f(x()); +////|]} // Disable checking for next line -verify.rangeAfterCodeFix(`f( +verify.rangeAfterCodeFix(` /** comment for f */ // @ts-ignore - x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0); + f(x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0);