From e51d0a7ed8224dce073cc9fbbaf4507e9a9fc446 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Thu, 1 Mar 2018 14:33:57 -0800 Subject: [PATCH 1/7] Update baselines for user tests --- .../reference/user/chrome-devtools-frontend.log | 16 ++-------------- tests/baselines/reference/user/leveldown.log | 8 -------- tests/baselines/reference/user/sift.log | 8 -------- 3 files changed, 2 insertions(+), 30 deletions(-) delete mode 100644 tests/baselines/reference/user/leveldown.log delete mode 100644 tests/baselines/reference/user/sift.log diff --git a/tests/baselines/reference/user/chrome-devtools-frontend.log b/tests/baselines/reference/user/chrome-devtools-frontend.log index ff54d52ea8640..1d046e53073ed 100644 --- a/tests/baselines/reference/user/chrome-devtools-frontend.log +++ b/tests/baselines/reference/user/chrome-devtools-frontend.log @@ -18,12 +18,9 @@ Standard output: node_modules/chrome-devtools-frontend/front_end/Runtime.js(43,8): error TS2339: Property '_importScriptPathPrefix' does not exist on type 'Window'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(95,28): error TS2339: Property 'response' does not exist on type 'EventTarget'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(147,37): error TS2339: Property '_importScriptPathPrefix' does not exist on type 'Window'. -node_modules/chrome-devtools-frontend/front_end/Runtime.js(158,21): error TS2345: Argument of type 'Promise' is not assignable to parameter of type 'Promise'. - Type 'string' is not assignable to type 'undefined'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(161,5): error TS2322: Type 'Promise' is not assignable to type 'Promise'. Type 'undefined[]' is not assignable to type 'undefined'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(187,12): error TS2339: Property 'eval' does not exist on type 'Window'. -node_modules/chrome-devtools-frontend/front_end/Runtime.js(197,5): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(219,13): error TS2339: Property 'timeStamp' does not exist on type 'Console'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(267,14): error TS2339: Property 'runtime' does not exist on type 'Window'. node_modules/chrome-devtools-frontend/front_end/Runtime.js(269,59): error TS2339: Property 'runtime' does not exist on type 'Window'. @@ -3655,8 +3652,6 @@ node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(341, node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(351,31): error TS2694: Namespace 'Protocol' has no exported member 'Debugger'. node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(362,31): error TS2694: Namespace 'Protocol' has no exported member 'Debugger'. node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(375,9): error TS2322: Type 'Promise' is not assignable to type 'Promise'. -node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(378,9): error TS2322: Type 'Promise' is not assignable to type 'Promise'. -node_modules/chrome-devtools-frontend/front_end/bindings/BlackboxManager.js(381,5): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/bindings/BreakpointManager.js(43,52): error TS2339: Property 'Storage' does not exist on type 'typeof (Anonymous class)'. node_modules/chrome-devtools-frontend/front_end/bindings/BreakpointManager.js(49,89): error TS2694: Namespace 'Bindings' has no exported member 'BreakpointManager'. node_modules/chrome-devtools-frontend/front_end/bindings/BreakpointManager.js(51,63): error TS2694: Namespace 'Bindings' has no exported member 'BreakpointManager'. @@ -8056,7 +8051,6 @@ node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(28 node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2906,7): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2910,7): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2918,7): error TS2322: Type 'Promise' is not assignable to type 'Promise'. -node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2956,5): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2977,107): error TS1003: Identifier expected. node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2978,35): error TS2300: Duplicate identifier 'Context'. node_modules/chrome-devtools-frontend/front_end/elements/StylesSidebarPane.js(2978,35): error TS2339: Property 'Context' does not exist on type 'typeof (Anonymous class)'. @@ -13090,9 +13084,9 @@ node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1057,39): node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1108,15): error TS2339: Property 'valuesArray' does not exist on type 'Set'. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1116,15): error TS2339: Property 'firstValue' does not exist on type 'Set'. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1126,15): error TS2339: Property 'addAll' does not exist on type 'Set'. -node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1127,17): error TS2495: Type 'Iterable | T[]' is not an array type or a string type. +node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1127,17): error TS2495: Type 'T[] | Iterable' is not an array type or a string type. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1136,15): error TS2339: Property 'containsAll' does not exist on type 'Set'. -node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1137,17): error TS2495: Type 'Iterable | T[]' is not an array type or a string type. +node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1137,17): error TS2495: Type 'T[] | Iterable' is not an array type or a string type. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1148,15): error TS2339: Property 'remove' does not exist on type 'Map'. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1155,21): error TS2304: Cannot find name 'VALUE'. node_modules/chrome-devtools-frontend/front_end/platform/utilities.js(1157,15): error TS2339: Property 'valuesArray' does not exist on type 'Map'. @@ -16337,10 +16331,6 @@ node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1153,31): er node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1176,31): error TS2694: Namespace 'Protocol' has no exported member 'Runtime'. node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1234,21): error TS2694: Namespace 'SDK' has no exported member 'CallFunctionResult'. node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1265,21): error TS2694: Namespace 'SDK' has no exported member 'CallFunctionResult'. -node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1325,5): error TS2322: Type 'Promise<{ properties: (Anonymous class)[]; internalProperties: (Anonymous class)[]; }>' is not assignable to type 'Promise<(Anonymous class)>'. -node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1325,5): error TS2322: Type 'Promise<{ properties: (Anonymous class)[]; internalProperties: (Anonymous class)[]; }>' is not assignable to type 'Promise<(Anonymous class)>'. - Type '{ properties: (Anonymous class)[]; internalProperties: (Anonymous class)[]; }' is not assignable to type '(Anonymous class)'. - Property 'customPreview' is missing in type '{ properties: (Anonymous class)[]; internalProperties: (Anonymous class)[]; }'. node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1345,29): error TS2694: Namespace 'SDK' has no exported member 'DebuggerModel'. node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1352,31): error TS2694: Namespace 'SDK' has no exported member 'DebuggerModel'. node_modules/chrome-devtools-frontend/front_end/sdk/RemoteObject.js(1363,21): error TS2694: Namespace 'SDK' has no exported member 'DebuggerModel'. @@ -18006,12 +17996,10 @@ node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSid node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(73,36): error TS2339: Property 'createChild' does not exist on type 'Node'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(74,56): error TS2339: Property '_snippetElementSymbol' does not exist on type 'typeof (Anonymous class)'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(77,51): error TS2339: Property 'get' does not exist on type '{ _map: Map>; }'. -node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(78,11): error TS2403: Subsequent variable declarations must have the same type. Variable 'uiLocation' must be of type '(Anonymous class)', but here has type 'any'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(91,13): error TS2339: Property 'remove' does not exist on type 'Node'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(110,54): error TS2339: Property '_locationSymbol' does not exist on type 'typeof (Anonymous class)'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(113,74): error TS2339: Property '_checkboxLabelSymbol' does not exist on type 'typeof (Anonymous class)'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(118,75): error TS2339: Property '_snippetElementSymbol' does not exist on type 'typeof (Anonymous class)'. -node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(119,5): error TS2322: Type 'Promise' is not assignable to type 'Promise'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(141,29): error TS2339: Property 'enclosingNodeOrSelfWithClass' does not exist on type 'EventTarget'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(144,58): error TS2339: Property '_locationSymbol' does not exist on type 'typeof (Anonymous class)'. node_modules/chrome-devtools-frontend/front_end/sources/JavaScriptBreakpointsSidebarPane.js(156,33): error TS2339: Property 'checkboxElement' does not exist on type 'EventTarget'. diff --git a/tests/baselines/reference/user/leveldown.log b/tests/baselines/reference/user/leveldown.log deleted file mode 100644 index 170ed7ca0a39c..0000000000000 --- a/tests/baselines/reference/user/leveldown.log +++ /dev/null @@ -1,8 +0,0 @@ -Exit Code: 1 -Standard output: -node_modules/abstract-leveldown/index.d.ts(43,27): error TS1005: ',' expected. -node_modules/abstract-leveldown/index.d.ts(43,28): error TS1139: Type parameter declaration expected. - - - -Standard error: diff --git a/tests/baselines/reference/user/sift.log b/tests/baselines/reference/user/sift.log deleted file mode 100644 index 62386ea0fca20..0000000000000 --- a/tests/baselines/reference/user/sift.log +++ /dev/null @@ -1,8 +0,0 @@ -Exit Code: 1 -Standard output: -node_modules/sift/index.d.ts(22,54): error TS2344: Type 'T[0][index]' does not satisfy the constraint 'any[]'. -node_modules/sift/index.d.ts(32,35): error TS2344: Type 'T[0][P]' does not satisfy the constraint 'any[]'. - - - -Standard error: From 1b5629bfa84c974fe892959f0d4f427a0fd1247f Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Mon, 5 Mar 2018 11:36:53 -0800 Subject: [PATCH 2/7] Add explicit indentation --- src/services/codefixes/disableJsDiagnostics.ts | 5 +++-- .../cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts | 8 ++++---- .../cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts | 8 ++++---- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 091ee07a9cc0a..5cc44d0fbb14f 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -49,6 +49,7 @@ namespace ts.codefix { const { line: lineNumber } = getLineAndCharacterOfPosition(sourceFile, position); const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); + const indentation = sourceFile.text.substring(lineStartPosition, startPosition); // 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. @@ -58,11 +59,11 @@ namespace ts.codefix { 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}`) }; + return { lineNumber, change: createTextChangeFromStartLength(lineStartPosition, 0, `${indentation}// @ts-ignore${newLineCharacter}`) }; } } // 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}`) }; + return { lineNumber, change: createTextChangeFromStartLength(position, 0, `${position === startPosition ? "" : newLineCharacter}${indentation}// @ts-ignore${newLineCharacter}${indentation}`) }; } } diff --git a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts index 5b93f4953d1fc..952628c6cac89 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 767fa98430465..11f027d4f35b9 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); From 2e5bf3694bd4b834bd3e064c4aa0e151ae722d98 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Mon, 5 Mar 2018 12:13:13 -0800 Subject: [PATCH 3/7] Fix https://github.com/Microsoft/TypeScript/issues/21355: Format `// @ts-ignore` added by quick fix --- .../codefixes/disableJsDiagnostics.ts | 50 +++++++++---------- src/services/textChanges.ts | 6 ++- .../codeFixDisableJsDiagnosticsInFile8.ts | 10 ++-- 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 5cc44d0fbb14f..49e10462bd002 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -9,17 +9,17 @@ 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 newLineCharacter = getNewLineOrDefaultFromHost(host, formatContext.options); return [{ description: getLocaleSpecificMessage(Diagnostics.Ignore_this_error_message), - changes: [createFileTextChanges(sourceFile.fileName, [getIgnoreCommentLocationForLocation(sourceFile, span.start, newLineCharacter).change])], + changes: textChanges.ChangeTracker.with(context, t => makeChange(t, sourceFile, span.start, newLineCharacter)), fixId, }, { @@ -33,37 +33,37 @@ namespace ts.codefix { }, 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 newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); + const seenLines = createMap(); + return codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file!, diag.start!, newLineCharacter, seenLines)); }, }); - function getIgnoreCommentLocationForLocation(sourceFile: SourceFile, position: number, newLineCharacter: string): { lineNumber: number, change: TextChange } { + function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, newLineCharacter: string, seenLines?: Map) { + if (isInComment(sourceFile, position) || isInString(sourceFile, position) || isInTemplateString(sourceFile, position)) { + return; + } + 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); - const indentation = sourceFile.text.substring(lineStartPosition, startPosition); // 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(lineStartPosition, 0, `${indentation}// @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 = !isInComment(sourceFile, startPosition) && + !isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition); - // If all fails, add an extra new line immediately before the error span. - return { lineNumber, change: createTextChangeFromStartLength(position, 0, `${position === startPosition ? "" : newLineCharacter}${indentation}// @ts-ignore${newLineCharacter}${indentation}`) }; + 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, { preseveLeadingWhiteSpaces: true, prefix: insertAtLineStart ? undefined : newLineCharacter }); } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 133fd8f96bd40..f3e9e83d42e65 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 + */ + preseveLeadingWhiteSpaces?: boolean; } enum ChangeKind { @@ -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.preseveLeadingWhiteSpaces || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, ""); return (options.prefix || "") + noIndent + (options.suffix || ""); } diff --git a/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts b/tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts index 3df5864f198b9..334b275c87771 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); From 9a8705b9bfe27f33fcb48f288555f00b49123619 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Mon, 5 Mar 2018 14:32:36 -0800 Subject: [PATCH 4/7] Extract check to a separate function --- src/services/codefixes/disableJsDiagnostics.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 49e10462bd002..b98ebef41e56b 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -11,7 +11,7 @@ namespace ts.codefix { getCodeActions(context) { const { sourceFile, program, span, host, formatContext } = context; - if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions())) { + if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions()) || !isValidSuppressLocation(sourceFile, span.start)) { return undefined; } @@ -35,15 +35,19 @@ namespace ts.codefix { getAllCodeActions: context => { const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); const seenLines = createMap(); - return codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file!, diag.start!, newLineCharacter, seenLines)); + return codeFixAll(context, errorCodes, (changes, diag) => { + if (isValidSuppressLocation(diag.file!, diag.start!)) { + return makeChange(changes, diag.file!, diag.start!, newLineCharacter, seenLines); + } + }); }, }); - function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, newLineCharacter: string, seenLines?: Map) { - if (isInComment(sourceFile, position) || isInString(sourceFile, position) || isInTemplateString(sourceFile, position)) { - return; - } + 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, newLineCharacter: string, seenLines?: Map) { const { line: lineNumber } = getLineAndCharacterOfPosition(sourceFile, position); // Only need to add `// @ts-ignore` for a line once. From 81e6b84e8fbc83f30bc69804d4b37326dd43d264 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Mon, 5 Mar 2018 15:27:37 -0800 Subject: [PATCH 5/7] Consolidate checking for valid insert location --- src/services/codefixes/disableJsDiagnostics.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index b98ebef41e56b..bdfe27b3c90d7 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -62,8 +62,7 @@ namespace ts.codefix { // We need to make sure that we are not in the middle of a string literal or a comment. // 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 = !isInComment(sourceFile, startPosition) && - !isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition); + const insertAtLineStart = isValidSuppressLocation(sourceFile, startPosition); const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position, /*includeJsDocComment*/ false); const clone = setStartsOnNewLine(getSynthesizedDeepClone(token), true); From f4902275447758b12bfd4280872eda4850d471f2 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Tue, 6 Mar 2018 10:41:26 -0800 Subject: [PATCH 6/7] Code review comments --- .../codefixes/disableJsDiagnostics.ts | 43 +++++++++++-------- src/services/textChanges.ts | 6 +-- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index bdfe27b3c90d7..235069f5d8dc0 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -11,33 +11,38 @@ namespace ts.codefix { getCodeActions(context) { const { sourceFile, program, span, host, formatContext } = context; - if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions()) || !isValidSuppressLocation(sourceFile, span.start)) { + if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions())) { return undefined; } - const newLineCharacter = getNewLineOrDefaultFromHost(host, 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: textChanges.ChangeTracker.with(context, t => makeChange(t, sourceFile, span.start, newLineCharacter)), - 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 newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); const seenLines = createMap(); return codeFixAll(context, errorCodes, (changes, diag) => { if (isValidSuppressLocation(diag.file!, diag.start!)) { - return makeChange(changes, diag.file!, diag.start!, newLineCharacter, seenLines); + return makeChange(changes, diag.file!, diag.start!, seenLines); } }); }, @@ -47,7 +52,7 @@ namespace ts.codefix { return !isInComment(sourceFile, position) && !isInString(sourceFile, position) && !isInTemplateString(sourceFile, position); } - function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, newLineCharacter: string, seenLines?: Map) { + 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. @@ -67,6 +72,6 @@ namespace ts.codefix { 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, { preseveLeadingWhiteSpaces: true, prefix: insertAtLineStart ? undefined : newLineCharacter }); + 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 f3e9e83d42e65..0e75158c4d1c3 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -97,7 +97,7 @@ namespace ts.textChanges { /** * Do not trim leading white spaces in the edit range */ - preseveLeadingWhiteSpaces?: boolean; + preserveLeadingWhitespace?: boolean; } enum ChangeKind { @@ -216,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 }); @@ -632,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.preseveLeadingWhiteSpaces || 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 || ""); } From acc0783a40c5a02d3d3d5363790fad8156440482 Mon Sep 17 00:00:00 2001 From: Mohamed Hegazy Date: Tue, 6 Mar 2018 11:01:35 -0800 Subject: [PATCH 7/7] Do not return makeChange --- src/services/codefixes/disableJsDiagnostics.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/codefixes/disableJsDiagnostics.ts b/src/services/codefixes/disableJsDiagnostics.ts index 235069f5d8dc0..57778d7352607 100644 --- a/src/services/codefixes/disableJsDiagnostics.ts +++ b/src/services/codefixes/disableJsDiagnostics.ts @@ -42,7 +42,7 @@ namespace ts.codefix { const seenLines = createMap(); return codeFixAll(context, errorCodes, (changes, diag) => { if (isValidSuppressLocation(diag.file!, diag.start!)) { - return makeChange(changes, diag.file!, diag.start!, seenLines); + makeChange(changes, diag.file!, diag.start!, seenLines); } }); },