From a0741cc0264e8c79615017b8fc17b39a2af8f1a9 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 4 Apr 2019 16:41:42 +0200 Subject: [PATCH] fix(jsii): check that static and nonstatic members don't share a name (#430) This is not allowed in Java, and leads to compiler warnings in C#. Fixes #427. --- packages/jsii/doc/STATICS.md | 40 ++++++++ packages/jsii/lib/assembler.ts | 93 +++++++++++++++++-- .../negatives/neg.static-member-mixing.1.ts | 17 ++++ .../negatives/neg.static-member-mixing.2.ts | 11 +++ 4 files changed, 152 insertions(+), 9 deletions(-) create mode 100644 packages/jsii/doc/STATICS.md create mode 100644 packages/jsii/test/negatives/neg.static-member-mixing.1.ts create mode 100644 packages/jsii/test/negatives/neg.static-member-mixing.2.ts diff --git a/packages/jsii/doc/STATICS.md b/packages/jsii/doc/STATICS.md new file mode 100644 index 0000000000..df57fb0cbc --- /dev/null +++ b/packages/jsii/doc/STATICS.md @@ -0,0 +1,40 @@ +# Statics + +## Constraints + +### TypeScript + +Static and non-static members live in completely different namespaces. Statics only exist on the class, +and cannot be accessed through the class. + +Hence, it is perfectly fine to have a static and a non-static with the same name. + +Superclass statics can be accessed through subclasses, and they can be overridden by subclasses. + +### Java + +Statics and non-statics share a namespace. There cannot be a static and a +nonstatic with the same name on the same class. The same holds for inherited +members; a non-static in a superclass prevents a static of the same name in a +subclass, same for a static in a superclass and a nonstatic in a subclass. + +Superclass statics can be accessed through subclasses, and even through +instances and subclass instances. + +Statics can be overridden, though they do not participate in polymorphism. + +### C# + +Does not allow static and nonstatic members with the same name on the same class. + +**Will** allow static and nonstatic members of the same name on subclasses, +but will issue a compiler warning that the user probably intended to use the +`new` keyword to unambiguously introduce a new symbol. + +**Will** allow overriding of statics on a subclass, but will issue a warning +about the `new` keyword again. + +## Rules + +In order to accomodate Java, we disallow any inherited members to conflict on +staticness. \ No newline at end of file diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 03abb824f9..dce938cff6 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -159,6 +159,11 @@ export class Assembler implements Emitter { * * Will not invoke the function with any 'undefined's; an error will already have been emitted in * that case anyway. + * + * @param fqn FQN of the current type (the type that has a dependency on baseTypes) + * @param baseTypes Array of type references to be looked up + * @param referencingNode Node to report a diagnostic on if we fail to look up a t ype + * @param cb Callback to be invoked with the Types corresponding to the TypeReferences in baseTypes */ // tslint:disable-next-line:max-line-length private _deferUntilTypesAvailable(fqn: string, baseTypes: spec.NamedTypeReference[], referencingNode: ts.Node, cb: (...xs: spec.Type[]) => void) { @@ -585,9 +590,64 @@ export class Assembler implements Emitter { jsiiType.initializer = { initializer: true }; } + this._verifyNoStaticMixing(jsiiType, type.symbol.valueDeclaration); + return _sortMembers(jsiiType); } + /** + * Check that this class doesn't declare any members that are of different staticness in itself or any of its bases + */ + private _verifyNoStaticMixing(klass: spec.ClassType, decl: ts.Declaration) { + function stat(s?: boolean) { + return s ? 'static' : 'non-static'; + } + + // Check class itself--may have two methods/props with the same name, so check the arrays + const statics = new Set((klass.methods || []).concat(klass.properties || []).filter(x => x.static).map(x => x.name)); + const nonStatics = new Set((klass.methods || []).concat(klass.properties || []).filter(x => !x.static).map(x => x.name)); + // Intersect + for (const member of intersect(statics, nonStatics)) { + this._diagnostic(decl, ts.DiagnosticCategory.Error, + `member '${member}' of class '${klass.name}' cannot be declared both statically and non-statically`); + } + + // Check against base classes. They will not contain duplicate member names so we can load + // the members into a map. + const classMembers = typeMembers(klass); + this._withBaseClass(klass, decl, (base, recurse) => { + for (const [name, baseMember] of Object.entries(typeMembers(base))) { + const member = classMembers[name]; + if (!member) { continue; } + + if (!!baseMember.static !== !!member.static) { + this._diagnostic(decl, ts.DiagnosticCategory.Error, + // tslint:disable-next-line:max-line-length + `${stat(member.static)} member '${name}' of class '${klass.name}' conflicts with ${stat(baseMember.static)} member in ancestor '${base.name}'`); + } + } + + recurse(); + }); + } + + /** + * Wrapper around _deferUntilTypesAvailable, invoke the callback with the given classes' base type + * + * Does nothing if the given class doesn't have a base class. + * + * The second argument will be a `recurse` function for easy recursion up the inheritance tree + * (no messing around with binding 'self' and 'this' and doing multiple calls to _withBaseClass.) + */ + private _withBaseClass(klass: spec.ClassType, decl: ts.Declaration, cb: (base: spec.ClassType, recurse: () => void) => void) { + if (klass.base) { + this._deferUntilTypesAvailable(klass.fqn, [klass.base], decl, (base) => { + if (!spec.isClassType(base)) { throw new Error('Oh no'); } + cb(base, () => this._withBaseClass(base, decl, cb)); + }); + } + } + /** * @returns true if this member is internal and should be omitted from the type manifest */ @@ -791,13 +851,13 @@ export class Assembler implements Emitter { // Check that no interface declares a member that's already declared // in a base type (not allowed in C#). - const memberNames = interfaceMemberNames(jsiiType); + const names = memberNames(jsiiType); const checkNoIntersection = (...bases: spec.Type[]) => { for (const base of bases) { if (!spec.isInterfaceType(base)) { continue; } - const baseMembers = interfaceMemberNames(base); - for (const memberName of memberNames) { + const baseMembers = memberNames(base); + for (const memberName of names) { if (baseMembers.includes(memberName)) { this._diagnostic(type.symbol.declarations[0], ts.DiagnosticCategory.Error, @@ -1390,14 +1450,21 @@ function intersection(xs: Set, ys: Set): Set { * * Returns empty string for a non-interface type. */ -function interfaceMemberNames(jsiiType: spec.InterfaceType): string[] { - const ret = new Array(); - if (jsiiType.methods) { - ret.push(...jsiiType.methods.map(m => m.name).filter(x => x !== undefined) as string[]); +function memberNames(jsiiType: spec.InterfaceType | spec.ClassType): string[] { + return Object.keys(typeMembers(jsiiType)).filter(n => n !== ''); +} + +function typeMembers(jsiiType: spec.InterfaceType | spec.ClassType): {[key: string]: spec.Property | spec.Method} { + const ret: {[key: string]: spec.Property | spec.Method} = {}; + + for (const prop of jsiiType.properties || []) { + ret[prop.name] = prop; } - if (jsiiType.properties) { - ret.push(...jsiiType.properties.map(m => m.name)); + + for (const method of jsiiType.methods || []) { + ret[method.name || ''] = method; } + return ret; } @@ -1415,3 +1482,11 @@ function getConstructor(type: ts.Type): ts.Symbol | undefined { return type.symbol.members && type.symbol.members.get(ts.InternalSymbolName.Constructor); } + +function* intersect(xs: Set, ys: Set) { + for (const x of xs) { + if (ys.has(x)) { + yield x; + } + } +} \ No newline at end of file diff --git a/packages/jsii/test/negatives/neg.static-member-mixing.1.ts b/packages/jsii/test/negatives/neg.static-member-mixing.1.ts new file mode 100644 index 0000000000..3116d61a04 --- /dev/null +++ b/packages/jsii/test/negatives/neg.static-member-mixing.1.ts @@ -0,0 +1,17 @@ +///!MATCH_ERROR: non-static member 'funFunction' of class 'Sub' conflicts with static member in ancestor 'SuperDuper' + +export class SuperDuper { + public static funFunction() { + // Empty + } +} + +export class Super extends SuperDuper { + // Empty +} + +export class Sub extends Super { + public funFunction() { + // Oops + } +} \ No newline at end of file diff --git a/packages/jsii/test/negatives/neg.static-member-mixing.2.ts b/packages/jsii/test/negatives/neg.static-member-mixing.2.ts new file mode 100644 index 0000000000..82370197d3 --- /dev/null +++ b/packages/jsii/test/negatives/neg.static-member-mixing.2.ts @@ -0,0 +1,11 @@ +///!MATCH_ERROR: member 'funFunction' of class 'TheClass' cannot be declared both statically and non-statically + +export class TheClass { + public static funFunction() { + // Empty + } + + public funFunction() { + // Empty + } +} \ No newline at end of file