-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c827007
128ef93
a90f97c
04e6b6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5505,6 +5505,10 @@ namespace ts { | |
return symbol.declarations && find(symbol.declarations, s => !!getEffectiveTypeAnnotationNode(s) && (!enclosingDeclaration || !!findAncestor(s, n => n === enclosingDeclaration))); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kinda surprising to me - this checks whether the existing type has more type arguments which I'd assume to be the opposite condition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, if the existing declaration provides the minimum required or more, it's good to reuse; how's that surprising? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's a naming issue? What is the meaning of |
||
function existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(existing: TypeNode, type: Type) { | ||
return !(getObjectFlags(type) & ObjectFlags.Reference) || !isTypeReferenceNode(existing) || length(existing.typeArguments) >= getMinTypeArgumentCount((type as TypeReference).target.typeParameters); | ||
} | ||
|
||
/** | ||
* Unlike `typeToTypeNodeHelper`, this handles setting up the `AllowUniqueESSymbolType` flag | ||
* so a `unique symbol` is returned when appropriate for the input symbol, rather than `typeof sym` | ||
|
@@ -5515,7 +5519,7 @@ namespace ts { | |
if (declWithExistingAnnotation && !isFunctionLikeDeclaration(declWithExistingAnnotation)) { | ||
// try to reuse the existing annotation | ||
const existing = getEffectiveTypeAnnotationNode(declWithExistingAnnotation)!; | ||
if (getTypeFromTypeNode(existing) === type) { | ||
if (getTypeFromTypeNode(existing) === type && existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(existing, type)) { | ||
const result = serializeExistingTypeNode(context, existing, includePrivateSymbol, bundled); | ||
if (result) { | ||
return result; | ||
|
@@ -5536,7 +5540,7 @@ namespace ts { | |
function serializeReturnTypeForSignature(context: NodeBuilderContext, type: Type, signature: Signature, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) { | ||
if (type !== errorType && context.enclosingDeclaration) { | ||
const annotation = signature.declaration && getEffectiveReturnTypeNode(signature.declaration); | ||
if (!!findAncestor(annotation, n => n === context.enclosingDeclaration) && annotation && instantiateType(getTypeFromTypeNode(annotation), signature.mapper) === type) { | ||
if (!!findAncestor(annotation, n => n === context.enclosingDeclaration) && annotation && instantiateType(getTypeFromTypeNode(annotation), signature.mapper) === type && existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(annotation, type)) { | ||
const result = serializeExistingTypeNode(context, annotation, includePrivateSymbol, bundled); | ||
if (result) { | ||
return result; | ||
|
@@ -5577,6 +5581,21 @@ namespace ts { | |
if (isJSDocVariadicType(node)) { | ||
return createArrayTypeNode(visitNode((node as JSDocVariadicType).type, visitExistingNodeTreeSymbols)); | ||
} | ||
if (isJSDocTypeLiteral(node)) { | ||
return createTypeLiteralNode(map(node.jsDocPropertyTags, t => { | ||
const name = isIdentifier(t.name) ? t.name : t.name.right; | ||
const typeViaParent = getTypeOfPropertyOfType(getTypeFromTypeNode(node), name.escapedText); | ||
const overrideTypeNode = typeViaParent && t.typeExpression && getTypeFromTypeNode(t.typeExpression.type) !== typeViaParent ? typeToTypeNodeHelper(typeViaParent, context) : undefined; | ||
|
||
return createPropertySignature( | ||
/*modifiers*/ undefined, | ||
name, | ||
t.typeExpression && isJSDocOptionalType(t.typeExpression.type) ? createToken(SyntaxKind.QuestionToken) : undefined, | ||
overrideTypeNode || (t.typeExpression && visitNode(t.typeExpression.type, visitExistingNodeTreeSymbols)) || createKeywordTypeNode(SyntaxKind.AnyKeyword), | ||
/*initializer*/ undefined | ||
); | ||
})); | ||
} | ||
if (isTypeReferenceNode(node) && isIdentifier(node.typeName) && node.typeName.escapedText === "") { | ||
return setOriginalNode(createKeywordTypeNode(SyntaxKind.AnyKeyword), node); | ||
} | ||
|
@@ -5628,6 +5647,9 @@ namespace ts { | |
); | ||
} | ||
} | ||
if (isTypeReferenceNode(node) && isInJSDoc(node) && (getIntendedTypeFromJSDocTypeReference(node) || unknownSymbol === resolveTypeReferenceName(getTypeReferenceName(node), SymbolFlags.Type, /*ignoreErrors*/ true))) { | ||
return setOriginalNode(typeToTypeNodeHelper(getTypeFromTypeNode(node), context), node); | ||
} | ||
if (isLiteralImportTypeNode(node)) { | ||
return updateImportTypeNode( | ||
node, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
//// [index.js] | ||
// these are recognized as TS concepts by the checker | ||
/** @type {String} */const a = ""; | ||
/** @type {Number} */const b = 0; | ||
/** @type {Boolean} */const c = true; | ||
/** @type {Void} */const d = undefined; | ||
/** @type {Undefined} */const e = undefined; | ||
/** @type {Null} */const f = null; | ||
|
||
/** @type {Function} */const g = () => void 0; | ||
/** @type {function} */const h = () => void 0; | ||
/** @type {array} */const i = []; | ||
/** @type {promise} */const j = Promise.resolve(0); | ||
/** @type {Object<string, string>} */const k = {x: "x"}; | ||
|
||
|
||
// these are not recognized as anything and should just be lookup failures | ||
// ignore the errors to try to ensure they're emitted as `any` in declaration emit | ||
// @ts-ignore | ||
/** @type {class} */const l = true; | ||
// @ts-ignore | ||
/** @type {bool} */const m = true; | ||
// @ts-ignore | ||
/** @type {int} */const n = true; | ||
// @ts-ignore | ||
/** @type {float} */const o = true; | ||
// @ts-ignore | ||
/** @type {integer} */const p = true; | ||
|
||
// or, in the case of `event` likely erroneously refers to the type of the global Event object | ||
/** @type {event} */const q = undefined; | ||
|
||
//// [index.js] | ||
"use strict"; | ||
// these are recognized as TS concepts by the checker | ||
/** @type {String} */ const a = ""; | ||
/** @type {Number} */ const b = 0; | ||
/** @type {Boolean} */ const c = true; | ||
/** @type {Void} */ const d = undefined; | ||
/** @type {Undefined} */ const e = undefined; | ||
/** @type {Null} */ const f = null; | ||
/** @type {Function} */ const g = () => void 0; | ||
/** @type {function} */ const h = () => void 0; | ||
/** @type {array} */ const i = []; | ||
/** @type {promise} */ const j = Promise.resolve(0); | ||
/** @type {Object<string, string>} */ const k = { x: "x" }; | ||
// these are not recognized as anything and should just be lookup failures | ||
// ignore the errors to try to ensure they're emitted as `any` in declaration emit | ||
// @ts-ignore | ||
/** @type {class} */ const l = true; | ||
// @ts-ignore | ||
/** @type {bool} */ const m = true; | ||
// @ts-ignore | ||
/** @type {int} */ const n = true; | ||
// @ts-ignore | ||
/** @type {float} */ const o = true; | ||
// @ts-ignore | ||
/** @type {integer} */ const p = true; | ||
// or, in the case of `event` likely erroneously refers to the type of the global Event object | ||
/** @type {event} */ const q = undefined; | ||
|
||
|
||
//// [index.d.ts] | ||
/** @type {String} */ declare const a: string; | ||
/** @type {Number} */ declare const b: number; | ||
/** @type {Boolean} */ declare const c: boolean; | ||
/** @type {Void} */ declare const d: void; | ||
/** @type {Undefined} */ declare const e: undefined; | ||
/** @type {Null} */ declare const f: null; | ||
/** @type {Function} */ declare const g: Function; | ||
/** @type {function} */ declare const h: Function; | ||
/** @type {array} */ declare const i: any[]; | ||
/** @type {promise} */ declare const j: Promise<any>; | ||
/** @type {Object<string, string>} */ declare const k: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's actually not a thing we interpret -
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, but the handbook says
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the point is to test out stuff might people write in JSDoc, because people often write terrible stuff in JSDoc because it was terribly specified. |
||
[x: string]: string; | ||
}; | ||
/** @type {class} */ declare const l: any; | ||
/** @type {bool} */ declare const m: any; | ||
/** @type {int} */ declare const n: any; | ||
/** @type {float} */ declare const o: any; | ||
/** @type {integer} */ declare const p: any; | ||
/** @type {event} */ declare const q: Event | undefined; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
=== tests/cases/conformance/jsdoc/declarations/index.js === | ||
// these are recognized as TS concepts by the checker | ||
/** @type {String} */const a = ""; | ||
>a : Symbol(a, Decl(index.js, 1, 26)) | ||
|
||
/** @type {Number} */const b = 0; | ||
>b : Symbol(b, Decl(index.js, 2, 26)) | ||
|
||
/** @type {Boolean} */const c = true; | ||
>c : Symbol(c, Decl(index.js, 3, 27)) | ||
|
||
/** @type {Void} */const d = undefined; | ||
>d : Symbol(d, Decl(index.js, 4, 24)) | ||
>undefined : Symbol(undefined) | ||
|
||
/** @type {Undefined} */const e = undefined; | ||
>e : Symbol(e, Decl(index.js, 5, 29)) | ||
>undefined : Symbol(undefined) | ||
|
||
/** @type {Null} */const f = null; | ||
>f : Symbol(f, Decl(index.js, 6, 24)) | ||
|
||
/** @type {Function} */const g = () => void 0; | ||
>g : Symbol(g, Decl(index.js, 8, 28)) | ||
|
||
/** @type {function} */const h = () => void 0; | ||
>h : Symbol(h, Decl(index.js, 9, 28)) | ||
|
||
/** @type {array} */const i = []; | ||
>i : Symbol(i, Decl(index.js, 10, 25)) | ||
|
||
/** @type {promise} */const j = Promise.resolve(0); | ||
>j : Symbol(j, Decl(index.js, 11, 27)) | ||
>Promise.resolve : Symbol(PromiseConstructor.resolve, Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --)) | ||
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --)) | ||
>resolve : Symbol(PromiseConstructor.resolve, Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --)) | ||
|
||
/** @type {Object<string, string>} */const k = {x: "x"}; | ||
>k : Symbol(k, Decl(index.js, 12, 42)) | ||
>x : Symbol(x, Decl(index.js, 12, 48)) | ||
|
||
|
||
// these are not recognized as anything and should just be lookup failures | ||
// ignore the errors to try to ensure they're emitted as `any` in declaration emit | ||
// @ts-ignore | ||
/** @type {class} */const l = true; | ||
>l : Symbol(l, Decl(index.js, 18, 25)) | ||
|
||
// @ts-ignore | ||
/** @type {bool} */const m = true; | ||
>m : Symbol(m, Decl(index.js, 20, 24)) | ||
|
||
// @ts-ignore | ||
/** @type {int} */const n = true; | ||
>n : Symbol(n, Decl(index.js, 22, 23)) | ||
|
||
// @ts-ignore | ||
/** @type {float} */const o = true; | ||
>o : Symbol(o, Decl(index.js, 24, 25)) | ||
|
||
// @ts-ignore | ||
/** @type {integer} */const p = true; | ||
>p : Symbol(p, Decl(index.js, 26, 27)) | ||
|
||
// or, in the case of `event` likely erroneously refers to the type of the global Event object | ||
/** @type {event} */const q = undefined; | ||
>q : Symbol(q, Decl(index.js, 29, 25)) | ||
>undefined : Symbol(undefined) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
=== tests/cases/conformance/jsdoc/declarations/index.js === | ||
// these are recognized as TS concepts by the checker | ||
/** @type {String} */const a = ""; | ||
>a : string | ||
>"" : "" | ||
|
||
/** @type {Number} */const b = 0; | ||
>b : number | ||
>0 : 0 | ||
|
||
/** @type {Boolean} */const c = true; | ||
>c : boolean | ||
>true : true | ||
|
||
/** @type {Void} */const d = undefined; | ||
>d : void | ||
>undefined : undefined | ||
|
||
/** @type {Undefined} */const e = undefined; | ||
>e : undefined | ||
>undefined : undefined | ||
|
||
/** @type {Null} */const f = null; | ||
>f : null | ||
>null : null | ||
|
||
/** @type {Function} */const g = () => void 0; | ||
>g : Function | ||
>() => void 0 : () => undefined | ||
>void 0 : undefined | ||
>0 : 0 | ||
|
||
/** @type {function} */const h = () => void 0; | ||
>h : Function | ||
>() => void 0 : () => undefined | ||
>void 0 : undefined | ||
>0 : 0 | ||
|
||
/** @type {array} */const i = []; | ||
>i : any[] | ||
>[] : never[] | ||
|
||
/** @type {promise} */const j = Promise.resolve(0); | ||
>j : Promise<any> | ||
>Promise.resolve(0) : Promise<number> | ||
>Promise.resolve : { <T>(value: T | PromiseLike<T>): Promise<T>; (): Promise<void>; } | ||
>Promise : PromiseConstructor | ||
>resolve : { <T>(value: T | PromiseLike<T>): Promise<T>; (): Promise<void>; } | ||
>0 : 0 | ||
|
||
/** @type {Object<string, string>} */const k = {x: "x"}; | ||
>k : { [x: string]: string; } | ||
>{x: "x"} : { x: string; } | ||
>x : string | ||
>"x" : "x" | ||
|
||
|
||
// these are not recognized as anything and should just be lookup failures | ||
// ignore the errors to try to ensure they're emitted as `any` in declaration emit | ||
// @ts-ignore | ||
/** @type {class} */const l = true; | ||
>l : error | ||
>true : true | ||
|
||
// @ts-ignore | ||
/** @type {bool} */const m = true; | ||
>m : error | ||
>true : true | ||
|
||
// @ts-ignore | ||
/** @type {int} */const n = true; | ||
>n : error | ||
>true : true | ||
|
||
// @ts-ignore | ||
/** @type {float} */const o = true; | ||
>o : error | ||
>true : true | ||
|
||
// @ts-ignore | ||
/** @type {integer} */const p = true; | ||
>p : error | ||
>true : true | ||
|
||
// or, in the case of `event` likely erroneously refers to the type of the global Event object | ||
/** @type {event} */const q = undefined; | ||
>q : Event | undefined | ||
>undefined : undefined | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
//// [file.js] | ||
/** | ||
* @param {Array} x | ||
*/ | ||
function x(x) {} | ||
/** | ||
* @param {Promise} x | ||
*/ | ||
function y(x) {} | ||
|
||
//// [file.js] | ||
/** | ||
* @param {Array} x | ||
*/ | ||
function x(x) { } | ||
/** | ||
* @param {Promise} x | ||
*/ | ||
function y(x) { } | ||
|
||
|
||
//// [file.d.ts] | ||
/** | ||
* @param {Array} x | ||
*/ | ||
declare function x(x: any[]): void; | ||
/** | ||
* @param {Promise} x | ||
*/ | ||
declare function y(x: Promise<any>): void; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
=== tests/cases/conformance/jsdoc/declarations/file.js === | ||
/** | ||
* @param {Array} x | ||
*/ | ||
function x(x) {} | ||
>x : Symbol(x, Decl(file.js, 0, 0)) | ||
>x : Symbol(x, Decl(file.js, 3, 11)) | ||
|
||
/** | ||
* @param {Promise} x | ||
*/ | ||
function y(x) {} | ||
>y : Symbol(y, Decl(file.js, 3, 16)) | ||
>x : Symbol(x, Decl(file.js, 7, 11)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
=== tests/cases/conformance/jsdoc/declarations/file.js === | ||
/** | ||
* @param {Array} x | ||
*/ | ||
function x(x) {} | ||
>x : (x: any[]) => void | ||
>x : any[] | ||
|
||
/** | ||
* @param {Promise} x | ||
*/ | ||
function y(x) {} | ||
>y : (x: Promise<any>) => void | ||
>x : Promise<any> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this start with the word
get
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention, yes - pretty much all of our complex internal functions start with verbs,
get
,create
,serialize
, and so on.It makes completions easier to trigger, tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about predicate functions like
typeHasCallOrConstructSignatures
? Doesn't seem appropriate to start with aget
here IMOThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but emacs doesn't show completions until I've typed 3 characters!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to
existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount
which surely isn't a mouthful. I hope the removal of the verb is to your liking?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named perfectly!