Skip to content

Fix resolution of properties from prototype assignment in JS #29302

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

64 changes: 21 additions & 43 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8687,21 +8687,17 @@ namespace ts {
* the type of this reference is just the type of the value we resolved to.
*/
function getJSDocTypeReference(node: NodeWithTypeArguments, symbol: Symbol, typeArguments: Type[] | undefined): Type | undefined {
if (!pushTypeResolution(symbol, TypeSystemPropertyName.JSDocTypeReference)) {
return errorType;
}
const assignedType = getAssignedClassType(symbol);
const valueType = getTypeOfSymbol(symbol);
const referenceType = valueType.symbol && valueType.symbol !== symbol && !isInferredClassType(valueType) && getTypeReferenceTypeWorker(node, valueType.symbol, typeArguments);
if (!popTypeResolution()) {
getSymbolLinks(symbol).resolvedJSDocType = errorType;
error(node, Diagnostics.JSDoc_type_0_circularly_references_itself, symbolToString(symbol));
return errorType;
}
if (referenceType || assignedType) {
// TODO: GH#18217 (should the `|| assignedType` be at a lower precedence?)
const type = (referenceType && assignedType ? getIntersectionType([assignedType, referenceType]) : referenceType || assignedType)!;
return getSymbolLinks(symbol).resolvedJSDocType = type;
// In the case of an assignment of a function expression (binary expressions, variable declarations, etc.), we will get the
// correct instance type for the symbol on the LHS by finding the type for RHS. For example if we want to get the type of the symbol `foo`:
// var foo = function() {}
// We will find the static type of the assigned anonymous function.
const staticType = getTypeOfSymbol(symbol);
const instanceType =
staticType.symbol &&
staticType.symbol !== symbol && // Make sure this is an assignment like expression by checking that symbol -> type -> symbol doesn't roundtrips.
getTypeReferenceTypeWorker(node, staticType.symbol, typeArguments); // Get the instance type of the RHS symbol.
if (instanceType) {
return getSymbolLinks(symbol).resolvedJSDocType = instanceType;
}
}

Expand All @@ -8722,8 +8718,11 @@ namespace ts {

if (symbol.flags & SymbolFlags.Function &&
isJSDocTypeReference(node) &&
(symbol.members || getJSDocClassTag(symbol.valueDeclaration))) {
return getInferredClassType(symbol);
isJSConstructor(symbol.valueDeclaration)) {
const resolved = resolveStructuredTypeMembers(<ObjectType>getTypeOfSymbol(symbol));
if (resolved.callSignatures.length === 1) {
return getReturnTypeOfSignature(resolved.callSignatures[0]);
}
}
}

Expand Down Expand Up @@ -16735,7 +16734,7 @@ namespace ts {
else if (isInJS &&
(container.kind === SyntaxKind.FunctionExpression || container.kind === SyntaxKind.FunctionDeclaration) &&
getJSDocClassTag(container)) {
const classType = getJSClassType(container.symbol);
const classType = getJSClassType(getMergedSymbol(container.symbol));
if (classType) {
return getFlowTypeOfReference(node, classType);
}
Expand Down Expand Up @@ -20893,7 +20892,7 @@ namespace ts {

// If the symbol of the node has members, treat it like a constructor.
const symbol = getSymbolOfNode(func);
return !!symbol && symbol.members !== undefined;
return !!symbol && (symbol.members !== undefined || symbol.exports !== undefined && symbol.exports.get("prototype" as __String) !== undefined);
}
return false;
}
Expand All @@ -20912,10 +20911,6 @@ namespace ts {
inferred = getInferredClassType(symbol);
}
const assigned = getAssignedClassType(symbol);
const valueType = getTypeOfSymbol(symbol);
if (valueType.symbol && !isInferredClassType(valueType) && isJSConstructor(valueType.symbol.valueDeclaration)) {
inferred = getInferredClassType(valueType.symbol);
}
return assigned && inferred ?
getIntersectionType([inferred, assigned]) :
assigned || inferred;
Expand Down Expand Up @@ -20955,12 +20950,6 @@ namespace ts {
return links.inferredClassType;
}

function isInferredClassType(type: Type) {
return type.symbol
&& getObjectFlags(type) & ObjectFlags.Anonymous
&& getSymbolLinks(type.symbol).inferredClassType === type;
}

/**
* Syntactically and semantically checks a call or new expression.
* @param node The call/new expression to be checked.
Expand All @@ -20982,21 +20971,10 @@ namespace ts {
declaration.kind !== SyntaxKind.Constructor &&
declaration.kind !== SyntaxKind.ConstructSignature &&
declaration.kind !== SyntaxKind.ConstructorType &&
!isJSDocConstructSignature(declaration)) {
!isJSDocConstructSignature(declaration) &&
!isJSConstructor(declaration)) {

// When resolved signature is a call signature (and not a construct signature) the result type is any, unless
// the declaring function had members created through 'x.prototype.y = expr' or 'this.y = expr' psuedodeclarations
// in a JS file
// Note:JS inferred classes might come from a variable declaration instead of a function declaration.
// In this case, using getResolvedSymbol directly is required to avoid losing the members from the declaration.
let funcSymbol = checkExpression(node.expression).symbol;
if (!funcSymbol && node.expression.kind === SyntaxKind.Identifier) {
funcSymbol = getResolvedSymbol(node.expression as Identifier);
}
const type = funcSymbol && getJSClassType(funcSymbol);
if (type) {
return signature.target ? instantiateType(type, signature.mapper) : type;
}
// When resolved signature is a call signature (and not a construct signature) the result type is any
if (noImplicitAny) {
error(node, Diagnostics.new_expression_whose_target_lacks_a_construct_signature_implicitly_has_an_any_type);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ soup.flavour
>flavour : number

var chowder = new Chowder({ claim: "ignorant" });
>chowder : any
>new Chowder({ claim: "ignorant" }) : any
>chowder : Chowder
>new Chowder({ claim: "ignorant" }) : Chowder
>Chowder : typeof Chowder
>{ claim: "ignorant" } : { claim: "ignorant"; }
>claim : "ignorant"
Expand All @@ -279,7 +279,7 @@ var chowder = new Chowder({ claim: "ignorant" });
chowder.flavour.claim
>chowder.flavour.claim : any
>chowder.flavour : any
>chowder : any
>chowder : Chowder
>flavour : any
>claim : any

Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/jsContainerMergeJsContainer.types
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ const a = {};
>{} : {}

a.d = function() {};
>a.d = function() {} : { (): void; prototype: {}; }
>a.d : { (): void; prototype: {}; }
>a.d = function() {} : typeof d
>a.d : typeof d
>a : typeof a
>d : { (): void; prototype: {}; }
>function() {} : { (): void; prototype: {}; }
>d : typeof d
>function() {} : typeof d

=== tests/cases/conformance/salsa/b.js ===
a.d.prototype = {};
Copy link
Member

Choose a reason for hiding this comment

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

interesting...the change in type display indicates that we're now treating a.d as a class. I'm pretty sure that's because of the prototype assignment, where previously we only checked for members on the symbol.

(we print the type of 'a' as 'typeof a' because the expando assignments turn it into a namespace.)

>a.d.prototype = {} : {}
>a.d.prototype : {}
>a.d : { (): void; prototype: {}; }
>a.d : typeof d
>a : typeof a
>d : { (): void; prototype: {}; }
>d : typeof d
>prototype : {}
>{} : {}

16 changes: 8 additions & 8 deletions tests/baselines/reference/typeFromPropertyAssignment14.types
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@ var Outer = {};

=== tests/cases/conformance/salsa/work.js ===
Outer.Inner = function () {}
>Outer.Inner = function () {} : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner = function () {} : typeof Inner
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>function () {} : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner
>function () {} : typeof Inner

Outer.Inner.prototype = {
>Outer.Inner.prototype = { x: 1, m() { }} : { x: number; m(): void; }
>Outer.Inner.prototype : { x: number; m(): void; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner
>prototype : { x: number; m(): void; }
>{ x: 1, m() { }} : { x: number; m(): void; }

Expand Down Expand Up @@ -47,9 +47,9 @@ inner.m()
var inno = new Outer.Inner()
>inno : { x: number; m(): void; }
>new Outer.Inner() : { x: number; m(): void; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner

inno.x
>inno.x : number
Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/typeFromPropertyAssignment16.types
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@ var Outer = {};
>{} : {}

Outer.Inner = function () {}
>Outer.Inner = function () {} : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner = function () {} : typeof Inner
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>function () {} : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner
>function () {} : typeof Inner

Outer.Inner.prototype = {
>Outer.Inner.prototype = { x: 1, m() { }} : { x: number; m(): void; }
>Outer.Inner.prototype : { x: number; m(): void; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner
>prototype : { x: number; m(): void; }
>{ x: 1, m() { }} : { x: number; m(): void; }

Expand Down Expand Up @@ -45,9 +45,9 @@ inner.m()
var inno = new Outer.Inner()
>inno : { x: number; m(): void; }
>new Outer.Inner() : { x: number; m(): void; }
>Outer.Inner : { (): void; prototype: { x: number; m(): void; }; }
>Outer.Inner : typeof Inner
>Outer : typeof Outer
>Inner : { (): void; prototype: { x: number; m(): void; }; }
>Inner : typeof Inner

inno.x
>inno.x : number
Expand Down
26 changes: 26 additions & 0 deletions tests/baselines/reference/typeFromPrototypeAssignment3.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
tests/cases/conformance/salsa/bug26885.js(2,5): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
tests/cases/conformance/salsa/bug26885.js(11,16): error TS7017: Element implicitly has an 'any' type because type '{}' has no index signature.


==== tests/cases/conformance/salsa/bug26885.js (2 errors) ====
function Multimap3() {
this._map = {};
~~~~
!!! error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
};

Multimap3.prototype = {
/**
* @param {string} key
* @returns {number} the value ok
*/
get(key) {
return this._map[key + ''];
~~~~~~~~~~~~~~~~~~~
!!! error TS7017: Element implicitly has an 'any' type because type '{}' has no index signature.
}
}

/** @type {Multimap3} */
const map = new Multimap3();
const n = map.get('hi')
40 changes: 40 additions & 0 deletions tests/baselines/reference/typeFromPrototypeAssignment3.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
=== tests/cases/conformance/salsa/bug26885.js ===
function Multimap3() {
>Multimap3 : Symbol(Multimap3, Decl(bug26885.js, 0, 0), Decl(bug26885.js, 2, 2))

this._map = {};
>_map : Symbol(Multimap3._map, Decl(bug26885.js, 0, 22))

};

Multimap3.prototype = {
>Multimap3.prototype : Symbol(Multimap3.prototype, Decl(bug26885.js, 2, 2))
>Multimap3 : Symbol(Multimap3, Decl(bug26885.js, 0, 0), Decl(bug26885.js, 2, 2))
>prototype : Symbol(Multimap3.prototype, Decl(bug26885.js, 2, 2))

/**
* @param {string} key
* @returns {number} the value ok
*/
get(key) {
>get : Symbol(get, Decl(bug26885.js, 4, 23))
>key : Symbol(key, Decl(bug26885.js, 9, 8))

return this._map[key + ''];
>this._map : Symbol(Multimap3._map, Decl(bug26885.js, 0, 22))
>_map : Symbol(Multimap3._map, Decl(bug26885.js, 0, 22))
>key : Symbol(key, Decl(bug26885.js, 9, 8))
}
}

/** @type {Multimap3} */
const map = new Multimap3();
>map : Symbol(map, Decl(bug26885.js, 15, 5))
>Multimap3 : Symbol(Multimap3, Decl(bug26885.js, 0, 0), Decl(bug26885.js, 2, 2))

const n = map.get('hi')
>n : Symbol(n, Decl(bug26885.js, 16, 5))
>map.get : Symbol(get, Decl(bug26885.js, 4, 23))
>map : Symbol(map, Decl(bug26885.js, 15, 5))
>get : Symbol(get, Decl(bug26885.js, 4, 23))

53 changes: 53 additions & 0 deletions tests/baselines/reference/typeFromPrototypeAssignment3.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
=== tests/cases/conformance/salsa/bug26885.js ===
function Multimap3() {
>Multimap3 : typeof Multimap3

this._map = {};
>this._map = {} : {}
>this._map : any
>this : any
>_map : any
>{} : {}

};

Multimap3.prototype = {
>Multimap3.prototype = { /** * @param {string} key * @returns {number} the value ok */ get(key) { return this._map[key + '']; }} : { get(key: string): number; }
>Multimap3.prototype : { get(key: string): number; }
>Multimap3 : typeof Multimap3
>prototype : { get(key: string): number; }
>{ /** * @param {string} key * @returns {number} the value ok */ get(key) { return this._map[key + '']; }} : { get(key: string): number; }

/**
* @param {string} key
* @returns {number} the value ok
*/
get(key) {
>get : (key: string) => number
>key : string

return this._map[key + ''];
>this._map[key + ''] : any
>this._map : {}
>this : Multimap3 & { get(key: string): number; }
>_map : {}
>key + '' : string
>key : string
>'' : ""
}
}

/** @type {Multimap3} */
const map = new Multimap3();
>map : Multimap3 & { get(key: string): number; }
>new Multimap3() : Multimap3 & { get(key: string): number; }
>Multimap3 : typeof Multimap3

const n = map.get('hi')
>n : number
>map.get('hi') : number
>map.get : (key: string) => number
>map : Multimap3 & { get(key: string): number; }
>get : (key: string) => number
>'hi' : "hi"

Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
tests/cases/conformance/jsdoc/bug27346.js(2,4): error TS8030: The type of a function declaration must match the function's signature.
tests/cases/conformance/jsdoc/bug27346.js(2,11): error TS2587: JSDoc type 'MyClass' circularly references itself.


==== tests/cases/conformance/jsdoc/bug27346.js (2 errors) ====
==== tests/cases/conformance/jsdoc/bug27346.js (1 errors) ====
/**
* @type {MyClass}
~~~~~~~~~~~~~~~
!!! error TS8030: The type of a function declaration must match the function's signature.
Copy link
Member

Choose a reason for hiding this comment

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

I guess the second error goes away because getJSClassType doesn't have const valueType = getTypeOfSymbol() any more? Not entirely sure.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it might be worthwhile to check whether we have any other error baselines that include the "JSDoc type circularly references itself".

Copy link
Member Author

Choose a reason for hiding this comment

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

No other baselines had this error, and the original repro from the #27346 doesn't give this error anymore. I removed the circularity guard and no baselines changed, so I'll take that as part of this change too.

~~~~~~~
!!! error TS2587: JSDoc type 'MyClass' circularly references itself.
*/
function MyClass() { }
MyClass.prototype = {};
Expand Down
23 changes: 23 additions & 0 deletions tests/cases/conformance/salsa/typeFromPrototypeAssignment3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @noEmit: true
// @allowJs: true
// @checkJs: true
// @Filename: bug26885.js
// @strict: true

function Multimap3() {
this._map = {};
};

Multimap3.prototype = {
/**
* @param {string} key
* @returns {number} the value ok
*/
get(key) {
return this._map[key + ''];
}
}

/** @type {Multimap3} */
const map = new Multimap3();
const n = map.get('hi')