From b5037b9adf74c1978f1fd940921ce6c2050c297b Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Tue, 18 Jan 2022 12:23:51 +0100 Subject: [PATCH] fix(jsii): compiler allows inheriting interface-violating members (#3343) The compiler failed to check inherited members from a base class against interfaces declared on the inheriting class, so that it was possible to inherit members that changed the signature of the interface declarations they implemented (typically specializing them: required implementations of optional properties, etc...). While this is valid TypeScript (the implementation is allowed to be strictly more specific than the interface declaration), this is not allowed by jsii as this results in type models that cannot be represented in C# (and other languages that do not allow specialization by implementing or overriding members). In addition to fixing this issue, this change adds source bindings to diagnostics generated when property implementations are rejected, and fixes a logical error in the message for `JSII5010` (`immutable` and `mutable` were reversed so the message read the wrong way around). Fixes #3342 --- 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 --- packages/jsii/lib/jsii-diagnostic.ts | 34 ++++- packages/jsii/lib/validator.ts | 122 ++++++++++++++++-- .../test/__snapshots__/negatives.test.ts.snap | 103 +++++++++++++-- ...neg.inheritance-changes-types.from-base.ts | 15 +++ 4 files changed, 248 insertions(+), 26 deletions(-) create mode 100644 packages/jsii/test/negatives/neg.inheritance-changes-types.from-base.ts 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. +}