fix: omitting optional property fails C# compilation (#1448)

The following is a minimal reproducing example: ```ts export interface ISomeInterface { readonly xyz?: string; } export abstract class SomeClass implements ISomeInterface { } ``` This breaks code generation for C#, as the `SomeClass._Proxy` class we generate will try to generate an `override` for the `Xyz` property, which is missing from the `SomeClass` class itself. This issue is exhibited by a difference in logic between how we iterate the members of a class when we generate the class itself vs when we generate its proxy. However, after looking at the code I'm not convinced it's correct either way (nor for Java), so to be safe we picked the solution where we force the user to declare the property on the class. The correct declaration would be: ```ts export abstract class SomeClass implements ISomeInterface { public abstract readonly xyz?: string; } ``` Related: https://github.com/aws/aws-cdk/pull/31946/files --- 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 --------- Signed-off-by: github-actions <github-actions@github.com> Co-authored-by: github-actions <github-actions@github.com>

Author: rix0rrr

src/jsii-diagnostic.ts

@@ -628,6 +628,13 @@ export class JsiiDiagnostic implements ts.Diagnostic {
  name: 'language-compatibility/static-member-name-conflicts-with-nested-type',
  });
 
+ public static readonly JSII_5021_ABSTRACT_CLASS_MISSING_PROP_IMPL = Code.error({
+ code: 5021,
+ formatter: (intf: spec.InterfaceType, cls: spec.ClassType, prop: string) =>
+ `A declaration of "${intf.name}.${prop}" is missing on class "${cls.name}". Declare the property as "public abstract" if you want to defer it to subclasses.`,
+ name: 'language-compatibility/abstract-class-missing-prop-impl',
+ });
+
  //////////////////////////////////////////////////////////////////////////////
  // 6000 => 6999 -- RESERVED
 

src/validator.ts

@@ -2,7 +2,6 @@ import * as assert from 'node:assert';
 import * as spec from '@jsii/spec';
 import * as deepEqual from 'fast-deep-equal';
 import * as ts from 'typescript';
-
 import * as Case from './case';
 import { Emitter } from './emitter';
 import { JsiiDiagnostic } from './jsii-diagnostic';
@@ -41,6 +40,7 @@ function _defaultValidations(): ValidationFunction[] {
  _allTypeReferencesAreValid,
  _inehritanceDoesNotChangeContracts,
  _staticMembersAndNestedTypesMustNotSharePascalCaseName,
+ _abstractClassesMustImplementAllProperties,
  ];
 
  function _enumMembersMustUserUpperSnakeCase(_: Validator, assembly: spec.Assembly, diagnostic: DiagnosticEmitter) {
@@ -382,7 +382,7 @@ function _defaultValidations(): ValidationFunction[] {
  expectedNode?.modifiers?.find(
  (mod) => mod.kind === ts.SyntaxKind.PublicKeyword || mod.kind === ts.SyntaxKind.ProtectedKeyword,
  ) ?? declarationName(expectedNode),
- 'The implemented delcaration is here.',
+ 'The implemented declaration is here.',
  ),
  );
  }
@@ -396,7 +396,7 @@ function _defaultValidations(): ValidationFunction[] {
  expected.type,
  ).maybeAddRelatedInformation(
  expectedNode?.type ?? declarationName(expectedNode),
- 'The implemented delcaration is here.',
+ 'The implemented declaration is here.',
  ),
  );
  }
@@ -412,7 +412,7 @@ function _defaultValidations(): ValidationFunction[] {
  ).maybeAddRelatedInformation(
  expectedNode?.modifiers?.find((mod) => mod.kind === ts.SyntaxKind.ReadonlyKeyword) ??
  declarationName(expectedNode),
- 'The implemented delcaration is here.',
+ 'The implemented declaration is here.',
  ),
  );
  }
@@ -426,13 +426,90 @@ function _defaultValidations(): ValidationFunction[] {
  expected.optional,
  ).maybeAddRelatedInformation(
  expectedNode?.questionToken ?? expectedNode?.type ?? declarationName(expectedNode),
- 'The implemented delcaration is here.',
+ 'The implemented declaration is here.',
  ),
  );
  }
  }
  }
 
+ /**
+ * Abstract classes that implement an interface should have a declaration for every member.
+ *
+ * For non-optional members, TypeScript already enforces this. This leaves the user the
+ * ability to forget optional properties (`readonly prop?: string`).
+ *
+ * At least our codegen for this case fails in C#, and I'm not convinced it does the right
+ * thing in Java either. So we will disallow this, and require users to declare these
+ * fields on the class. It can always be `public abstract readonly prop?: string` if they
+ * don't want to give an implementation yet.
+ */
+ function _abstractClassesMustImplementAllProperties(
+ validator: Validator,
+ assembly: spec.Assembly,
+ diagnostic: DiagnosticEmitter,
+ ) {
+ for (const type of _allTypes(assembly)) {
+ if (!spec.isClassType(type) || !type.abstract) {
+ continue;
+ }
+
+ const classProps = collectClassProps(type, new Set());
+
+ for (const implFqn of type.interfaces ?? []) {
+ checkInterfacePropsImplemented(implFqn, type, classProps);
+ }
+ }
+
+ /**
+ * Return all property names declared on this class and its base classes
+ */
+ function collectClassProps(type: spec.ClassType, into: Set<string>): Set<string> {
+ for (const prop of type.properties ?? []) {
+ into.add(prop.name);
+ }
+
+ if (type.base) {
+ const base = _dereference(type.base, assembly, validator);
+ if (spec.isClassType(base)) {
+ collectClassProps(base, into);
+ }
+ }
+
+ return into;
+ }
+
+ function checkInterfacePropsImplemented(interfaceFqn: string, cls: spec.ClassType, propNames: Set<string>) {
+ const intf = _dereference(interfaceFqn, assembly, validator);
+ if (!spec.isInterfaceType(intf)) {
+ return;
+ }
+
+ // We only have to check for optional properties, because anything required
+ // would have been caught by the TypeScript compiler already.
+ for (const prop of intf.properties ?? []) {
+ if (!prop.optional) {
+ continue;
+ }
+
+ if (!propNames.has(prop.name)) {
+ diagnostic(
+ JsiiDiagnostic.JSII_5021_ABSTRACT_CLASS_MISSING_PROP_IMPL.create(
+ bindings.getClassOrInterfaceRelatedNode(cls),
+ intf,
+ cls,
+ prop.name,
+ ).maybeAddRelatedInformation(bindings.getPropertyRelatedNode(prop), 'The implemented declaration is here.'),
+ );
+ }
+ }
+
+ for (const extFqn of intf.interfaces ?? []) {
+ checkInterfacePropsImplemented(extFqn, cls, propNames);
+ }
+ }
+ }
+
  function _staticMembersAndNestedTypesMustNotSharePascalCaseName(
  _: Validator,
  assembly: spec.Assembly,

test/__snapshots__/negatives.test.ts.snap

@@ -0,0 +1,6 @@
+export interface ISomeInterface {
+ readonly xyz?: string;
+}
+
+export abstract class SomeClass implements ISomeInterface {
+}