Skip to content

Fix serialisation of static class members in JS #37780

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

Merged
merged 3 commits into from
Apr 4, 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
28 changes: 15 additions & 13 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5969,7 +5969,7 @@ namespace ts {
}

function getNamespaceMembersForSerialization(symbol: Symbol) {
return !symbol.exports ? [] : filter(arrayFrom((symbol.exports).values()), p => !((p.flags & SymbolFlags.Prototype) || (p.escapedName === "prototype")));
return !symbol.exports ? [] : filter(arrayFrom(symbol.exports.values()), isNamespaceMember);
}

function isTypeOnlyNamespace(symbol: Symbol) {
Expand Down Expand Up @@ -6101,7 +6101,7 @@ namespace ts {
}
// Module symbol emit will take care of module-y members, provided it has exports
if (!(symbol.flags & (SymbolFlags.ValueModule | SymbolFlags.NamespaceModule) && !!symbol.exports && !!symbol.exports.size)) {
const props = filter(getPropertiesOfType(type), p => !((p.flags & SymbolFlags.Prototype) || (p.escapedName === "prototype")));
const props = filter(getPropertiesOfType(type), isNamespaceMember);
serializeAsNamespaceDeclaration(props, localName, modifierFlags, /*suppressNewPrivateContext*/ true);
}
}
Expand Down Expand Up @@ -6157,6 +6157,10 @@ namespace ts {
}
}

function isNamespaceMember(p: Symbol) {
return !(p.flags & SymbolFlags.Prototype || p.escapedName === "prototype" || p.valueDeclaration && isClassLike(p.valueDeclaration.parent));
}

function serializeAsClass(symbol: Symbol, localName: string, modifierFlags: ModifierFlags) {
const localParams = getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol);
const typeParamDecls = map(localParams, p => typeParameterToDeclaration(p, context));
Expand Down Expand Up @@ -6193,12 +6197,9 @@ namespace ts {
emptyArray;
const publicProperties = flatMap<Symbol, ClassElement>(publicSymbolProps, p => serializePropertySymbolForClass(p, /*isStatic*/ false, baseTypes[0]));
// Consider static members empty if symbol also has function or module meaning - function namespacey emit will handle statics
const staticMembers = symbol.flags & (SymbolFlags.Function | SymbolFlags.ValueModule)
? []
: flatMap(filter(
getPropertiesOfType(staticType),
p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype"
), p => serializePropertySymbolForClass(p, /*isStatic*/ true, staticBaseType));
const staticMembers = flatMap(
filter(getPropertiesOfType(staticType), p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" && !isNamespaceMember(p)),
p => serializePropertySymbolForClass(p, /*isStatic*/ true, staticBaseType));
const constructors = serializeSignatures(SignatureKind.Construct, staticType, baseTypes[0], SyntaxKind.Constructor) as ConstructorDeclaration[];
Copy link
Member

Choose a reason for hiding this comment

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

Should we extract this predicate to a function, so it's obvious each place is using the same predicate (albeit inverted)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, although it reveals that this usage is not quite the same as the others, because it needs to (1) accept static members (the opposite of the others) but (2) exclude prototype (same as the others)

for (const c of constructors) {
// A constructor's return type and type parameters are supposed to be controlled by the enclosing class declaration
Expand Down Expand Up @@ -6361,7 +6362,7 @@ namespace ts {
includePrivateSymbol(referenced || target);
}

// We disable the context's symbol traker for the duration of this name serialization
// We disable the context's symbol tracker for the duration of this name serialization
// as, by virtue of being here, the name is required to print something, and we don't want to
// issue a visibility error on it. Only anonymous classes that an alias points at _would_ issue
// a visibility error here (as they're not visible within any scope), but we want to hoist them
Expand Down Expand Up @@ -6477,10 +6478,11 @@ namespace ts {
// need to be merged namespace members
return [];
}
if (p.flags & SymbolFlags.Prototype || (baseType && getPropertyOfType(baseType, p.escapedName)
&& isReadonlySymbol(getPropertyOfType(baseType, p.escapedName)!) === isReadonlySymbol(p)
&& (p.flags & SymbolFlags.Optional) === (getPropertyOfType(baseType, p.escapedName)!.flags & SymbolFlags.Optional)
&& isTypeIdenticalTo(getTypeOfSymbol(p), getTypeOfPropertyOfType(baseType, p.escapedName)!))) {
if (p.flags & SymbolFlags.Prototype ||
(baseType && getPropertyOfType(baseType, p.escapedName)
&& isReadonlySymbol(getPropertyOfType(baseType, p.escapedName)!) === isReadonlySymbol(p)
&& (p.flags & SymbolFlags.Optional) === (getPropertyOfType(baseType, p.escapedName)!.flags & SymbolFlags.Optional)
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just a formatting change to make obvious that it's x || (y && z && ka && ki ...)

&& isTypeIdenticalTo(getTypeOfSymbol(p), getTypeOfPropertyOfType(baseType, p.escapedName)!))) {
return [];
}
const flag = (modifierFlags & ~ModifierFlags.Async) | (isStatic ? ModifierFlags.Static : 0);
Expand Down
74 changes: 74 additions & 0 deletions tests/baselines/reference/jsDeclarationsClassStatic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
//// [source.js]
class Handler {
static get OPTIONS() {
return 1;
}

process() {
}
}
Handler.statische = function() { }
const Strings = {
a: "A",
b: "B"
}

module.exports = Handler;
module.exports.Strings = Strings

/**
* @typedef {Object} HandlerOptions
* @property {String} name
* Should be able to export a type alias at the same time.
*/


//// [source.js]
var Handler = /** @class */ (function () {
function Handler() {
}
Object.defineProperty(Handler, "OPTIONS", {
get: function () {
return 1;
},
enumerable: false,
configurable: true
});
Handler.prototype.process = function () {
};
return Handler;
}());
Handler.statische = function () { };
var Strings = {
a: "A",
b: "B"
};
module.exports = Handler;
module.exports.Strings = Strings;
/**
* @typedef {Object} HandlerOptions
* @property {String} name
* Should be able to export a type alias at the same time.
*/


//// [source.d.ts]
export = Handler;
declare class Handler {
static get OPTIONS(): number;
process(): void;
}
declare namespace Handler {
export { statische, Strings, HandlerOptions };
}
declare function statische(): void;
declare namespace Strings {
export const a: string;
export const b: string;
}
type HandlerOptions = {
/**
* Should be able to export a type alias at the same time.
*/
name: String;
};
49 changes: 49 additions & 0 deletions tests/baselines/reference/jsDeclarationsClassStatic.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
=== tests/cases/conformance/jsdoc/declarations/source.js ===
class Handler {
>Handler : Symbol(Handler, Decl(source.js, 0, 0), Decl(source.js, 7, 1))

static get OPTIONS() {
>OPTIONS : Symbol(Handler.OPTIONS, Decl(source.js, 0, 15))

return 1;
}

process() {
>process : Symbol(Handler.process, Decl(source.js, 3, 2))
}
}
Handler.statische = function() { }
>Handler.statische : Symbol(Handler.statische, Decl(source.js, 7, 1))
>Handler : Symbol(Handler, Decl(source.js, 0, 0), Decl(source.js, 7, 1))
>statische : Symbol(Handler.statische, Decl(source.js, 7, 1))

const Strings = {
>Strings : Symbol(Strings, Decl(source.js, 9, 5))

a: "A",
>a : Symbol(a, Decl(source.js, 9, 17))

b: "B"
>b : Symbol(b, Decl(source.js, 10, 11))
}

module.exports = Handler;
>module.exports : Symbol("tests/cases/conformance/jsdoc/declarations/source", Decl(source.js, 0, 0))
>module : Symbol(export=, Decl(source.js, 12, 1))
>exports : Symbol(export=, Decl(source.js, 12, 1))
>Handler : Symbol(Handler, Decl(source.js, 0, 0), Decl(source.js, 7, 1))

module.exports.Strings = Strings
>module.exports.Strings : Symbol(Strings)
>module.exports : Symbol(Strings, Decl(source.js, 14, 25))
>module : Symbol(module, Decl(source.js, 12, 1))
>exports : Symbol("tests/cases/conformance/jsdoc/declarations/source", Decl(source.js, 0, 0))
>Strings : Symbol(Strings, Decl(source.js, 14, 25))
>Strings : Symbol(Strings, Decl(source.js, 9, 5))

/**
* @typedef {Object} HandlerOptions
* @property {String} name
* Should be able to export a type alias at the same time.
*/

57 changes: 57 additions & 0 deletions tests/baselines/reference/jsDeclarationsClassStatic.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
=== tests/cases/conformance/jsdoc/declarations/source.js ===
class Handler {
>Handler : Handler

static get OPTIONS() {
>OPTIONS : number

return 1;
>1 : 1
}

process() {
>process : () => void
}
}
Handler.statische = function() { }
>Handler.statische = function() { } : () => void
>Handler.statische : () => void
>Handler : typeof Handler
>statische : () => void
>function() { } : () => void

const Strings = {
>Strings : { a: string; b: string; }
>{ a: "A", b: "B"} : { a: string; b: string; }

a: "A",
>a : string
>"A" : "A"

b: "B"
>b : string
>"B" : "B"
}

module.exports = Handler;
>module.exports = Handler : typeof Handler
>module.exports : typeof Handler
>module : { "\"tests/cases/conformance/jsdoc/declarations/source\"": typeof Handler; }
>exports : typeof Handler
>Handler : typeof Handler

module.exports.Strings = Strings
>module.exports.Strings = Strings : { a: string; b: string; }
>module.exports.Strings : { a: string; b: string; }
>module.exports : typeof Handler
>module : { "\"tests/cases/conformance/jsdoc/declarations/source\"": typeof Handler; }
>exports : typeof Handler
>Strings : { a: string; b: string; }
>Strings : { a: string; b: string; }

/**
* @typedef {Object} HandlerOptions
* @property {String} name
* Should be able to export a type alias at the same time.
*/

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// @allowJs: true
// @checkJs: true
// @target: es5
// @outDir: ./out
// @declaration: true
// @filename: source.js
class Handler {
static get OPTIONS() {
return 1;
}

process() {
}
}
Handler.statische = function() { }
const Strings = {
a: "A",
b: "B"
}

module.exports = Handler;
module.exports.Strings = Strings

/**
* @typedef {Object} HandlerOptions
* @property {String} name
* Should be able to export a type alias at the same time.
*/