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.

Author: rix0rrr

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.

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<T>(xs: Set<T>, ys: Set<T>): Set<T> {
  *
  * Returns empty string for a non-interface type.
  */
-function interfaceMemberNames(jsiiType: spec.InterfaceType): string[] {
-  const ret = new Array<string>();
-  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<T>(xs: Set<T>, ys: Set<T>) {
+  for (const x of xs) {
+    if (ys.has(x)) {
+      yield x;
+    }
+  }
+}

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
+  }
+}

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
+  }
+}