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
aws · Dec 16, 2019 · fc8db71 · fc8db71
1 parent 6b64ced
commit fc8db71
Show file tree

Hide file tree

Showing 8 changed files with 52 additions and 42 deletions.
diff --git a/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts b/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
@@ -643,8 +643,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();
@@ -661,26 +661,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) {
@@ -691,7 +696,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
@@ -888,6 +888,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.
@@ -896,18 +897,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) {
@@ -916,18 +922,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} ${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
@@ -39,9 +39,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.

diff --git a/...JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/AbstractClassBase.cs b/...JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/AbstractClassBase.cs
@@ -24,9 +24,9 @@ protected AbstractClassBase(DeputyProps props): base(props)
         /// stability: Experimental
         /// </remarks>
         [JsiiProperty(name: "abstractProperty", typeJson: "{\"primitive\":\"string\"}")]
-        public virtual string AbstractProperty
+        public abstract string AbstractProperty
         {
-            get => GetInstanceProperty<string>();
+            get;
         }
     }
 }
diff --git a/...lculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/composition/CompositeOperation.cs b/...lculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/composition/CompositeOperation.cs
@@ -36,9 +36,9 @@ public override string ToString()
         /// stability: Experimental
         /// </remarks>
         [JsiiProperty(name: "expression", typeJson: "{\"fqn\":\"@scope/jsii-calc-lib.Value\"}")]
-        public virtual Amazon.JSII.Tests.CalculatorNamespace.LibNamespace.Value_ Expression
+        public abstract Amazon.JSII.Tests.CalculatorNamespace.LibNamespace.Value_ Expression
         {
-            get => GetInstanceProperty<Amazon.JSII.Tests.CalculatorNamespace.LibNamespace.Value_>();
+            get;
         }
 
         /// <summary>The value.</summary>

diff --git a/...jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/AbstractClassBase.java b/...jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/AbstractClassBase.java
@@ -25,9 +25,7 @@ protected AbstractClassBase() {
      * EXPERIMENTAL
      */
     @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
-    public java.lang.String getAbstractProperty() {
-        return this.jsiiGet("abstractProperty", java.lang.String.class);
-    }
+    public abstract java.lang.String getAbstractProperty();
 
     /**
      * A proxy class which represents a concrete javascript instance of this type.

diff --git a/...a/src/main/java/software/amazon/jsii/tests/calculator/composition/CompositeOperation.java b/...a/src/main/java/software/amazon/jsii/tests/calculator/composition/CompositeOperation.java
@@ -40,9 +40,7 @@ public java.lang.String toString() {
      * EXPERIMENTAL
      */
     @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
-    public software.amazon.jsii.tests.calculator.lib.Value getExpression() {
-        return this.jsiiGet("expression", software.amazon.jsii.tests.calculator.lib.Value.class);
-    }
+    public abstract software.amazon.jsii.tests.calculator.lib.Value getExpression();
 
     /**
      * The value.