From f3a6df1e252ddd6cfc4e531cba4870e7ec2298d5 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 3 Apr 2020 13:42:00 -0700 Subject: [PATCH 1/3] Fix serialisation of static class members in JS Previously static class members would be treated the same way as expando namespace assignments to a class: ```ts class C { static get x() { return 1 } } C.y = 12 ``` This PR adds a syntactic check to the static/namespace filter that treats symbols whose valueDeclaration.parent is a class as statics. Fixes #37289 --- src/compiler/checker.ts | 26 ++++--- .../reference/jsDeclarationsClassStatic.js | 74 +++++++++++++++++++ .../jsDeclarationsClassStatic.symbols | 49 ++++++++++++ .../reference/jsDeclarationsClassStatic.types | 57 ++++++++++++++ .../declarations/jsDeclarationsClassStatic.ts | 28 +++++++ 5 files changed, 223 insertions(+), 11 deletions(-) create mode 100644 tests/baselines/reference/jsDeclarationsClassStatic.js create mode 100644 tests/baselines/reference/jsDeclarationsClassStatic.symbols create mode 100644 tests/baselines/reference/jsDeclarationsClassStatic.types create mode 100644 tests/cases/conformance/jsdoc/declarations/jsDeclarationsClassStatic.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 9d083185ce106..06ce070d7b073 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -5969,7 +5969,10 @@ 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()), + p => !(p.flags & SymbolFlags.Prototype || p.escapedName === "prototype" || p.valueDeclaration && isClassLike(p.valueDeclaration.parent))); } function isTypeOnlyNamespace(symbol: Symbol) { @@ -6101,7 +6104,9 @@ 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), + p => !(p.flags & SymbolFlags.Prototype || p.escapedName === "prototype" || p.valueDeclaration && isClassLike(p.valueDeclaration.parent))); serializeAsNamespaceDeclaration(props, localName, modifierFlags, /*suppressNewPrivateContext*/ true); } } @@ -6193,11 +6198,9 @@ namespace ts { emptyArray; const publicProperties = flatMap(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( + const staticMembers = flatMap(filter( getPropertiesOfType(staticType), - p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" + p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" && p.valueDeclaration && isClassLike(p.valueDeclaration.parent) ), p => serializePropertySymbolForClass(p, /*isStatic*/ true, staticBaseType)); const constructors = serializeSignatures(SignatureKind.Construct, staticType, baseTypes[0], SyntaxKind.Constructor) as ConstructorDeclaration[]; for (const c of constructors) { @@ -6361,7 +6364,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 @@ -6477,10 +6480,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) + && isTypeIdenticalTo(getTypeOfSymbol(p), getTypeOfPropertyOfType(baseType, p.escapedName)!))) { return []; } const flag = (modifierFlags & ~ModifierFlags.Async) | (isStatic ? ModifierFlags.Static : 0); diff --git a/tests/baselines/reference/jsDeclarationsClassStatic.js b/tests/baselines/reference/jsDeclarationsClassStatic.js new file mode 100644 index 0000000000000..1498b8bd6bd5f --- /dev/null +++ b/tests/baselines/reference/jsDeclarationsClassStatic.js @@ -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; +}; diff --git a/tests/baselines/reference/jsDeclarationsClassStatic.symbols b/tests/baselines/reference/jsDeclarationsClassStatic.symbols new file mode 100644 index 0000000000000..d79d144e5096a --- /dev/null +++ b/tests/baselines/reference/jsDeclarationsClassStatic.symbols @@ -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. + */ + diff --git a/tests/baselines/reference/jsDeclarationsClassStatic.types b/tests/baselines/reference/jsDeclarationsClassStatic.types new file mode 100644 index 0000000000000..50783314fac53 --- /dev/null +++ b/tests/baselines/reference/jsDeclarationsClassStatic.types @@ -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. + */ + diff --git a/tests/cases/conformance/jsdoc/declarations/jsDeclarationsClassStatic.ts b/tests/cases/conformance/jsdoc/declarations/jsDeclarationsClassStatic.ts new file mode 100644 index 0000000000000..06b2af747efd1 --- /dev/null +++ b/tests/cases/conformance/jsdoc/declarations/jsDeclarationsClassStatic.ts @@ -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. + */ From 51f1311bc12c31bce304896e61216b652d4c28b9 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 3 Apr 2020 13:44:59 -0700 Subject: [PATCH 2/3] fix messed-up indent --- src/compiler/checker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 06ce070d7b073..a39ccd0819fcc 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -6200,7 +6200,7 @@ namespace ts { // Consider static members empty if symbol also has function or module meaning - function namespacey emit will handle statics const staticMembers = flatMap(filter( getPropertiesOfType(staticType), - p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" && p.valueDeclaration && isClassLike(p.valueDeclaration.parent) + p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" && p.valueDeclaration && isClassLike(p.valueDeclaration.parent) ), p => serializePropertySymbolForClass(p, /*isStatic*/ true, staticBaseType)); const constructors = serializeSignatures(SignatureKind.Construct, staticType, baseTypes[0], SyntaxKind.Constructor) as ConstructorDeclaration[]; for (const c of constructors) { From bb23c15380975e184005ca01b31c858545789ad0 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 3 Apr 2020 17:11:39 -0700 Subject: [PATCH 3/3] Extract function --- src/compiler/checker.ts | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index a39ccd0819fcc..e82a7be9c0c79 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -5969,10 +5969,7 @@ namespace ts { } function getNamespaceMembersForSerialization(symbol: Symbol) { - return !symbol.exports ? [] : - filter( - arrayFrom(symbol.exports.values()), - p => !(p.flags & SymbolFlags.Prototype || p.escapedName === "prototype" || p.valueDeclaration && isClassLike(p.valueDeclaration.parent))); + return !symbol.exports ? [] : filter(arrayFrom(symbol.exports.values()), isNamespaceMember); } function isTypeOnlyNamespace(symbol: Symbol) { @@ -6104,9 +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" || p.valueDeclaration && isClassLike(p.valueDeclaration.parent))); + const props = filter(getPropertiesOfType(type), isNamespaceMember); serializeAsNamespaceDeclaration(props, localName, modifierFlags, /*suppressNewPrivateContext*/ true); } } @@ -6162,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)); @@ -6198,10 +6197,9 @@ namespace ts { emptyArray; const publicProperties = flatMap(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 = flatMap(filter( - getPropertiesOfType(staticType), - p => !(p.flags & SymbolFlags.Prototype) && p.escapedName !== "prototype" && p.valueDeclaration && isClassLike(p.valueDeclaration.parent) - ), 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[]; for (const c of constructors) { // A constructor's return type and type parameters are supposed to be controlled by the enclosing class declaration