fix(java,dotnet): abstract properties have concrete implementations (#…

…1128) * fix(java,dotnet): abstract properties have concrete implementations The generated code for abstract properties in Java and C# included fully concrete implementations, instead of an abstract declaration. This made it possible to subclass those types without actually implementing those members, resulting in invalid code. This changes the code generation to actually emit the `abstract` keyword and not generate a full concrete implementation. Fixes #240 Fixes #1011 * add some test coverage
aws · Dec 19, 2019 · c9351a3 · c9351a3
1 parent 242c55f
commit c9351a3
Show file tree

Hide file tree

Showing 22 changed files with 612 additions and 47 deletions.
diff --git a/...es/@jsii/dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs b/...es/@jsii/dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs
@@ -1398,5 +1398,30 @@ public void LiftedKwargWithSameNameAsPositionalArg()
             Assert.Equal(bell, amb.Scope);
             Assert.Equal("Driiiing!", amb.Props.Scope);
         }
+
+        [Fact(DisplayName = Prefix + nameof(AbstractMembersAreCorrectlyHandled))]
+        public void AbstractMembersAreCorrectlyHandled()
+        {
+            var abstractSuite = new AbstractSuiteImpl();
+            Assert.Equal("Wrapped<String<Oomf!>>", abstractSuite.WorkItAll("Oomf!"));
+        }
+
+        private sealed class AbstractSuiteImpl : AbstractSuite
+        {
+            private string _property;
+
+            public AbstractSuiteImpl() {}
+
+            protected override string SomeMethod(string str)
+            {
+                return $"Wrapped<{str}>";
+            }
+
+            protected override string Property
+            {
+                get => _property;
+                set => _property = $"String<{value}>";
+            }
+        }
     }
 }
diff --git a/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs b/packages/@jsii/dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs
@@ -155,7 +155,7 @@ private static CallbackResult InvokeGetter(GetRequest request, IReferenceMap ref
 
             var attribute = propertyInfo.GetAttribute<JsiiPropertyAttribute>();
 
-            var methodInfo = propertyInfo.GetGetMethod();
+            var methodInfo = propertyInfo.GetMethod;
             if (methodInfo == null)
             {
                 throw new InvalidOperationException($"Received callback for {deputy.GetType().Name}.{request.Property} getter, but this property does not have a getter");
@@ -175,7 +175,7 @@ private static void InvokeSetter(SetRequest request, IReferenceMap referenceMap)
                 throw new InvalidOperationException($"Received callback for {deputy.GetType().Name}.{request.Property} setter, but this property does not exist");
             }
 
-            var methodInfo = propertyInfo.GetSetMethod();
+            var methodInfo = propertyInfo.SetMethod;
             if (methodInfo == null)
             {
                 throw new InvalidOperationException($"Received callback for {deputy.GetType().Name}.{request.Property} setter, but this property does not have a setter");

diff --git a/.../java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java b/.../java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
@@ -1667,4 +1667,28 @@ public void liftedKwargWithSameNameAsPositionalArg() {
         assertEquals(bell, amb.getScope());
         assertEquals(StructParameterType.builder().scope("Driiiing!").build(), amb.getProps());
     }
+
+    @Test
+    public void abstractMembersAreCorrectlyHandled() {
+        final AbstractSuite abstractSuite = new AbstractSuite() {
+            private String property;
+
+            @Override
+            protected String someMethod(String str) {
+                return String.format("Wrapped<%s>", str);
+            }
+
+            @Override
+            protected String getProperty() {
+                return this.property;
+            }
+
+            @Override
+            protected void setProperty(String value) {
+                this.property = String.format("String<%s>", value);
+            }
+        };
+
+        assertEquals("Wrapped<String<Oomf!>>", abstractSuite.workItAll("Oomf!"));
+    }
 }
diff --git a/packages/@jsii/python-runtime/tests/test_compliance.py b/packages/@jsii/python-runtime/tests/test_compliance.py
@@ -10,6 +10,7 @@
 
 from jsii_calc import (
     AbstractClassReturner,
+    AbstractSuite,
     Add,
     AllTypes,
     AllTypesEnum,
@@ -1130,3 +1131,19 @@ def test_lifted_kwarg_with_same_name_as_positional_arg():
 
     assert amb.scope == bell
     assert amb.props == StructParameterType(scope='Driiiing!')
+
+def test_abstract_members_are_correctly_handled():
+    class AbstractSuiteImpl(AbstractSuite):
+        @property
+        def _property(self):
+            return self.property
+
+        @_property.setter
+        def _property(self, value):
+            self.property = "String<%s>" % value
+
+        def _some_method(self, str):
+            return "Wrapped<%s>" % str
+
+    abstract_suite = AbstractSuiteImpl()
+    assert "Wrapped<String<Oomf!>>" == abstract_suite.work_it_all("Oomf!")
diff --git a/packages/jsii-calc/lib/calculator.ts b/packages/jsii-calc/lib/calculator.ts
@@ -394,3 +394,20 @@ export interface SmellyStruct {
     readonly property: string;
     readonly yetAnoterOne: boolean;
 }
+
+/**
+ * Ensures abstract members implementations correctly register overrides in various languages.
+ */
+export abstract class AbstractSuite {
+    protected abstract property: string;
+    protected abstract someMethod(str: string): string;
+
+    /**
+     * Sets `seed` to `this.property`, then calls `someMethod` with `this.property` and returns the result.
+     * @param seed a `string`.
+     */
+    public workItAll(seed: string) {
+        this.property = seed;
+        return this.someMethod(this.property);
+    }
+}
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -401,6 +401,93 @@
         }
       ]
     },
+    "jsii-calc.AbstractSuite": {
+      "abstract": true,
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "experimental",
+        "summary": "Ensures abstract members implementations correctly register overrides in various languages."
+      },
+      "fqn": "jsii-calc.AbstractSuite",
+      "initializer": {},
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/calculator.ts",
+        "line": 401
+      },
+      "methods": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "locationInModule": {
+            "filename": "lib/calculator.ts",
+            "line": 403
+          },
+          "name": "someMethod",
+          "parameters": [
+            {
+              "name": "str",
+              "type": {
+                "primitive": "string"
+              }
+            }
+          ],
+          "protected": true,
+          "returns": {
+            "type": {
+              "primitive": "string"
+            }
+          }
+        },
+        {
+          "docs": {
+            "stability": "experimental",
+            "summary": "Sets `seed` to `this.property`, then calls `someMethod` with `this.property` and returns the result."
+          },
+          "locationInModule": {
+            "filename": "lib/calculator.ts",
+            "line": 409
+          },
+          "name": "workItAll",
+          "parameters": [
+            {
+              "docs": {
+                "summary": "a `string`."
+              },
+              "name": "seed",
+              "type": {
+                "primitive": "string"
+              }
+            }
+          ],
+          "returns": {
+            "type": {
+              "primitive": "string"
+            }
+          }
+        }
+      ],
+      "name": "AbstractSuite",
+      "properties": [
+        {
+          "abstract": true,
+          "docs": {
+            "stability": "experimental"
+          },
+          "locationInModule": {
+            "filename": "lib/calculator.ts",
+            "line": 402
+          },
+          "name": "property",
+          "protected": true,
+          "type": {
+            "primitive": "string"
+          }
+        }
+      ]
+    },
     "jsii-calc.Add": {
       "assembly": "jsii-calc",
       "base": "jsii-calc.BinaryOperation",
@@ -12002,5 +12089,5 @@
     }
   },
   "version": "0.20.11",
-  "fingerprint": "eenqZx1+dNMvfZCY02AIrinXeK98iQC4XLLj0yPIXuM="
+  "fingerprint": "2idHJnE1Vv8YAFFrhqRzB2SwAvTXdSFkD7jKYcbabX4="
 }
diff --git a/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts b/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
@@ -628,8 +628,8 @@ export class DotNetGenerator extends Generator {
   }
 
   /**
-     * Emits a property
-     */
+   * Emits a property
+   */
   private emitProperty(cls: spec.Type, prop: spec.Property, datatype = false, proxy = false): void {
 
     this.emitNewLineIfNecessary();
@@ -646,26 +646,31 @@ export class DotNetGenerator extends Generator {
     this.dotnetRuntimeGenerator.emitAttributesForProperty(prop, datatype);
 
     let isOverrideKeyWord = '';
-
     let isVirtualKeyWord = '';
+    let isAbstractKeyword = '';
+
     // If the prop parent is a class
     if (cls.kind === spec.TypeKind.Class) {
       const implementedInBase = this.isMemberDefinedOnAncestor(cls as spec.ClassType, prop);
       if (implementedInBase || datatype || proxy) {
         // Override if the property is in a datatype or proxy class or declared in a parent class
         isOverrideKeyWord = 'override ';
-      } else if (!prop.static && (prop.abstract || !implementedInBase)) {
-        // Virtual if the prop is not static, and is abstract or not implemented in base member, this way we can later override it.
+      } else if (prop.abstract) {
+        // Abstract members get decorated as such
+        isAbstractKeyword = 'abstract ';
+      } else if (!prop.static && !implementedInBase) {
+        // Virtual if the prop is not static, and is not implemented in base member, this way we can later override it.
         isVirtualKeyWord = 'virtual ';
       }
     }
+
     const propTypeFQN = this.typeresolver.toDotNetType(prop.type);
     const isOptionalPrimitive = this.isOptionalPrimitive(prop) ? '?' : '';
-    const statement = `${access} ${isVirtualKeyWord}${isOverrideKeyWord}${staticKeyWord}${propTypeFQN}${isOptionalPrimitive} ${propName}`;
+    const statement = `${access} ${isAbstractKeyword}${isVirtualKeyWord}${isOverrideKeyWord}${staticKeyWord}${propTypeFQN}${isOptionalPrimitive} ${propName}`;
     this.code.openBlock(statement);
 
     // Emit getters
-    if (datatype || prop.const) {
+    if (datatype || prop.const || prop.abstract) {
       this.code.line('get;');
     } else {
       if (prop.static) {
@@ -676,7 +681,7 @@ export class DotNetGenerator extends Generator {
     }
 
     // Emit setters
-    if (datatype) {
+    if (datatype || (!prop.immutable && prop.abstract)) {
       this.code.line('set;');
     } else {
       if (!prop.immutable) {

diff --git a/packages/jsii-pacmak/lib/targets/java.ts b/packages/jsii-pacmak/lib/targets/java.ts
@@ -894,6 +894,7 @@ class JavaGenerator extends Generator {
     const propName = this.code.toPascalCase(JavaGenerator.safeJavaPropertyName(prop.name));
     const access = this.renderAccessLevel(prop);
     const statc = prop.static ? 'static ' : '';
+    const abstract = prop.abstract ? 'abstract ' : '';
     const javaClass = this.toJavaType(cls);
 
     // for unions we only generate overloads for setters, not getters.
@@ -902,18 +903,23 @@ class JavaGenerator extends Generator {
       this.addJavaDocs(prop);
       if (overrides) { this.code.line('@Override'); }
       this.emitStabilityAnnotations(prop);
-      this.code.openBlock(`${access} ${statc}${getterType} get${propName}()`);
-      let statement;
-      if (prop.static) {
-        statement = `software.amazon.jsii.JsiiObject.jsiiStaticGet(${javaClass}.class, `;
+      const signature = `${access} ${abstract}${statc}${getterType} get${propName}()`;
+      if (prop.abstract) {
+        this.code.line(`${signature};`);
       } else {
-        statement = 'this.jsiiGet(';
-      }
+        this.code.openBlock(signature);
+        let statement;
+        if (prop.static) {
+          statement = `software.amazon.jsii.JsiiObject.jsiiStaticGet(${javaClass}.class, `;
+        } else {
+          statement = 'this.jsiiGet(';
+        }
 
-      statement += `"${prop.name}", ${propClass}.class)`;
+        statement += `"${prop.name}", ${propClass}.class)`;
 
-      this.code.line(`return ${this.wrapCollection(statement, prop.type, prop.optional)};`);
-      this.code.closeBlock();
+        this.code.line(`return ${this.wrapCollection(statement, prop.type, prop.optional)};`);
+        this.code.closeBlock();
+      }
     }
 
     if (!prop.immutable) {
@@ -922,18 +928,23 @@ class JavaGenerator extends Generator {
         this.addJavaDocs(prop);
         if (overrides) { this.code.line('@Override'); }
         this.emitStabilityAnnotations(prop);
-        this.code.openBlock(`${access} ${statc}void set${propName}(final ${type} value)`);
-        let statement = '';
-
-        if (prop.static) {
-          statement += `software.amazon.jsii.JsiiObject.jsiiStaticSet(${javaClass}.class, `;
+        const signature = `${access} ${abstract}${statc}void set${propName}(final ${type} value)`;
+        if (prop.abstract) {
+          this.code.line(`${signature};`);
         } else {
-          statement += 'this.jsiiSet(';
+          this.code.openBlock(signature);
+          let statement = '';
+
+          if (prop.static) {
+            statement += `software.amazon.jsii.JsiiObject.jsiiStaticSet(${javaClass}.class, `;
+          } else {
+            statement += 'this.jsiiSet(';
+          }
+          const value = prop.optional ? 'value' : `java.util.Objects.requireNonNull(value, "${prop.name} is required")`;
+          statement += `"${prop.name}", ${value});`;
+          this.code.line(statement);
+          this.code.closeBlock();
         }
-        const value = prop.optional ? 'value' : `java.util.Objects.requireNonNull(value, "${prop.name} is required")`;
-        statement += `"${prop.name}", ${value});`;
-        this.code.line(statement);
-        this.code.closeBlock();
       }
     }
   }

diff --git a/...ulatorPackageId.LibPackageId/Amazon/JSII/Tests/CalculatorNamespace/LibNamespace/Value_.cs b/...ulatorPackageId.LibPackageId/Amazon/JSII/Tests/CalculatorNamespace/LibNamespace/Value_.cs
@@ -44,9 +44,9 @@ public override string ToString()
         /// </remarks>
         [JsiiProperty(name: "value", typeJson: "{\"primitive\":\"number\"}")]
         [System.Obsolete()]
-        public virtual double Value
+        public abstract double Value
         {
-            get => GetInstanceProperty<double>();
+            get;
         }
     }
 }
diff --git a/...ted.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/Value.java b/...ted.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/Value.java
@@ -36,9 +36,7 @@ public java.lang.String toString() {
      */
     @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Deprecated)
     @Deprecated
-    public java.lang.Number getValue() {
-        return this.jsiiGet("value", java.lang.Number.class);
-    }
+    public abstract java.lang.Number getValue();
 
     /**
      * A proxy class which represents a concrete javascript instance of this type.