Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: omitting optional property fails C# compilation #1448

Merged
merged 5 commits into from
Oct 30, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: omitting optional property fails C# compilation
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;
}
```
  • Loading branch information
rix0rrr committed Oct 30, 2024
commit 574ab711d527e824c5be74da80391784a293db34
7 changes: 7 additions & 0 deletions src/jsii-diagnostic.ts
Original file line number Diff line number Diff line change
@@ -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 it as "public abstract" if the omission was on purpose.`,
mrgrain marked this conversation as resolved.
Show resolved Hide resolved
name: 'language-compatibility/abstract-class-missing-prop-impl',
});

//////////////////////////////////////////////////////////////////////////////
// 6000 => 6999 -- RESERVED

87 changes: 82 additions & 5 deletions src/validator.ts
Original file line number Diff line number Diff line change
@@ -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,
15 changes: 15 additions & 0 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 {
}
Loading