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

@@ -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

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<T extends spec.Property | spec.Method>(
+      type: spec.ClassType | spec.InterfaceType,
+      getter: (type: spec.ClassType | spec.InterfaceType) => T[] | undefined,
+    ): T[] {
+      const result = new Array<T>();
+      const known = new Set<string>();
+
+      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;
+}