Skip to content
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

Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit #38368

Merged
merged 4 commits into from
May 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5505,6 +5505,10 @@ namespace ts {
return symbol.declarations && find(symbol.declarations, s => !!getEffectiveTypeAnnotationNode(s) && (!enclosingDeclaration || !!findAncestor(s, n => n === enclosingDeclaration)));
}
Copy link
Member

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?

Copy link
Member Author

@weswigham weswigham May 7, 2020

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.

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 7, 2020

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 a get here IMO

Copy link
Member

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!

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named perfectly!


Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a naming issue? What is the meaning of hasNoTypeParameters in this case?

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`
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/exportNestedNamespaces.types
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var classic = new s.Classic()
/** @param {s.n.K} c
@param {s.Classic} classic */
function f(c, classic) {
>f : (c: s.n.K, classic: s.Classic) => void
>f : (c: K, classic: s.Classic) => void
>c : K
>classic : Classic

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/jsDeclarationsClassStatic.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,5 @@ type HandlerOptions = {
/**
* Should be able to export a type alias at the same time.
*/
name: String;
name: string;
};
82 changes: 82 additions & 0 deletions tests/baselines/reference/jsDeclarationsJSDocRedirectedLookups.js
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: {
Copy link
Contributor

@Jack-Works Jack-Works May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about add a Object => object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually not a thing we interpret - Object with type arguments is interpreted as an index signature, without isn't recognized special (so it looks up the global Object constructor type, which is the apparent type of... pretty much everything except null and undefined) except when noImplicitAny is false, where it is any.

getIntendedTypeFromJSDocTypeReference in the checker has the special jsdoc types that we lookup in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but the handbook says

Don’t ever use the types Number, String, Boolean, Symbol, or Object These types refer to non-primitive boxed objects that are almost never used appropriately in JavaScript code.

Instead of Object, use the non-primitive object type (added in TypeScript 2.2).

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 8, 2020

Choose a reason for hiding this comment

The 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

30 changes: 30 additions & 0 deletions tests/baselines/reference/jsDeclarationsMissingGenerics.js
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;
15 changes: 15 additions & 0 deletions tests/baselines/reference/jsDeclarationsMissingGenerics.symbols
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))

15 changes: 15 additions & 0 deletions tests/baselines/reference/jsDeclarationsMissingGenerics.types
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>

Loading