Merge pull request #19304 from Microsoft/dedupe-jsdoc-annotation-refa…

…ctors Fixes for refactor "Annotate with type from JSDoc"
microsoft · Oct 18, 2017 · b40e18d · b40e18d
2 parents f592419 + f82dd7b
commit b40e18d
Show file tree

Hide file tree

Showing 24 changed files with 230 additions and 58 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -7030,13 +7030,11 @@ namespace ts {
         function getIntendedTypeFromJSDocTypeReference(node: TypeReferenceNode): Type {
             if (isIdentifier(node.typeName)) {
                 if (node.typeName.escapedText === "Object") {
-                    if (node.typeArguments && node.typeArguments.length === 2) {
+                    if (isJSDocIndexSignature(node)) {
                         const indexed = getTypeFromTypeNode(node.typeArguments[0]);
                         const target = getTypeFromTypeNode(node.typeArguments[1]);
                         const index = createIndexInfo(target, /*isReadonly*/ false);
-                        if (indexed === stringType || indexed === numberType) {
-                            return createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, indexed === stringType && index, indexed === numberType && index);
-                        }
+                        return createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, indexed === stringType && index, indexed === numberType && index);
                     }
                     return anyType;
                 }
@@ -19179,7 +19177,10 @@ namespace ts {
                             // There is no resolved symbol cached if the type resolved to a builtin
                             // via JSDoc type reference resolution (eg, Boolean became boolean), none
                             // of which are generic when they have no associated symbol
-                            error(node, Diagnostics.Type_0_is_not_generic, typeToString(type));
+                            // (additionally, JSDoc's index signature syntax, Object<string, T> actually uses generic syntax without being generic)
+                            if (!isJSDocIndexSignature(node)) {
+                                error(node, Diagnostics.Type_0_is_not_generic, typeToString(type));
+                            }
                             return;
                         }
                         let typeParameters = symbol.flags & SymbolFlags.TypeAlias && getSymbolLinks(symbol).typeParameters;

diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -1360,6 +1360,14 @@ namespace ts {
         return node && !!(node.flags & NodeFlags.JSDoc);
     }
 
+    export function isJSDocIndexSignature(node: TypeReferenceNode | ExpressionWithTypeArguments) {
+        return isTypeReferenceNode(node) &&
+            isIdentifier(node.typeName) &&
+            node.typeName.escapedText === "Object" &&
+            node.typeArguments && node.typeArguments.length === 2 &&
+            (node.typeArguments[0].kind === SyntaxKind.StringKeyword || node.typeArguments[0].kind === SyntaxKind.NumberKeyword);
+    }
+
     /**
      * Returns true if the node is a CallExpression to the identifier 'require' with
      * exactly one argument (of the form 'require("name")').

diff --git a/src/services/refactors/annotateWithTypeFromJSDoc.ts b/src/services/refactors/annotateWithTypeFromJSDoc.ts
@@ -5,16 +5,9 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
     const annotateTypeFromJSDoc: Refactor = {
         name: "Annotate with type from JSDoc",
         description: Diagnostics.Annotate_with_type_from_JSDoc.message,
-        getEditsForAction: getEditsForAnnotation,
+        getEditsForAction,
         getAvailableActions
     };
-    const annotateFunctionFromJSDoc: Refactor = {
-        name: "Annotate with types from JSDoc",
-        description: Diagnostics.Annotate_with_types_from_JSDoc.message,
-        getEditsForAction: getEditsForFunctionAnnotation,
-        getAvailableActions
-    };
-
     type DeclarationWithType =
         | FunctionLikeDeclaration
         | VariableDeclaration
@@ -23,42 +16,60 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
         | PropertyDeclaration;
 
     registerRefactor(annotateTypeFromJSDoc);
-    registerRefactor(annotateFunctionFromJSDoc);
 
     function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined {
         if (isInJavaScriptFile(context.file)) {
             return undefined;
         }
 
         const node = getTokenAtPosition(context.file, context.startPosition, /*includeJsDocComment*/ false);
-        const decl = findAncestor(node, isDeclarationWithType);
-        if (!decl || decl.type) {
-            return undefined;
-        }
-        const jsdocType = getJSDocType(decl);
-        const isFunctionWithJSDoc = isFunctionLikeDeclaration(decl) && (getJSDocReturnType(decl) || decl.parameters.some(p => !!getJSDocType(p)));
-        const refactor = (isFunctionWithJSDoc || jsdocType && decl.kind === SyntaxKind.Parameter) ? annotateFunctionFromJSDoc :
-            jsdocType ? annotateTypeFromJSDoc :
-            undefined;
-        if (refactor) {
+        if (hasUsableJSDoc(findAncestor(node, isDeclarationWithType))) {
             return [{
-                name: refactor.name,
-                description: refactor.description,
+                name: annotateTypeFromJSDoc.name,
+                description: annotateTypeFromJSDoc.description,
                 actions: [
                     {
-                    description: refactor.description,
-                    name: actionName
-                }
+                        description: annotateTypeFromJSDoc.description,
+                        name: actionName
+                    }
                 ]
             }];
         }
     }
 
-    function getEditsForAnnotation(context: RefactorContext, action: string): RefactorEditInfo | undefined {
+    function hasUsableJSDoc(decl: DeclarationWithType): boolean {
+        if (!decl) {
+            return false;
+        }
+        if (isFunctionLikeDeclaration(decl)) {
+            return decl.parameters.some(hasUsableJSDoc) || (!decl.type && !!getJSDocReturnType(decl));
+        }
+        return !decl.type && !!getJSDocType(decl);
+    }
+
+    function getEditsForAction(context: RefactorContext, action: string): RefactorEditInfo | undefined {
         if (actionName !== action) {
             return Debug.fail(`actionName !== action: ${actionName} !== ${action}`);
         }
+        const node = getTokenAtPosition(context.file, context.startPosition, /*includeJsDocComment*/ false);
+        const decl = findAncestor(node, isDeclarationWithType);
+        if (!decl || decl.type) {
+            return undefined;
+        }
+        const jsdocType = getJSDocType(decl);
+        const isFunctionWithJSDoc = isFunctionLikeDeclaration(decl) && (getJSDocReturnType(decl) || decl.parameters.some(p => !!getJSDocType(p)));
+        if (isFunctionWithJSDoc || jsdocType && decl.kind === SyntaxKind.Parameter) {
+            return getEditsForFunctionAnnotation(context);
+        }
+        else if (jsdocType) {
+            return getEditsForAnnotation(context);
+        }
+        else {
+            Debug.assert(!!refactor, "No applicable refactor found.");
+        }
+    }
 
+    function getEditsForAnnotation(context: RefactorContext): RefactorEditInfo | undefined {
         const sourceFile = context.file;
         const token = getTokenAtPosition(sourceFile, context.startPosition, /*includeJsDocComment*/ false);
         const decl = findAncestor(token, isDeclarationWithType);
@@ -78,11 +89,7 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
         };
     }
 
-    function getEditsForFunctionAnnotation(context: RefactorContext, action: string): RefactorEditInfo | undefined {
-        if (actionName !== action) {
-            return Debug.fail(`actionName !== action: ${actionName} !== ${action}`);
-        }
-
+    function getEditsForFunctionAnnotation(context: RefactorContext): RefactorEditInfo | undefined {
         const sourceFile = context.file;
         const token = getTokenAtPosition(sourceFile, context.startPosition, /*includeJsDocComment*/ false);
         const decl = findAncestor(token, isFunctionLikeDeclaration);
@@ -166,7 +173,9 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
             case SyntaxKind.TypeReference:
                 return transformJSDocTypeReference(node as TypeReferenceNode);
             default:
-                return visitEachChild(node, transformJSDocType, /*context*/ undefined) as TypeNode;
+                const visited = visitEachChild(node, transformJSDocType, /*context*/ undefined) as TypeNode;
+                setEmitFlags(visited, EmitFlags.SingleLine);
+                return visited;
         }
     }
 
@@ -199,6 +208,9 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
         let name = node.typeName;
         let args = node.typeArguments;
         if (isIdentifier(node.typeName)) {
+            if (isJSDocIndexSignature(node)) {
+                return transformJSDocIndexSignature(node);
+            }
             let text = node.typeName.text;
             switch (node.typeName.text) {
                 case "String":
@@ -223,4 +235,18 @@ namespace ts.refactor.annotateWithTypeFromJSDoc {
         }
         return createTypeReferenceNode(name, args);
     }
+
+    function transformJSDocIndexSignature(node: TypeReferenceNode) {
+        const index = createParameter(
+            /*decorators*/ undefined,
+            /*modifiers*/ undefined,
+            /*dotDotDotToken*/ undefined,
+            node.typeArguments[0].kind === SyntaxKind.NumberKeyword ? "n" : "s",
+            /*questionToken*/ undefined,
+            createTypeReferenceNode(node.typeArguments[0].kind === SyntaxKind.NumberKeyword ? "number" : "string", []),
+            /*initializer*/ undefined);
+        const indexSignature = createTypeLiteralNode([createIndexSignature(/*decorators*/ undefined, /*modifiers*/ undefined, [index], node.typeArguments[1])]);
+        setEmitFlags(indexSignature, EmitFlags.SingleLine);
+        return indexSignature;
+    }
 }
diff --git a/tests/baselines/reference/jsdocIndexSignature.errors.txt b/tests/baselines/reference/jsdocIndexSignature.errors.txt
@@ -0,0 +1,18 @@
+tests/cases/conformance/jsdoc/indices.js(9,5): error TS2322: Type '1' is not assignable to type 'boolean'.
+
+
+==== tests/cases/conformance/jsdoc/indices.js (1 errors) ====
+    /** @type {Object.<string, number>} */
+    var o1;
+    /** @type {Object.<number, boolean>} */
+    var o2;
+    /** @type {Object.<boolean, string>} */
+    var o3;
+    /** @param {Object.<string, boolean>} o */
+    function f(o) {
+        o.foo = 1; // error
+        ~~~~~
+!!! error TS2322: Type '1' is not assignable to type 'boolean'.
+        o.bar = false; // ok
+    }
+
diff --git a/tests/baselines/reference/jsdocIndexSignature.symbols b/tests/baselines/reference/jsdocIndexSignature.symbols
@@ -11,3 +11,15 @@ var o2;
 var o3;
 >o3 : Symbol(o3, Decl(indices.js, 5, 3))
 
+/** @param {Object.<string, boolean>} o */
+function f(o) {
+>f : Symbol(f, Decl(indices.js, 5, 7))
+>o : Symbol(o, Decl(indices.js, 7, 11))
+
+    o.foo = 1; // error
+>o : Symbol(o, Decl(indices.js, 7, 11))
+
+    o.bar = false; // ok
+>o : Symbol(o, Decl(indices.js, 7, 11))
+}
+
diff --git a/tests/baselines/reference/jsdocIndexSignature.types b/tests/baselines/reference/jsdocIndexSignature.types
@@ -11,3 +11,23 @@ var o2;
 var o3;
 >o3 : any
 
+/** @param {Object.<string, boolean>} o */
+function f(o) {
+>f : (o: { [x: string]: boolean; }) => void
+>o : { [x: string]: boolean; }
+
+    o.foo = 1; // error
+>o.foo = 1 : 1
+>o.foo : boolean
+>o : { [x: string]: boolean; }
+>foo : boolean
+>1 : 1
+
+    o.bar = false; // ok
+>o.bar = false : false
+>o.bar : boolean
+>o : { [x: string]: boolean; }
+>bar : boolean
+>false : false
+}
+
diff --git a/tests/cases/conformance/jsdoc/jsdocIndexSignature.ts b/tests/cases/conformance/jsdoc/jsdocIndexSignature.ts
@@ -8,3 +8,8 @@ var o1;
 var o2;
 /** @type {Object.<boolean, string>} */
 var o3;
+/** @param {Object.<string, boolean>} o */
+function f(o) {
+    o.foo = 1; // error
+    o.bar = false; // ok
+}
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc10.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc10.ts
@@ -12,4 +12,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
  * @param {?} x
  * @returns {number}
  */
-var f = (x: any): number => x`, 'Annotate with types from JSDoc', 'annotate');
+var f = (x: any): number => x`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc11.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc11.ts
@@ -12,4 +12,4 @@ verify.fileAfterApplyingRefactorAtMarker('2',
  * @param {?} x
  * @returns {number}
  */
-var f = (x: any): number => x`, 'Annotate with types from JSDoc', 'annotate');
+var f = (x: any): number => x`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc12.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc12.ts
@@ -15,4 +15,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
      */
     m(x): any[] {
     }
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc13.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc13.ts
@@ -8,4 +8,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
 `class C {
     /** @return {number} */
     get c(): number { return 12; }
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc14.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc14.ts
@@ -8,4 +8,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
 `/** @return {number} */
 function f(): number {
     return 12;
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc15.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc15.ts
@@ -27,4 +27,4 @@ verify.fileAfterApplyingRefactorAtMarker('9',
  * @param {promise<String>} zeta
  */
 function f(x: boolean, y: string, z: number, alpha: object, beta: Date, gamma: Promise<any>, delta: Array<any>, epsilon: Array<number>, zeta: Promise<string>) {
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc17.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc17.ts
@@ -14,5 +14,5 @@ verify.fileAfterApplyingRefactorAtMarker('1',
      */
     constructor(x: number) {
     }
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
 
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc18.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc18.ts
@@ -8,4 +8,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
 `class C {
     /** @param {number} value */
     set c(value: number) { return 12; }
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc19.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc19.ts
@@ -16,4 +16,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
  * @param {T} b
  */
 function f<T>(a: number, b: T) {
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc20.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc20.ts
@@ -14,4 +14,4 @@ verify.fileAfterApplyingRefactorAtMarker('1',
  * @param {T} b
  */
 function f<T>(a: number, b: T) {
-}`, 'Annotate with types from JSDoc', 'annotate');
+}`, 'Annotate with type from JSDoc', 'annotate');
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc21.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc21.ts
@@ -0,0 +1,73 @@
+/// <reference path='fourslash.ts' />
+// @strict: true
+/////**
+//// * @return {number}
+//// */
+////function /*1*/f(x, y) {
+////}
+////
+/////**
+//// * @return {number}
+//// */
+////function /*2*/g(x, y): number {
+////    return 0;
+////}
+/////**
+//// * @param {number} x
+//// */
+////function /*3*/h(x: number, y): number {
+////    return 0;
+////}
+////
+/////**
+//// * @param {number} x
+//// * @param {string} y
+//// */
+////function /*4*/i(x: number, y: string) {
+////}
+/////**
+//// * @param {number} x
+//// * @return {boolean}
+//// */
+////function /*5*/j(x: number, y): boolean {
+////    return true;
+////}
+
+verify.not.applicableRefactorAvailableAtMarker('2');
+verify.not.applicableRefactorAvailableAtMarker('3');
+verify.not.applicableRefactorAvailableAtMarker('4');
+verify.not.applicableRefactorAvailableAtMarker('5');
+verify.applicableRefactorAvailableAtMarker('1');
+verify.fileAfterApplyingRefactorAtMarker('1',
+`/**
+ * @return {number}
+ */
+function f(x, y): number {
+}
+
+/**
+ * @return {number}
+ */
+function g(x, y): number {
+    return 0;
+}
+/**
+ * @param {number} x
+ */
+function h(x: number, y): number {
+    return 0;
+}
+
+/**
+ * @param {number} x
+ * @param {string} y
+ */
+function i(x: number, y: string) {
+}
+/**
+ * @param {number} x
+ * @return {boolean}
+ */
+function j(x: number, y): boolean {
+    return true;
+}`, 'Annotate with type from JSDoc', 'annotate');