diff --git a/packages/jsii/lib/jsii-diagnostic.ts b/packages/jsii/lib/jsii-diagnostic.ts index c78b4ccfd2..67c6ff643e 100644 --- a/packages/jsii/lib/jsii-diagnostic.ts +++ b/packages/jsii/lib/jsii-diagnostic.ts @@ -523,7 +523,7 @@ export class JsiiDiagnostic implements ts.Diagnostic { newOptional = false, oldOptional = false, ) => - `"${newElement}" turns ${ + `"${newElement}" turns ${ newOptional ? 'optional' : 'required' } when ${action}. Make it ${oldOptional ? 'optional' : 'required'}`, name: 'language-compatibility/override-changes-prop-optional', @@ -534,12 +534,12 @@ export class JsiiDiagnostic implements ts.Diagnostic { formatter: ( newElement: string, action: string, - newMutable = false, - oldMutable = false, + newReadonly = false, + oldReadonly = false, ) => `"${newElement}" turns ${ - newMutable ? 'mutable' : 'readonly' - } when ${action}. Make it ${oldMutable ? 'mutable' : 'readonly'}`, + newReadonly ? 'readonly' : 'mutable' + } when ${action}. Make it ${oldReadonly ? 'readonly' : 'mutable'}`, name: 'language-compatibility/override-changes-mutability', }); @@ -862,6 +862,30 @@ export class JsiiDiagnostic implements ts.Diagnostic { return this; } + /** + * Adds related information to this `JsiiDiagnostic` instance if the provided + * `node` is defined. + * + * @param node the node to bind as related information, or `undefined`. + * @param message the message to attach to the related information. + * + * @returns `this` + */ + public maybeAddRelatedInformation( + node: ts.Node | undefined, + message: JsiiDiagnostic['messageText'], + ): this { + if (node == null) { + return this; + } + this.relatedInformation.push( + JsiiDiagnostic.JSII_9999_RELATED_INFO.create(node, message), + ); + // Clearing out #formatted, as this would no longer be the correct string. + this.#formatted = undefined; + return this; + } + /** * Formats this diagnostic with color and context if possible, and returns it. * The formatted diagnostic is cached, so that it can be re-used. This is diff --git a/packages/jsii/lib/validator.ts b/packages/jsii/lib/validator.ts index a357844780..2c478c687c 100644 --- a/packages/jsii/lib/validator.ts +++ b/packages/jsii/lib/validator.ts @@ -1,4 +1,5 @@ import * as spec from '@jsii/spec'; +import * as assert from 'assert'; import * as Case from 'case'; // eslint-disable-next-line @typescript-eslint/no-require-imports import deepEqual = require('deep-equal'); @@ -231,19 +232,55 @@ function _defaultValidations(): ValidationFunction[] { spec.isClassOrInterfaceType(type) && (type.interfaces?.length ?? 0) > 0 ) { - for (const method of type.methods ?? []) { - // Overrides "win" over implementations - if (method.overrides) { - continue; - } + for (const method of _allImplementations(type, (t) => t.methods)) { _validateMethodImplementation(method, type); } - for (const property of type.properties ?? []) { + for (const property of _allImplementations(type, (t) => t.properties)) { _validatePropertyImplementation(property, type); } } } + /** + * Lists all "implementations" from the given type, using the provided + * implementation getter. Note that abstract members may be part of the + * result (in particular, if `type` is an interface type, or if it's an + * abstract class with unimplemented members) -- I just couldn't come up + * with a name that actually describes this. + * + * @param type the type which implemented members are needed. + * @param getter the getter to obtain methods or properties from the type. + * + * @returns a list of members (possibly empty, always defined) + */ + function _allImplementations( + type: spec.ClassType | spec.InterfaceType, + getter: (type: spec.ClassType | spec.InterfaceType) => T[] | undefined, + ): T[] { + const result = new Array(); + const known = new Set(); + + for (const member of getter(type) ?? []) { + result.push(member); + known.add(member.name); + } + + if (spec.isClassType(type) && type.base) { + // We have a parent class, collect their concrete members, too (recursively)... + const base = _dereference(type.base, assembly, validator); + assert(base != null && spec.isClassType(base)); + for (const member of _allImplementations(base, getter)) { + if (known.has(member.name)) { + continue; + } + result.push(member); + known.add(member.name); + } + } + + return result; + } + function _validateMethodOverride( method: spec.Method, type: spec.ClassType, @@ -336,7 +373,9 @@ function _defaultValidations(): ValidationFunction[] { `${type.fqn}#${method.name}`, `implementing ${ifaceType.fqn}`, ); - method.overrides = iface; + // We won't replace a previous overrides declaration from a method override, as those have + // higher precedence than an initial implementation. + method.overrides = method.overrides ?? iface; return true; } if (_validateMethodImplementation(method, ifaceType)) { @@ -376,7 +415,9 @@ function _defaultValidations(): ValidationFunction[] { `${type.fqn}#${property.name}`, `implementing ${ifaceType.fqn}`, ); - property.overrides = ifaceType.fqn; + // We won't replace a previous overrides declaration from a property override, as those + // have higher precedence than an initial implementation. + property.overrides = property.overrides ?? ifaceType.fqn; return true; } if (_validatePropertyImplementation(property, ifaceType)) { @@ -472,45 +513,79 @@ function _defaultValidations(): ValidationFunction[] { label: string, action: string, ) { + const actualNode = bindings.getPropertyRelatedNode(actual); + const expectedNode = bindings.getPropertyRelatedNode(expected); if (!!expected.protected !== !!actual.protected) { const expVisibility = expected.protected ? 'protected' : 'public'; const actVisibility = actual.protected ? 'protected' : 'public'; diagnostic( - JsiiDiagnostic.JSII_5002_OVERRIDE_CHANGES_VISIBILITY.createDetached( + JsiiDiagnostic.JSII_5002_OVERRIDE_CHANGES_VISIBILITY.create( + actualNode?.modifiers?.find( + (mod) => + mod.kind === ts.SyntaxKind.PublicKeyword || + mod.kind === ts.SyntaxKind.ProtectedKeyword, + ) ?? declarationName(actualNode), label, action, actVisibility, expVisibility, + ).maybeAddRelatedInformation( + expectedNode?.modifiers?.find( + (mod) => + mod.kind === ts.SyntaxKind.PublicKeyword || + mod.kind === ts.SyntaxKind.ProtectedKeyword, + ) ?? declarationName(expectedNode), + 'The implemented delcaration is here.', ), ); } if (!deepEqual(expected.type, actual.type)) { diagnostic( - JsiiDiagnostic.JSII_5004_OVERRIDE_CHANGES_PROP_TYPE.createDetached( + JsiiDiagnostic.JSII_5004_OVERRIDE_CHANGES_PROP_TYPE.create( + actualNode?.type ?? declarationName(actualNode), label, action, actual.type, expected.type, + ).maybeAddRelatedInformation( + expectedNode?.type ?? declarationName(expectedNode), + 'The implemented delcaration is here.', ), ); } if (expected.immutable !== actual.immutable) { diagnostic( - JsiiDiagnostic.JSII_5010_OVERRIDE_CHANGES_MUTABILITY.createDetached( + JsiiDiagnostic.JSII_5010_OVERRIDE_CHANGES_MUTABILITY.create( + actualNode?.modifiers?.find( + (mod) => mod.kind === ts.SyntaxKind.ReadonlyKeyword, + ) ?? declarationName(actualNode), label, action, actual.immutable, expected.immutable, + ).maybeAddRelatedInformation( + expectedNode?.modifiers?.find( + (mod) => mod.kind === ts.SyntaxKind.ReadonlyKeyword, + ) ?? declarationName(expectedNode), + 'The implemented delcaration is here.', ), ); } if (expected.optional !== actual.optional) { diagnostic( - JsiiDiagnostic.JSII_5009_OVERRIDE_CHANGES_PROP_OPTIONAL.createDetached( + JsiiDiagnostic.JSII_5009_OVERRIDE_CHANGES_PROP_OPTIONAL.create( + actualNode?.questionToken ?? + actualNode?.type ?? + declarationName(actualNode), label, action, actual.optional, expected.optional, + ).maybeAddRelatedInformation( + expectedNode?.questionToken ?? + expectedNode?.type ?? + declarationName(expectedNode), + 'The implemented delcaration is here.', ), ); } @@ -725,3 +800,26 @@ function _isEmpty(array: undefined | any[]): array is undefined { function isConstantCase(x: string) { return !/[^A-Z0-9_]/.exec(x); } + +/** + * Obtains the name of the given declaration, if it has one, or returns the declaration itself. + * This function is meant to be used as a convenience to obtain the `ts.Node` to bind a + * `JsiiDianostic` instance on. + * + * It may return `undefined` but is typed as `ts.Node` so that it is easier to use with + * `JsiiDiagnostic` factories. + * + * @param decl the declaration which name is needed. + * + * @returns the name of the declaration if it has one, or the declaration itself. Might return + * `undefined` if the provided declaration is undefined. + */ +function declarationName( + decl: ts.Declaration | ts.Expression | undefined, +): ts.Node { + if (decl == null) { + // Pretend we returned a node - this is used to create diagnostics, worst case it'll be unbound. + return decl as any; + } + return ts.getNameOfDeclaration(decl) ?? decl; +} diff --git a/packages/jsii/test/__snapshots__/negatives.test.ts.snap b/packages/jsii/test/__snapshots__/negatives.test.ts.snap index 9eaa50f870..fd25017d8f 100644 --- a/packages/jsii/test/__snapshots__/negatives.test.ts.snap +++ b/packages/jsii/test/__snapshots__/negatives.test.ts.snap @@ -90,7 +90,15 @@ neg.double-interface-members-method.ts:4:1 - error JSII5015: Interface "jsii.IB" `; exports[`downgrade-to-readonly 1`] = ` -error JSII5010: "jsii.Implementation#property" turns mutable when implementing jsii.IInterface. Make it readonly +neg.downgrade-to-readonly.ts:8:24 - error JSII5010: "jsii.Implementation#property" turns readonly when implementing jsii.IInterface. Make it mutable + +8 constructor(public readonly property: string) {} + ~~~~~~~~ + + neg.downgrade-to-readonly.ts:4:5 + 4 property: string; + ~~~~~~~~ + The implemented delcaration is here. `; @@ -223,12 +231,28 @@ error JSII5006: "jsii.Something#takeSomething" changes the type of parameter "_a `; exports[`implementation-changes-types.4 1`] = ` -error JSII5004: "jsii.SomethingImpl#something" changes the property type to "jsii.Subclass" when implementing jsii.ISomething. Change it to "jsii.Superclass" +neg.implementation-changes-types.4.ts:9:21 - error JSII5004: "jsii.SomethingImpl#something" changes the property type to "jsii.Subclass" when implementing jsii.ISomething. Change it to "jsii.Superclass" + +9 public something: Subclass = new Subclass(); + ~~~~~~~~ + + neg.implementation-changes-types.4.ts:5:14 + 5 something: Superclass; + ~~~~~~~~~~ + The implemented delcaration is here. `; exports[`implementation-changes-types.5 1`] = ` -error JSII5004: "jsii.ISomethingElse#something" changes the property type to "jsii.Subclass" when implementing jsii.ISomething. Change it to "jsii.Superclass" +neg.implementation-changes-types.5.ts:14:21 - error JSII5004: "jsii.ISomethingElse#something" changes the property type to "jsii.Subclass" when implementing jsii.ISomething. Change it to "jsii.Superclass" + +14 public something: Subclass = new Subclass(); + ~~~~~~~~ + + neg.implementation-changes-types.5.ts:5:14 + 5 something: Superclass; + ~~~~~~~~~~ + The implemented delcaration is here. `; @@ -248,17 +272,41 @@ error JSII5008: "jsii.Implementor#method" turns parameter "_optional" required w `; exports[`implementing-property-changes-optionality 1`] = ` -error JSII5009: "jsii.Implementor#property" turns required when implementing jsii.IInterface. Make it optional +neg.implementing-property-changes-optionality.ts:7:20 - error JSII5009: "jsii.Implementor#property" turns required when implementing jsii.IInterface. Make it optional + +7 public property: string; + ~~~~~~ + + neg.implementing-property-changes-optionality.ts:3:11 + 3 property?: string; + ~ + The implemented delcaration is here. `; exports[`implementing-property-changes-optionality.1 1`] = ` -error JSII5009: "jsii.Implementor#property" turns required when overriding jsii.AbstractClass. Make it optional +neg.implementing-property-changes-optionality.1.ts:7:20 - error JSII5009: "jsii.Implementor#property" turns required when overriding jsii.AbstractClass. Make it optional + +7 public property: string; + ~~~~~~ + + neg.implementing-property-changes-optionality.1.ts:3:27 + 3 public abstract property?: string; + ~ + The implemented delcaration is here. `; exports[`implementing-property-changes-optionality.2 1`] = ` -error JSII5009: "jsii.Implementor#property" turns required when overriding jsii.ParentClass. Make it optional +neg.implementing-property-changes-optionality.2.ts:7:20 - error JSII5009: "jsii.Implementor#property" turns required when overriding jsii.ParentClass. Make it optional + +7 public property: string; + ~~~~~~ + + neg.implementing-property-changes-optionality.2.ts:3:18 + 3 public property?: string = undefined; + ~ + The implemented delcaration is here. `; @@ -294,12 +342,41 @@ error JSII5006: "jsii.SomethingSpecific#takeSomething" changes the type of param `; exports[`inheritance-changes-types.4 1`] = ` -error JSII5004: "jsii.SomethingSpecific#something" changes the property type to "jsii.Subclass" when overriding jsii.SomethingUnspecific. Change it to "jsii.Superclass" +neg.inheritance-changes-types.4.ts:9:21 - error JSII5004: "jsii.SomethingSpecific#something" changes the property type to "jsii.Subclass" when overriding jsii.SomethingUnspecific. Change it to "jsii.Superclass" + +9 public something: Subclass = new Subclass(); + ~~~~~~~~ + + neg.inheritance-changes-types.4.ts:5:10 + 5 public something = new Superclass(); + ~~~~~~~~~ + The implemented delcaration is here. `; exports[`inheritance-changes-types.5 1`] = ` -error JSII5004: "jsii.SomethingElse#something" changes the property type to "jsii.Subclass" when overriding jsii.SomethingBase. Change it to "jsii.Superclass" +neg.inheritance-changes-types.5.ts:14:21 - error JSII5004: "jsii.SomethingElse#something" changes the property type to "jsii.Subclass" when overriding jsii.SomethingBase. Change it to "jsii.Superclass" + +14 public something: Subclass = new Subclass(); + ~~~~~~~~ + + neg.inheritance-changes-types.5.ts:5:21 + 5 public something: Superclass = new Superclass(); + ~~~~~~~~~~ + The implemented delcaration is here. + +`; + +exports[`inheritance-changes-types.from-base 1`] = ` +neg.inheritance-changes-types.from-base.ts:6:30 - error JSII5009: "jsii.HasRequiredProperty#optionalProperty" turns required when implementing jsii.IHasOptionalProperty. Make it optional + +6 readonly optionalProperty: number; // Does not implement IHasOptionalProperty.optionalProperty + ~~~~~~ + + neg.inheritance-changes-types.from-base.ts:2:28 + 2 readonly optionalProperty?: number; + ~ + The implemented delcaration is here. `; @@ -527,8 +604,16 @@ neg.omit.4.ts:8:7 - error JSII1003: Only string-indexed map types are supported `; exports[`override-changes-visibility 1`] = ` +neg.override-changes-visibility.ts:14:3 - error JSII5002: "jsii.ChildClass#property" changes visibility to public when overriding jsii.BaseClass. Change it to protected + +14 public readonly property?: string; + ~~~~~~ + + neg.override-changes-visibility.ts:5:3 + 5 protected readonly property?: string; + ~~~~~~~~~ + The implemented delcaration is here. error JSII5002: "jsii.ChildClass#method" changes visibility to public when overriding jsii.BaseClass. Change it to protected -error JSII5002: "jsii.ChildClass#property" changes visibility to public when overriding jsii.BaseClass. Change it to protected `; diff --git a/packages/jsii/test/negatives/neg.inheritance-changes-types.from-base.ts b/packages/jsii/test/negatives/neg.inheritance-changes-types.from-base.ts new file mode 100644 index 0000000000..266e392b10 --- /dev/null +++ b/packages/jsii/test/negatives/neg.inheritance-changes-types.from-base.ts @@ -0,0 +1,15 @@ +export interface IHasOptionalProperty { + readonly optionalProperty?: number; +} + +export abstract class HasRequiredPropertyBase { + readonly optionalProperty: number; // Does not implement IHasOptionalProperty.optionalProperty + + protected constructor(value: number) { + this.optionalProperty = value; + } +} + +export class HasRequiredProperty extends HasRequiredPropertyBase implements IHasOptionalProperty { + // Inherits optionalProperty from HasRequiredPropertyBase, but does not implement IHasOptionalProperty correctly. +}