Skip to content

Commit

Permalink
fix: omitting optional property fails C# compilation (backport #1448) (
Browse files Browse the repository at this point in the history
…#1449)

# Backport

This will backport the following commits from `main` to
`maintenance/v5.3`:
- [fix: omitting optional property fails C# compilation
(#1448)](#1448)

<!--- Backport version: 9.5.1 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>
  • Loading branch information
aws-cdk-automation and rix0rrr authored Oct 30, 2024
1 parent d4c1e63 commit 09d956d
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 15 deletions.
7 changes: 7 additions & 0 deletions src/jsii-diagnostic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
87 changes: 82 additions & 5 deletions src/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -41,6 +40,7 @@ function _defaultValidations(): ValidationFunction[] {
_allTypeReferencesAreValid,
_inehritanceDoesNotChangeContracts,
_staticMembersAndNestedTypesMustNotSharePascalCaseName,
_abstractClassesMustImplementAllProperties,
];

function _enumMembersMustUserUpperSnakeCase(_: Validator, assembly: spec.Assembly, diagnostic: DiagnosticEmitter) {
Expand Down Expand Up @@ -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.',
),
);
}
Expand All @@ -396,7 +396,7 @@ function _defaultValidations(): ValidationFunction[] {
expected.type,
).maybeAddRelatedInformation(
expectedNode?.type ?? declarationName(expectedNode),
'The implemented delcaration is here.',
'The implemented declaration is here.',
),
);
}
Expand All @@ -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.',
),
);
}
Expand All @@ -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,
Expand Down
35 changes: 25 additions & 10 deletions test/__snapshots__/negatives.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions test/negatives/neg.missing-abstract.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface ISomeInterface {
readonly xyz?: string;
}

export abstract class SomeClass implements ISomeInterface {
}

0 comments on commit 09d956d

Please sign in to comment.