diff --git a/src/jsii-diagnostic.ts b/src/jsii-diagnostic.ts index 77ad6873..bf9e6087 100644 --- a/src/jsii-diagnostic.ts +++ b/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 diff --git a/src/validator.ts b/src/validator.ts index 08dd3cce..e46fa624 100644 --- a/src/validator.ts +++ b/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): Set { + 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) { + 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, diff --git a/test/__snapshots__/negatives.test.ts.snap b/test/__snapshots__/negatives.test.ts.snap index df69fd64..2baf1524 100644 --- a/test/__snapshots__/negatives.test.ts.snap +++ b/test/__snapshots__/negatives.test.ts.snap @@ -106,7 +106,7 @@ neg.downgrade-to-readonly.ts:8:24 - error JSII5010: "jsii.Implementation#propert neg.downgrade-to-readonly.ts:4:5 4 property: string; ~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -296,7 +296,7 @@ neg.implementation-changes-types.4.ts:9:21 - error JSII5004: "jsii.SomethingImpl neg.implementation-changes-types.4.ts:5:14 5 something: Superclass; ~~~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -309,7 +309,7 @@ neg.implementation-changes-types.5.ts:14:21 - error JSII5004: "jsii.ISomethingEl neg.implementation-changes-types.5.ts:5:14 5 something: Superclass; ~~~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -337,7 +337,7 @@ neg.implementing-property-changes-optionality.ts:7:20 - error JSII5009: "jsii.Im neg.implementing-property-changes-optionality.ts:3:11 3 property?: string; ~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -350,7 +350,7 @@ neg.implementing-property-changes-optionality.1.ts:7:20 - error JSII5009: "jsii. neg.implementing-property-changes-optionality.1.ts:3:27 3 public abstract property?: string; ~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -363,7 +363,7 @@ neg.implementing-property-changes-optionality.2.ts:7:20 - error JSII5009: "jsii. neg.implementing-property-changes-optionality.2.ts:3:18 3 public property?: string = undefined; ~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -421,7 +421,7 @@ neg.inheritance-changes-types.4.ts:9:21 - error JSII5004: "jsii.SomethingSpecifi neg.inheritance-changes-types.4.ts:5:10 5 public something = new Superclass(); ~~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -434,7 +434,7 @@ neg.inheritance-changes-types.5.ts:14:21 - error JSII5004: "jsii.SomethingElse#s neg.inheritance-changes-types.5.ts:5:21 5 public something: Superclass = new Superclass(); ~~~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -447,7 +447,7 @@ neg.inheritance-changes-types.from-base.ts:6:30 - error JSII5009: "jsii.HasRequi neg.inheritance-changes-types.from-base.ts:2:28 2 readonly optionalProperty?: number; ~ - The implemented delcaration is here. + The implemented declaration is here. `; @@ -574,6 +574,21 @@ error JSII5001: Methods and properties cannot have names like "setXxx": those co `; +exports[`missing-abstract 1`] = ` +neg.missing-abstract.ts:5:1 - error JSII5021: A declaration of "ISomeInterface.xyz" is missing on class "SomeClass". Declare the property as "public abstract" if you want to defer it to subclasses. + +5 export abstract class SomeClass implements ISomeInterface { + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +6 } + ~ + + neg.missing-abstract.ts:2:3 + 2 readonly xyz?: string; + ~~~~~~~~~~~~~~~~~~~~~~ + The implemented declaration is here. + +`; + exports[`mix-datatype-and-arg-name 1`] = ` neg.mix-datatype-and-arg-name.ts:10:3 - error JSII5017: Parameter name "dontWorry" is also the name of a property in a struct parameter. Rename the positional parameter. @@ -667,7 +682,7 @@ neg.override-changes-visibility.ts:14:3 - error JSII5002: "jsii.ChildClass#prope neg.override-changes-visibility.ts:5:3 5 protected readonly property?: string; ~~~~~~~~~ - The implemented delcaration is here. + The implemented declaration is here. error JSII5002: "jsii.ChildClass#method" changes visibility to public when overriding jsii.BaseClass. Change it to protected `; diff --git a/test/negatives/neg.missing-abstract.ts b/test/negatives/neg.missing-abstract.ts new file mode 100644 index 00000000..7fa3a5c4 --- /dev/null +++ b/test/negatives/neg.missing-abstract.ts @@ -0,0 +1,6 @@ +export interface ISomeInterface { + readonly xyz?: string; +} + +export abstract class SomeClass implements ISomeInterface { +}