fix(pacmak): illegal static overrides in java & c# (#2373)

Stop emitting "overrides" in the respecting language idiomatic way for static methods and properties (while ES6 supports those, this is not true of Java and C#). In Java, this is mostly getting rid of `@Overrides` on static declarations. C# *allows* (but does not require) explicitly hiding the parent declaration using the `new` keyword. This was introduced as it provides additional safeguards against our generating incorrect code. Fixes #2358 --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
aws · Dec 22, 2020 · 4672e4b · 4672e4b
1 parent 9e00b2f
commit 4672e4b
Show file tree

Hide file tree

Showing 12 changed files with 516 additions and 14 deletions.
diff --git a/gh-pages/content/language-support/assembly.md b/gh-pages/content/language-support/assembly.md
@@ -167,4 +167,8 @@ Methods that are `static` cannot feature the `overrides` attribute, as `static`
 Properties that are `const` must have a `name` that is `UPPER_SNAKE_CASED`. They represent constants similar to [Enum
 Members], which can be proactively resolved by the `jsii` runtimes.
 
-Properties that are `static` cannot feature the `overrides` attribute, as `static` members are not inherited.
+!!! danger
+    Properties and methods that are `static` **can** feature the `overrides` attribute, as `static` members are
+    inherited with the prototype in **JavaScript** (as part of the ES6 specification). Not all target languages have
+    this capability (most, like **C#** and **Java**, only support *hiding* static declarations), and consequently code
+    generators may ignore this (or explicitly hide parent declarations) instead.
diff --git a/packages/jsii-calc/lib/compliance.ts b/packages/jsii-calc/lib/compliance.ts
@@ -2840,3 +2840,36 @@ export namespace LevelOne {
     readonly value: boolean;
   }
 }
+
+/**
+ * Static methods that override parent class are technically overrides (the
+ * inheritance of statics is part of the ES6 specification), but certain other
+ * languages such as Java do not carry statics in the inheritance chain at all,
+ * so they cannot be overridden, only hidden.
+ *
+ * The difference is fairly minor (for typical use-cases, the end result is the
+ * same), however this has implications on what the generated code should look
+ * like.
+ */
+export class StaticHelloParent {
+  public static get property(): number {
+    return 1337;
+  }
+
+  public static method(): void {
+    /* A static method */
+  }
+}
+export class StaticHelloChild extends StaticHelloParent {
+  public static get property(): number {
+    return 42;
+  }
+
+  public static method(): void {
+    /* An ES6 Override */
+  }
+
+  private constructor() {
+    super();
+  }
+}
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -11422,6 +11422,102 @@
         }
       ]
     },
+    "jsii-calc.StaticHelloChild": {
+      "assembly": "jsii-calc",
+      "base": "jsii-calc.StaticHelloParent",
+      "docs": {
+        "stability": "stable"
+      },
+      "fqn": "jsii-calc.StaticHelloChild",
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2863
+      },
+      "methods": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2868
+          },
+          "name": "method",
+          "overrides": "jsii-calc.StaticHelloParent",
+          "static": true
+        }
+      ],
+      "name": "StaticHelloChild",
+      "properties": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2864
+          },
+          "name": "property",
+          "overrides": "jsii-calc.StaticHelloParent",
+          "static": true,
+          "type": {
+            "primitive": "number"
+          }
+        }
+      ]
+    },
+    "jsii-calc.StaticHelloParent": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "remarks": "The difference is fairly minor (for typical use-cases, the end result is the\nsame), however this has implications on what the generated code should look\nlike.",
+        "stability": "stable",
+        "summary": "Static methods that override parent class are technically overrides (the inheritance of statics is part of the ES6 specification), but certain other languages such as Java do not carry statics in the inheritance chain at all, so they cannot be overridden, only hidden."
+      },
+      "fqn": "jsii-calc.StaticHelloParent",
+      "initializer": {
+        "docs": {
+          "stability": "stable"
+        }
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2854
+      },
+      "methods": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2859
+          },
+          "name": "method",
+          "static": true
+        }
+      ],
+      "name": "StaticHelloParent",
+      "properties": [
+        {
+          "docs": {
+            "stability": "stable"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2855
+          },
+          "name": "property",
+          "static": true,
+          "type": {
+            "primitive": "number"
+          }
+        }
+      ]
+    },
     "jsii-calc.Statics": {
       "assembly": "jsii-calc",
       "docs": {
@@ -14345,5 +14441,5 @@
     }
   },
   "version": "0.0.0",
-  "fingerprint": "XfCnzPEGbEJR6hhBX8zWyFzWJP2wH9ztW2ZcW6Wdb+4="
+  "fingerprint": "8y6c87ScX7dJInuohtU3oLM8TCUz8yoYZ+j14fnHG9s="
 }
diff --git a/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts b/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
@@ -476,7 +476,7 @@ export class DotNetGenerator extends Generator {
     const returnType = method.returns
       ? this.typeresolver.toDotNetType(method.returns.type)
       : 'void';
-    let staticKeyWord = '';
+    const staticKeyWord = method.static ? 'static ' : '';
     let overrideKeyWord = '';
     let virtualKeyWord = '';
 
@@ -496,12 +496,14 @@ export class DotNetGenerator extends Generator {
         overrides = true;
       }
     }
-    if (method.static) {
-      staticKeyWord = 'static ';
-    } else if (overrides) {
-      // Add the override key word if the method is emitted for a proxy or data type or is defined on an ancestor
-      overrideKeyWord = 'override ';
+    if (overrides) {
+      // Add the override key word if the method is emitted for a proxy or data type or is defined on an ancestor. If
+      // the member is static, use the "new" keyword instead, to indicate we are intentionally hiding the ancestor
+      // declaration (as C# does not inherit statics, they can be hidden but not overridden). The "new" keyword is
+      // optional in this context, but helps clarify intention.
+      overrideKeyWord = method.static ? 'new ' : 'override ';
     } else if (
+      !method.static &&
       (method.abstract || !definedOnAncestor) &&
       !emitForProxyOrDatatype
     ) {
@@ -832,8 +834,11 @@ export class DotNetGenerator extends Generator {
         prop,
       );
       if (implementedInBase || datatype || proxy) {
-        // Override if the property is in a datatype or proxy class or declared in a parent class
-        isOverrideKeyWord = 'override ';
+        // Override if the property is in a datatype or proxy class or declared in a parent class. If the member is
+        // static, use the "new" keyword instead, to indicate we are intentionally hiding the ancestor declaration (as
+        // C# does not inherit statics, they can be hidden but not overridden).The "new" keyword is optional in this
+        // context, but helps clarify intention.
+        isOverrideKeyWord = prop.static ? 'new ' : 'override ';
       } else if (prop.abstract) {
         // Abstract members get decorated as such
         isAbstractKeyword = 'abstract ';
@@ -845,7 +850,7 @@ export class DotNetGenerator extends Generator {
 
     const propTypeFQN = this.typeresolver.toDotNetType(prop.type);
     const isOptional = prop.optional ? '?' : '';
-    const statement = `${access} ${isAbstractKeyword}${isVirtualKeyWord}${isOverrideKeyWord}${staticKeyWord}${propTypeFQN}${isOptional} ${propName}`;
+    const statement = `${access} ${isAbstractKeyword}${isVirtualKeyWord}${staticKeyWord}${isOverrideKeyWord}${propTypeFQN}${isOptional} ${propName}`;
     this.code.openBlock(statement);
 
     // Emit getters

diff --git a/packages/jsii-pacmak/lib/targets/java.ts b/packages/jsii-pacmak/lib/targets/java.ts
@@ -1274,7 +1274,7 @@ class JavaGenerator extends Generator {
     if (includeGetter) {
       this.code.line();
       this.addJavaDocs(prop);
-      if (overrides) {
+      if (overrides && !prop.static) {
         this.code.line('@Override');
       }
       this.emitStabilityAnnotations(prop);
@@ -1307,7 +1307,7 @@ class JavaGenerator extends Generator {
       for (const type of setterTypes) {
         this.code.line();
         this.addJavaDocs(prop);
-        if (overrides) {
+        if (overrides && !prop.static) {
           this.code.line('@Override');
         }
         this.emitStabilityAnnotations(prop);
@@ -1364,7 +1364,7 @@ class JavaGenerator extends Generator {
     this.code.line();
     this.addJavaDocs(method);
     this.emitStabilityAnnotations(method);
-    if (overrides) {
+    if (overrides && !method.static) {
       this.code.line('@Override');
     }
     if (method.abstract && !defaultImpl) {

diff --git a/packages/jsii-pacmak/test/generated-code/__snapshots__/target-dotnet.test.ts.snap b/packages/jsii-pacmak/test/generated-code/__snapshots__/target-dotnet.test.ts.snap