Java - problems implementing class/interface with optional property

[RomainMuller]

When implementing an abstract class or interface that defines optional properties, but not actually specifying the property (valid in TypeScript), the Java code generator will never emit the concrete implementation of the getter, resulting in an invalid class.

[zoewangg]

repro case: adding an optionalString field to the existing IInterfaceWithProperties and the concrete class ClassWithPrivateConstructorAndAutomaticProperties would fail to compile because there are no getters/setters for the optionalString.

export interface IInterfaceWithProperties {
    readonly readOnlyString: string;
    readWriteString: string;
    optionalString?: string;
}

@RomainMuller @eladb It appears that the root cause for this issue is when generating jsii file, the optional properties from the interface are not generated/included in the concrete classes. Is that intentional? If not, I will work on the fix. (I'd expect it generates inherited properties in jsii file)

Generated jsii snippets as follows:

    "jsii-calc.ClassWithPrivateConstructorAndAutomaticProperties": {
      // ...
      "interfaces": [
        "jsii-calc.IInterfaceWithProperties"
      ],
      "kind": "class",
     // ...
      "name": "ClassWithPrivateConstructorAndAutomaticProperties",
      "properties": [
        {
          "docs": {
            "stability": "experimental"
          },
          "immutable": true,
          "locationInModule": {
            "filename": "lib/compliance.ts",
            "line": 580
          },
          "name": "readOnlyString",
          "overrides": "jsii-calc.IInterfaceWithProperties",
          "type": {
            "primitive": "string"
          }
        },
        {
          "docs": {
            "stability": "experimental"
          },
          "locationInModule": {
            "filename": "lib/compliance.ts",
            "line": 581
          },
          "name": "readWriteString",
          "overrides": "jsii-calc.IInterfaceWithProperties",
          "type": {
            "primitive": "string"
          }
        }
       // missing the optionalString field.
      ]
    }

[rix0rrr]

Looks like the property is not in there because the class doesn't define it. It's probably fine to add it in, but I have no idea what that would do to the rest of the codebase.

jsii-reflect, which is a library to inspect JSII assemblies will probably properly tell you about it. It might also be safer to use jsii-reflect inside jsii-pacmak to reflect on the assembly and find the properties that way, without having to change the underlying generated assembly.

Go whichever way suits you. If you decide to with modifying the generated assembly, please make sure the code generated for the other languages also still makes sense.

[zoewangg]

FWIW, .net is also affected by this bug, and potentially all static languages.

Looks like there are three options:

1. Update assembler to include the inherited optional properties in the concrete classes.

• Pros
  • Fix for all static languages
• Cons
  • Might affect the generations for other languages.

2. as @rix0rrr mentioned, use jsii-reflect in jsii-pacmak to collect the properties on demand.
   I'm not exactly sure how the code would look like, but seems like it's less risky compared with option1. It would also affect generations for other languages I'd assume. Please correct me if I'm wrong.

3. Update generator.ts to handle it. Add a flag hook in generator.ts, say isVisitIheritedProperties, and make it return false by default. Override that method in java and .net generators to return true. In generator#visitClass method, if isVisitIheritedProperties is true, it will collect all properties from the interfaces/abstract classes(if any) and call onProperty in the for loop.

• Pros
  • Fixes the issue for java and .net without affecting other languages

I personally prefer option 3. Please let me know what you think or if you have other ideas.

[eladb]

I think # 1 would work. What are we worried about exactly?

[rix0rrr]

Maybe nothing. After thinking about it some more, it seems to make sense that classes should have the members listed out, and it shouldn't have any negative consequences.

We have to think about the rules though, because (as is usual) inheritance will throw a spanner in the works of naive implementations:

interface IParent { 
  readonly optionalProp?: string;
}

interface ISomething extends IParent { }

class Concrete1 implements ISomething { }

class Concrete2a extends Concrete1 { }
class Concrete2b extends Concrete1 implements ISomething { }

In the case of Concrete1 it's (somewhat) easy to say that there's a missing declaration of optionalProp.

Both in Concrete2a and Concrete2b I feel that the optional declaration of optionalProp should NOT be included, so that we can correctly determine that we would have inherited the implementation from Concrete1.

[eladb]

Alternatively, we can enforce that implementing classes will define all abstract members explicitly so basically it leaves the edge case resolution to users. That would be my preference.

[zoewangg]

Thank you both for your responses!

@eladb Do you mean having Concrete2a and Concrete2b to also declare the optionalProp?

I actually agree with @rix0rrr It seems a bit odd to me to declare the property in Concrete2a and Concreate2b if they do not intend to override the property at all.

There are also other questions with option 1 if we decide to define all inherited members? do we declare optionalProp in ISomething This is valid in Java.

In my opinion, option 3 seems more flexible, because different languages have different inheritance rules and having generator to handle it makes it easier to define different behaviors for different languages.

[eladb]

Yes, I mean that jsii will enforce that all abstract fields will always have a concrete implementation in concrete classes. The fact that in typescript you don't have to explicitly implement abstract fields that can be undefined is "sugar" that stems from how javascript works. For all intents and purposes, these fields are defined with the value undefined.

We had a few cases in the past where jsii reduced some implicit behavior from typescript (one example is how we implemented @internal by enforcing that internal fields will have a "_" prefix) in order to ensure that users are aware of the implications.

Bottom line: I think the .jsii spec should reflect that these fields are available on the derived classes. I am okay with doing that implicitly, and also ok with just adding a validation.

[RomainMuller]

We should generate interfaces w/ default implementations for optional members (that do nothing / return null) in both Java and C# so that adding optional members does not cause use code breakage.

[RomainMuller]

Note that while you can opt to not implement an optional interface property, you are required to implement all abstract properties of a parent class (or be abstract):

abstract class ExtendMe {
  abstract field: string;
}
class DidThat extends ExtendMe {
  // TS2515: The non-abstract class 'DidThat' does not implement abstract member 'field' inherited from class 'ExtendMe'
}