From 7a1f97a61bb5c73b52aa60797d062ff67bd81c51 Mon Sep 17 00:00:00 2001 From: Egor Verbitskiy Date: Fri, 21 Jul 2023 09:56:39 -0700 Subject: [PATCH] fix(54666): Codefix `convertTypedefToType to work for multiple typedefs in a row (#54667) Co-authored-by: everbits --- .../codefixes/convertTypedefToType.ts | 139 +++++++++--- src/services/suggestionDiagnostics.ts | 4 +- .../fourslash/codeFixConvertTypedefToType4.ts | 2 +- .../fourslash/codeFixConvertTypedefToType6.ts | 199 ++++++++++++++++++ .../fourslash/codeFixConvertTypedefToType7.ts | 75 +++++++ 5 files changed, 382 insertions(+), 37 deletions(-) create mode 100644 tests/cases/fourslash/codeFixConvertTypedefToType6.ts create mode 100644 tests/cases/fourslash/codeFixConvertTypedefToType7.ts diff --git a/src/services/codefixes/convertTypedefToType.ts b/src/services/codefixes/convertTypedefToType.ts index 5cf04b1aaa8..fb6016d1cab 100644 --- a/src/services/codefixes/convertTypedefToType.ts +++ b/src/services/codefixes/convertTypedefToType.ts @@ -1,14 +1,17 @@ import { Diagnostics, factory, - forEach, + flatMap, + getNewLineOrDefaultFromHost, getSynthesizedDeepClone, getTokenAtPosition, hasJSDocNodes, InterfaceDeclaration, isJSDocTypedefTag, isJSDocTypeLiteral, + JSDoc, JSDocPropertyLikeTag, + JSDocTag, JSDocTypedefTag, JSDocTypeExpression, JSDocTypeLiteral, @@ -29,12 +32,14 @@ registerCodeFix({ fixIds: [fixId], errorCodes, getCodeActions(context) { + const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); const node = getTokenAtPosition( context.sourceFile, context.span.start ); if (!node) return; - const changes = textChanges.ChangeTracker.with(context, t => doChange(t, node, context.sourceFile)); + + const changes = textChanges.ChangeTracker.with(context, t => doChange(t, node, context.sourceFile, newLineCharacter)); if (changes.length > 0) { return [ @@ -48,35 +53,102 @@ registerCodeFix({ ]; } }, - getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => { - const node = getTokenAtPosition(diag.file, diag.start); - if (node) doChange(changes, node, diag.file); - }) + getAllCodeActions: context => codeFixAll( + context, + errorCodes, + (changes, diag) => { + const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); + const node = getTokenAtPosition(diag.file, diag.start); + const fixAll = true; + if (node) doChange(changes, node, diag.file, newLineCharacter, fixAll); + } + ) }); -function doChange(changes: textChanges.ChangeTracker, node: Node, sourceFile: SourceFile) { - if (isJSDocTypedefTag(node)) { - fixSingleTypeDef(changes, node, sourceFile); - } -} - -function fixSingleTypeDef( +function doChange( changes: textChanges.ChangeTracker, - typeDefNode: JSDocTypedefTag | undefined, + node: Node, sourceFile: SourceFile, + newLine: string, + fixAll = false ) { - if (!typeDefNode) return; + if (!isJSDocTypedefTag(node)) return; - const declaration = createDeclaration(typeDefNode); + const declaration = createDeclaration(node); if (!declaration) return; - const comment = typeDefNode.parent; + const commentNode = node.parent; - changes.replaceNode( - sourceFile, - comment, - declaration + const { leftSibling, rightSibling } = getLeftAndRightSiblings(node); + + let pos = commentNode.getStart(); + let prefix = ""; + + // the first @typedef is the comment block with a text comment above + if (!leftSibling && commentNode.comment) { + pos = findEndOfTextBetween(commentNode, commentNode.getStart(), node.getStart()); + prefix = `${newLine} */${newLine}`; + } + + if (leftSibling) { + if (fixAll && isJSDocTypedefTag(leftSibling)) { + // Don't need to keep empty comment clock between created interfaces + pos = node.getStart(); + prefix = ""; + } + else { + pos = findEndOfTextBetween(commentNode, leftSibling.getStart(), node.getStart()); + prefix = `${newLine} */${newLine}`; + } + } + + let end = commentNode.getEnd(); + let suffix = ""; + + if (rightSibling) { + if (fixAll && isJSDocTypedefTag(rightSibling)) { + // Don't need to keep empty comment clock between created interfaces + end = rightSibling.getStart(); + suffix = `${newLine}${newLine}`; + } + else { + end = rightSibling.getStart(); + suffix = `${newLine}/**${newLine} * `; + } + } + + changes.replaceRange(sourceFile, { pos, end }, declaration, { prefix, suffix }); +} + +function getLeftAndRightSiblings(typedefNode: JSDocTypedefTag): { leftSibling?: Node, rightSibling?: Node } { + + const commentNode = typedefNode.parent; + const maxChildIndex = commentNode.getChildCount() - 1; + + const currentNodeIndex = commentNode.getChildren().findIndex( + (n) => n.getStart() === typedefNode.getStart() && n.getEnd() === typedefNode.getEnd() ); + + const leftSibling = currentNodeIndex > 0 ? commentNode.getChildAt(currentNodeIndex - 1) : undefined; + const rightSibling = currentNodeIndex < maxChildIndex ? commentNode.getChildAt(currentNodeIndex + 1) : undefined; + + return { leftSibling, rightSibling }; +} + +/** + * Finds the index of the last meaningful symbol (except empty spaces, * and /) in the comment + * between start and end positions + */ +function findEndOfTextBetween(jsDocComment: JSDoc, from: number, to: number): number { + const comment = jsDocComment.getText().substring(from - jsDocComment.getStart(), to - jsDocComment.getStart()); + + for (let i = comment.length; i > 0; i--) { + if(!/[*\/\s]/g.test(comment.substring(i - 1, i))) { + return from + i; + } + } + + return to; } function createDeclaration(tag: JSDocTypedefTag): InterfaceDeclaration | TypeAliasDeclaration | undefined { @@ -86,7 +158,7 @@ function createDeclaration(tag: JSDocTypedefTag): InterfaceDeclaration | TypeAli if (!typeName) return; // For use case @typedef {object}Foo @property{bar}number - // But object type can be nested, meaning the value in the k/v pair can be object itself + // But object type can be nested, meaning the value in the k/v pair can be the object itself if (typeExpression.kind === SyntaxKind.JSDocTypeLiteral) { return createInterfaceForTypeLiteral(typeName, typeExpression); } @@ -103,14 +175,14 @@ function createInterfaceForTypeLiteral( ): InterfaceDeclaration | undefined { const propertySignatures = createSignatureFromTypeLiteral(typeLiteral); if (!some(propertySignatures)) return; - const interfaceDeclaration = factory.createInterfaceDeclaration( + + return factory.createInterfaceDeclaration( /*modifiers*/ undefined, typeName, /*typeParameters*/ undefined, /*heritageClauses*/ undefined, propertySignatures, ); - return interfaceDeclaration; } function createTypeAliasForTypeExpression( @@ -119,13 +191,13 @@ function createTypeAliasForTypeExpression( ): TypeAliasDeclaration | undefined { const typeReference = getSynthesizedDeepClone(typeExpression.type); if (!typeReference) return; - const declaration = factory.createTypeAliasDeclaration( + + return factory.createTypeAliasDeclaration( /*modifiers*/ undefined, factory.createIdentifier(typeName), /*typeParameters*/ undefined, typeReference ); - return declaration; } function createSignatureFromTypeLiteral(typeLiteral: JSDocTypeLiteral): PropertySignature[] | undefined { @@ -150,19 +222,17 @@ function createSignatureFromTypeLiteral(typeLiteral: JSDocTypeLiteral): Property if (typeReference && name) { const questionToken = isOptional ? factory.createToken(SyntaxKind.QuestionToken) : undefined; - const prop = factory.createPropertySignature( + + return factory.createPropertySignature( /*modifiers*/ undefined, name, questionToken, typeReference ); - - return prop; } }; - const props = mapDefined(propertyTags, getSignature); - return props; + return mapDefined(propertyTags, getSignature); } function getPropertyName(tag: JSDocPropertyLikeTag): string | undefined { @@ -170,9 +240,10 @@ function getPropertyName(tag: JSDocPropertyLikeTag): string | undefined { } /** @internal */ -export function getJSDocTypedefNode(node: Node): JSDocTypedefTag | undefined { +export function getJSDocTypedefNodes(node: Node): readonly JSDocTag[] { if (hasJSDocNodes(node)) { - return forEach(node.jsDoc, (node) => node.tags?.find(isJSDocTypedefTag)); + return flatMap(node.jsDoc, (doc) => doc.tags?.filter((tag) => isJSDocTypedefTag(tag))); } - return undefined; + + return []; } diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index 3aa4a2d1096..4e425a1a32d 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -116,8 +116,8 @@ export function computeSuggestionDiagnostics(sourceFile: SourceFile, program: Pr } } - const jsdocTypedefNode = codefix.getJSDocTypedefNode(node); - if (jsdocTypedefNode) { + const jsdocTypedefNodes = codefix.getJSDocTypedefNodes(node); + for (const jsdocTypedefNode of jsdocTypedefNodes) { diags.push(createDiagnosticForNode(jsdocTypedefNode, Diagnostics.JSDoc_typedef_may_be_converted_to_TypeScript_type)); } diff --git a/tests/cases/fourslash/codeFixConvertTypedefToType4.ts b/tests/cases/fourslash/codeFixConvertTypedefToType4.ts index a84c1f9fda9..926afd5a4bd 100644 --- a/tests/cases/fourslash/codeFixConvertTypedefToType4.ts +++ b/tests/cases/fourslash/codeFixConvertTypedefToType4.ts @@ -27,4 +27,4 @@ interface Person { }; } `, -}); \ No newline at end of file +}); diff --git a/tests/cases/fourslash/codeFixConvertTypedefToType6.ts b/tests/cases/fourslash/codeFixConvertTypedefToType6.ts new file mode 100644 index 00000000000..2a2bbd9e779 --- /dev/null +++ b/tests/cases/fourslash/codeFixConvertTypedefToType6.ts @@ -0,0 +1,199 @@ +/// + +//// +//// /** +//// * @typedef {number} FooNum +//// */ +//// +//// /** +//// * Some text above the typedef +//// * +//// * @typedef {(number|string|undefined)} FooSome +//// */ +//// +//// /** +//// * @typedef {string} FooString +//// * +//// * @typedef FooPerson +//// * type {object} +//// * @property {string} id - person's ID +//// * @property name {string} // person's name +//// * @property {number|undefined} age - person's age +//// * +//// * @typedef {object} AnotherFooPerson +//// * @property {object} data +//// * @property {string} data.name +//// * @property {number} data.age +//// * @property {object} data.contact +//// * @property {string} data.contact.address +//// * @property {string} [data.contact.phone] +//// */ +//// + +verify.codeFix({ + description: ts.Diagnostics.Convert_typedef_to_TypeScript_type.message, + index: 0, + newFileContent: ` +type FooNum = number; + +/** + * Some text above the typedef + * + * @typedef {(number|string|undefined)} FooSome + */ + +/** + * @typedef {string} FooString + * + * @typedef FooPerson + * type {object} + * @property {string} id - person's ID + * @property name {string} // person's name + * @property {number|undefined} age - person's age + * + * @typedef {object} AnotherFooPerson + * @property {object} data + * @property {string} data.name + * @property {number} data.age + * @property {object} data.contact + * @property {string} data.contact.address + * @property {string} [data.contact.phone] + */ +`, +}); + +verify.codeFix({ + description: ts.Diagnostics.Convert_typedef_to_TypeScript_type.message, + index: 1, + newFileContent: ` +/** + * @typedef {number} FooNum + */ + +/** + * Some text above the typedef + */ +type FooSome = (number | string | undefined); + +/** + * @typedef {string} FooString + * + * @typedef FooPerson + * type {object} + * @property {string} id - person's ID + * @property name {string} // person's name + * @property {number|undefined} age - person's age + * + * @typedef {object} AnotherFooPerson + * @property {object} data + * @property {string} data.name + * @property {number} data.age + * @property {object} data.contact + * @property {string} data.contact.address + * @property {string} [data.contact.phone] + */ +`, +}); + +verify.codeFix({ + description: ts.Diagnostics.Convert_typedef_to_TypeScript_type.message, + index: 2, + newFileContent: ` +/** + * @typedef {number} FooNum + */ + +/** + * Some text above the typedef + * + * @typedef {(number|string|undefined)} FooSome + */ + +type FooString = string; +/** + * @typedef FooPerson + * type {object} + * @property {string} id - person's ID + * @property name {string} // person's name + * @property {number|undefined} age - person's age + * + * @typedef {object} AnotherFooPerson + * @property {object} data + * @property {string} data.name + * @property {number} data.age + * @property {object} data.contact + * @property {string} data.contact.address + * @property {string} [data.contact.phone] + */ +`, +}); + +verify.codeFix({ + description: ts.Diagnostics.Convert_typedef_to_TypeScript_type.message, + index: 3, + newFileContent: ` +/** + * @typedef {number} FooNum + */ + +/** + * Some text above the typedef + * + * @typedef {(number|string|undefined)} FooSome + */ + +/** + * @typedef {string} FooString + */ +interface FooPerson { + id: string; + name: string; + age: number | undefined; +} +/** + * @typedef {object} AnotherFooPerson + * @property {object} data + * @property {string} data.name + * @property {number} data.age + * @property {object} data.contact + * @property {string} data.contact.address + * @property {string} [data.contact.phone] + */ +`, +}); + +verify.codeFix({ + description: ts.Diagnostics.Convert_typedef_to_TypeScript_type.message, + index: 4, + newFileContent: ` +/** + * @typedef {number} FooNum + */ + +/** + * Some text above the typedef + * + * @typedef {(number|string|undefined)} FooSome + */ + +/** + * @typedef {string} FooString + * + * @typedef FooPerson + * type {object} + * @property {string} id - person's ID + * @property name {string} // person's name + * @property {number|undefined} age - person's age + */ +interface AnotherFooPerson { + data: { + name: string; + age: number; + contact: { + address: string; + phone?: string; + }; + }; +} +`, +}); diff --git a/tests/cases/fourslash/codeFixConvertTypedefToType7.ts b/tests/cases/fourslash/codeFixConvertTypedefToType7.ts new file mode 100644 index 00000000000..93db12876cc --- /dev/null +++ b/tests/cases/fourslash/codeFixConvertTypedefToType7.ts @@ -0,0 +1,75 @@ +/// + +//// +//// /** +//// * @typedef {number} FooNum +//// */ +//// +//// /** +//// * Comment above +//// * the typedef +//// * +//// * @typedef {(number|string|undefined)} FooSome +//// */ +//// +//// /** +//// * @typedef {string} Foo1 +//// * +//// * @typedef FooPerson +//// * type {object} +//// * @property {string} id - person's ID +//// * @property name {string} // person's name +//// * @property {number|undefined} age - person's age +//// * +//// * @see another content +//// * +//// * @typedef {object} AnotherFooPerson +//// * @property {object} data +//// * @property {string} data.name +//// * @property {number} data.age +//// * @property {object} data.contact +//// * @property {string} data.contact.address +//// * @property {string} [data.contact.phone] +//// * +//// * +//// * @extends another content +//// */ +//// + +verify.codeFixAll({ + fixId: 'convertTypedefToType', + fixAllDescription: ts.Diagnostics.Convert_all_typedef_to_TypeScript_types.message, + newFileContent: ` +type FooNum = number; + +/** + * Comment above + * the typedef + */ +type FooSome = (number | string | undefined); + +type Foo1 = string; + +interface FooPerson { + id: string; + name: string; + age: number | undefined; +} +/** + * @see another content + */ +interface AnotherFooPerson { + data: { + name: string; + age: number; + contact: { + address: string; + phone?: string; + }; + }; +} +/** + * @extends another content + */ +`, +});