diff --git a/packages/jsii-diff/README.md b/packages/jsii-diff/README.md index f1417856c8..b713c5e321 100644 --- a/packages/jsii-diff/README.md +++ b/packages/jsii-diff/README.md @@ -126,6 +126,45 @@ abstract members yet. for subclassing, but treating them as such would limit the evolvability of libraries too much. +## Accepting breaking changes + +Sometimes you want to move forward with a change, even if it would technically +be a breaking change to your API. In order to do that, run `jsii-diff` +with the flag `--keys`. It will then print an identifier for every violation +between square brackets, for example: + +``` +$ jsii-diff --keys +IFACE pkg.MyStruct: formerly required property 'prop' is optional: returned from pkg.IConsumer.someMethod [weakened:pkg.MyStruct] +``` + +To accept a breaking finding, put the key (in this example `weakened:pkg.MyStruct`) +into a text file, for example `allowed-breaking-changes.txt`, and pass it to +`jsii-diff` as an ignore file: + +``` +$ jsii-diff --keys --ignore-file allowed-breaking-changes.txt +(no error) +``` + +### Moving/renaming API elements + +If you've moved API elements around between versions of your library, you can +put a special ignore marker starting with `move:` into your `--ignore-file`. To +separate the old and new class names, you can use `:`, `,` or whitespace. + +For example: + +``` +move:package.OldClassName package.NewClassName +move:package.OldClassName:package.NewClassName +move:package.OldClassName, package.NewClassName +``` + +Moving API elements is always breaking, but using this feature you can confirm +that you at least didn't break anything in the API surface of the moved classes +themselves. + ## Help! jsii-diff is marking my changes as breaking See [BREAKING_CHANGES.md](./BREAKING_CHANGES.md) for more information. diff --git a/packages/jsii-diff/bin/jsii-diff.ts b/packages/jsii-diff/bin/jsii-diff.ts index 1a71c77bfc..73c6e2be9a 100644 --- a/packages/jsii-diff/bin/jsii-diff.ts +++ b/packages/jsii-diff/bin/jsii-diff.ts @@ -124,9 +124,15 @@ async function main(): Promise { ); } + const allowedBreakingChanges = await loadFilter(argv['ignore-file']); + const fqnRemapping: Record = extractFqnRemappings( + allowedBreakingChanges, + ); + LOG.info('Starting analysis'); const mismatches = compareAssemblies(original, updated, { defaultExperimental: argv['default-stability'] === 'experimental', + fqnRemapping, }); LOG.info(`Found ${mismatches.count} issues`); @@ -135,7 +141,7 @@ async function main(): Promise { const diags = classifyDiagnostics( mismatches, treatAsError(argv['error-on'] as ErrorClass, argv['experimental-errors']), - await loadFilter(argv['ignore-file']), + allowedBreakingChanges, ); process.stderr.write( @@ -155,6 +161,43 @@ async function main(): Promise { return 0; } +/** + * Extract all lines that start with `move:` from the given string set + * + * Interpret them as `move:OLDFQN NEWFQN`, mapping moved FQNs. + * + * Separator can be any of `:`, comma or whitespace. + * + * Modifies the input set in-place. + */ +function extractFqnRemappings( + allowedBreakingChanges: Set, +): Record { + const ret: Record = {}; + + for (const line of Array.from(allowedBreakingChanges)) { + const prefix = 'move:'; + if (!line.startsWith(prefix)) { + continue; + } + + const parts = line + .slice(prefix.length) + .trim() + .split(/[:, \t]+/g); + if (parts.length !== 2) { + throw new Error( + `Invalid moved FQN declaration: ${line}. Expected format is 'move:old:new'`, + ); + } + const [oldFqn, newFqn] = parts; + ret[oldFqn] = newFqn; + allowedBreakingChanges.delete(line); + } + + return ret; +} + // Allow both npm: (legacy) and npm:// (looks better) const NPM_REGEX = /^npm:(\/\/)?/; @@ -311,7 +354,7 @@ async function loadFilter(filterFilename?: string): Promise> { (await fs.readFile(filterFilename, { encoding: 'utf-8' })) .split('\n') .map((x) => x.trim()) - .filter((x) => !x.startsWith('#')), + .filter((x) => x && !x.startsWith('#')), ); } catch (e: any) { if (e.code !== 'ENOENT') { diff --git a/packages/jsii-diff/lib/type-analysis.ts b/packages/jsii-diff/lib/type-analysis.ts index 79b710fcb9..a4ce9af1f0 100644 --- a/packages/jsii-diff/lib/type-analysis.ts +++ b/packages/jsii-diff/lib/type-analysis.ts @@ -3,234 +3,238 @@ import * as reflect from 'jsii-reflect'; import { flatMap } from './util'; -/** - * Check whether A is a supertype of B - * - * Put differently: whether any value of type B would be assignable to a - * variable of type A. - * - * We always check the relationship in the NEW (latest, updated) typesystem. - */ -export function isSuperType( - a: reflect.TypeReference, - b: reflect.TypeReference, - updatedSystem: reflect.TypeSystem, -): Analysis { - if (a.void || b.void) { - throw new Error('isSuperType() does not handle voids'); - } - if (a.isAny) { - return { success: true }; - } - - if (a.primitive !== undefined) { - if (a.primitive === b.primitive) { +export class TypeAnalysis { + public constructor( + private readonly updatedSystem: reflect.TypeSystem, + private readonly fqnRemapping: Record = {}, + ) {} + + /** + * Check whether A is a supertype of B + * + * Put differently: whether any value of type B would be assignable to a + * variable of type A. + * + * We always check the relationship in the NEW (latest, updated) typesystem. + */ + public isSuperType( + a: reflect.TypeReference, + b: reflect.TypeReference, + ): Analysis { + if (a.void || b.void) { + throw new Error('isSuperType() does not handle voids'); + } + if (a.isAny) { return { success: true }; } - return failure(`${b.toString()} is not assignable to ${a.toString()}`); - } - if (a.arrayOfType !== undefined) { - // Arrays are covariant - if (b.arrayOfType === undefined) { - return failure(`${b.toString()} is not an array type`); + if (a.primitive !== undefined) { + if (a.primitive === b.primitive) { + return { success: true }; + } + return failure(`${b.toString()} is not assignable to ${a.toString()}`); } - return prependReason( - isSuperType(a.arrayOfType, b.arrayOfType, updatedSystem), - `${b.toString()} is not assignable to ${a.toString()}`, - ); - } - if (a.mapOfType !== undefined) { - // Maps are covariant (are they?) - if (b.mapOfType === undefined) { - return failure(`${b.toString()} is not a map type`); + if (a.arrayOfType !== undefined) { + // Arrays are covariant + if (b.arrayOfType === undefined) { + return failure(`${b.toString()} is not an array type`); + } + return prependReason( + this.isSuperType(a.arrayOfType, b.arrayOfType), + `${b.toString()} is not assignable to ${a.toString()}`, + ); } - return prependReason( - isSuperType(a.mapOfType, b.mapOfType, updatedSystem), - `${b.toString()} is not assignable to ${a.toString()}`, - ); - } - // Every element of B can be assigned to A - if (b.unionOfTypes !== undefined) { - const analyses = b.unionOfTypes.map((bbb) => - isSuperType(a, bbb, updatedSystem), - ); - if (analyses.every((x) => x.success)) { - return { success: true }; - } - return failure( - `some of ${b.toString()} are not assignable to ${a.toString()}`, - ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), - ); - } - // There should be an element of A which can accept all of B - if (a.unionOfTypes !== undefined) { - const analyses = a.unionOfTypes.map((aaa) => - isSuperType(aaa, b, updatedSystem), - ); - if (analyses.some((x) => x.success)) { - return { success: true }; + if (a.mapOfType !== undefined) { + // Maps are covariant (are they?) + if (b.mapOfType === undefined) { + return failure(`${b.toString()} is not a map type`); + } + return prependReason( + this.isSuperType(a.mapOfType, b.mapOfType), + `${b.toString()} is not assignable to ${a.toString()}`, + ); } - return failure( - `none of ${b.toString()} are assignable to ${a.toString()}`, - ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), - ); - } - // intersection A ⊒ B <=> all of the elements of A ⊒ B - if (a.intersectionOfTypes !== undefined) { - const analyses = a.intersectionOfTypes.map((aaa) => - isSuperType(aaa, b, updatedSystem), - ); - if (analyses.every((x) => x.success)) { - return { success: true }; + // Every element of B can be assigned to A + if (b.unionOfTypes !== undefined) { + const analyses = b.unionOfTypes.map((bbb) => this.isSuperType(a, bbb)); + if (analyses.every((x) => x.success)) { + return { success: true }; + } + return failure( + `some of ${b.toString()} are not assignable to ${a.toString()}`, + ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), + ); + } + // There should be an element of A which can accept all of B + if (a.unionOfTypes !== undefined) { + const analyses = a.unionOfTypes.map((aaa) => this.isSuperType(aaa, b)); + if (analyses.some((x) => x.success)) { + return { success: true }; + } + return failure( + `none of ${b.toString()} are assignable to ${a.toString()}`, + ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), + ); } - return failure( - `${b.toString()} is not assignable to all of ${a.toString()}`, - ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), - ); - } - // A ⊒ intersection B <=> A ⊒ any of the elements of B - if (b.intersectionOfTypes !== undefined) { - const analyses = b.intersectionOfTypes.map((bbb) => - isSuperType(a, bbb, updatedSystem), - ); - if (analyses.some((x) => x.success)) { - return { success: true }; + // intersection A ⊒ B <=> all of the elements of A ⊒ B + if (a.intersectionOfTypes !== undefined) { + const analyses = a.intersectionOfTypes.map((aaa) => + this.isSuperType(aaa, b), + ); + if (analyses.every((x) => x.success)) { + return { success: true }; + } + return failure( + `${b.toString()} is not assignable to all of ${a.toString()}`, + ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), + ); } - return failure( - `some of ${b.toString()} are not assignable to ${a.toString()}`, - ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), - ); - } - // We have two named types, recursion might happen so protect against it. - try { - // For named types, we'll always do a nominal typing relationship. - // That is, if in the updated typesystem someone were to use the type name - // from the old assembly, do they have a typing relationship that's accepted - // by a nominal type system. (That check also rules out enums) - const nominalCheck = isNominalSuperType(a, b, updatedSystem); - if (nominalCheck.success === false) { - return nominalCheck; + // A ⊒ intersection B <=> A ⊒ any of the elements of B + if (b.intersectionOfTypes !== undefined) { + const analyses = b.intersectionOfTypes.map((bbb) => + this.isSuperType(a, bbb), + ); + if (analyses.some((x) => x.success)) { + return { success: true }; + } + return failure( + `some of ${b.toString()} are not assignable to ${a.toString()}`, + ...flatMap(analyses, (x) => (x.success ? [] : x.reasons)), + ); } - // At this point, the only thing left to do is recurse into the structs. - // We used to do that here, but we don't anymore; structs check themselves - // for structural weakening/strengthening. - return { success: true }; - } catch (e: any) { - return failure(e.message); - } -} + // We have two named types, recursion might happen so protect against it. + try { + // For named types, we'll always do a nominal typing relationship. + // That is, if in the updated typesystem someone were to use the type name + // from the old assembly, do they have a typing relationship that's accepted + // by a nominal type system. (That check also rules out enums) + const nominalCheck = this.isNominalSuperType(a, b); + if (nominalCheck.success === false) { + return nominalCheck; + } -/** - * Find types A and B in the updated type system, and check whether they have a supertype relationship in the type system - */ -export function isNominalSuperType( - a: reflect.TypeReference, - b: reflect.TypeReference, - updatedSystem: reflect.TypeSystem, -): Analysis { - if (a.fqn === undefined) { - throw new Error(`I was expecting a named type, got '${a.toString()}'`); + // At this point, the only thing left to do is recurse into the structs. + // We used to do that here, but we don't anymore; structs check themselves + // for structural weakening/strengthening. + return { success: true }; + } catch (e: any) { + return failure(e.message); + } } - // Named type vs a non-named type - if (b.fqn === undefined) { - return failure(`${b.toString()} is not assignable to ${a.toString()}`); - } + /** + * Find types A and B in the updated type system, and check whether they have a supertype relationship in the type system + */ + public isNominalSuperType( + a: reflect.TypeReference, + b: reflect.TypeReference, + ): Analysis { + if (a.fqn === undefined) { + throw new Error(`I was expecting a named type, got '${a.toString()}'`); + } - // Short-circuit of the types are the same name, saves us some lookup - if (a.fqn === b.fqn) { - return { success: true }; - } + // Named type vs a non-named type + if (b.fqn === undefined) { + return failure(`${b.toString()} is not assignable to ${a.toString()}`); + } - // We now need to do subtype analysis on the - // Find A in B's typesystem, and see if B is a subtype of A' - const B = updatedSystem.tryFindFqn(b.fqn); - const A = updatedSystem.tryFindFqn(a.fqn); + // Short-circuit of the types are the same name, saves us some lookup + if (a.fqn === b.fqn) { + return { success: true }; + } - if (!B) { - return failure(`could not find type ${b.toString()}`); - } - if (!A) { - return failure(`could not find type ${a.toString()}`); - } + // We now need to do subtype analysis on the + // Find A in B's typesystem, and see if B is a subtype of A' + const B = this.updatedSystem.tryFindFqn(this.fqnRemapping[b.fqn] ?? b.fqn); + const A = this.updatedSystem.tryFindFqn(this.fqnRemapping[a.fqn] ?? a.fqn); - // If they're enums, they should have been exactly the same (tested above) - // enums are never subtypes of any other type. - if (A.isEnumType()) { - return failure(`${a.toString()} is an enum different from ${b.toString()}`); - } - if (B.isEnumType()) { - return failure(`${b.toString()} is an enum different from ${a.toString()}`); - } + if (!B) { + return failure(`could not find type ${b.toString()}`); + } + if (!A) { + return failure(`could not find type ${a.toString()}`); + } - if (B.extends(A)) { - return { success: true }; - } - return failure(`${b.toString()} does not extend ${a.toString()}`); -} + // If they're enums, they should have been exactly the same (tested above) + // enums are never subtypes of any other type. + if (A.isEnumType()) { + return failure( + `${a.toString()} is an enum different from ${b.toString()}`, + ); + } + if (B.isEnumType()) { + return failure( + `${b.toString()} is an enum different from ${a.toString()}`, + ); + } -/** - * Is struct A a structural supertype of struct B? - * - * Trying to answer the question, is this assignment legal for all values - * of `expr in B`. - * - * ```ts - * const var: A = expr as B; - * ``` - * - * A is a structural supertype of B if all required members of A are also - * required in B, and of a compatible type. - * - * Nullable members of A must either not exist in B, or be of a compatible - * type. - */ -export function isStructuralSuperType( - a: reflect.InterfaceType, - b: reflect.InterfaceType, - updatedSystem: reflect.TypeSystem, -): Analysis { - // We know all members can only be properties, so that makes it easier. - const bProps = b.getProperties(true); - - // Use timing words in error message to make it more understandable - const formerly = b.system === updatedSystem ? 'formerly' : 'newly'; - const is = b.system === updatedSystem ? 'is' : 'used to be'; - const removed = b.system === updatedSystem ? 'removed' : 'added'; - - for (const [name, aProp] of Object.entries(a.getProperties(true))) { - const bProp = bProps[name]; - - if (aProp.optional) { - // Optional field, only requirement is that IF it exists, the type must match. - if (!bProp) { - continue; - } - } else { - if (!bProp) { - return failure(`${formerly} required property '${name}' ${removed}`); + if (B.extends(A)) { + return { success: true }; + } + return failure(`${b.toString()} does not extend ${a.toString()}`); + } + + /** + * Is struct A a structural supertype of struct B? + * + * Trying to answer the question, is this assignment legal for all values + * of `expr in B`. + * + * ```ts + * const var: A = expr as B; + * ``` + * + * A is a structural supertype of B if all required members of A are also + * required in B, and of a compatible type. + * + * Nullable members of A must either not exist in B, or be of a compatible + * type. + */ + public isStructuralSuperType( + a: reflect.InterfaceType, + b: reflect.InterfaceType, + ): Analysis { + // We know all members can only be properties, so that makes it easier. + const bProps = b.getProperties(true); + + // Use timing words in error message to make it more understandable + const formerly = b.system === this.updatedSystem ? 'formerly' : 'newly'; + const is = b.system === this.updatedSystem ? 'is' : 'used to be'; + const removed = b.system === this.updatedSystem ? 'removed' : 'added'; + + for (const [name, aProp] of Object.entries(a.getProperties(true))) { + const bProp = bProps[name]; + + if (aProp.optional) { + // Optional field, only requirement is that IF it exists, the type must match. + if (!bProp) { + continue; + } + } else { + if (!bProp) { + return failure(`${formerly} required property '${name}' ${removed}`); + } + if (bProp.optional) { + return failure( + `${formerly} required property '${name}' ${is} optional`, + ); + } } - if (bProp.optional) { - return failure( - `${formerly} required property '${name}' ${is} optional`, - ); + + const ana = this.isSuperType(aProp.type, bProp.type); + if (!ana.success) { + return failure(`property ${name}`, ...ana.reasons); } } - const ana = isSuperType(aProp.type, bProp.type, updatedSystem); - if (!ana.success) { - return failure(`property ${name}`, ...ana.reasons); - } + return { success: true }; } - - return { success: true }; } // Oh tagged union types I love you so much! diff --git a/packages/jsii-diff/lib/type-comparison.ts b/packages/jsii-diff/lib/type-comparison.ts index 8986bc1f30..2d1bd7cce2 100644 --- a/packages/jsii-diff/lib/type-comparison.ts +++ b/packages/jsii-diff/lib/type-comparison.ts @@ -3,7 +3,7 @@ import * as reflect from 'jsii-reflect'; import * as log4js from 'log4js'; import { validateStabilities } from './stability'; -import { isStructuralSuperType, Analysis } from './type-analysis'; +import { TypeAnalysis, Analysis } from './type-analysis'; import { describeInterfaceType, describeType, @@ -63,7 +63,8 @@ export class AssemblyComparison { public load(original: reflect.Assembly, updated: reflect.Assembly) { /* eslint-disable prettier/prettier */ for (const [origClass, updatedClass] of this.typePairs(original.allClasses, updated)) { - this.types.set(origClass.fqn, new ComparableClassType(this, origClass, updatedClass)); + const { fqn, displayFqn } = this.resolveFqn(origClass); + this.types.set(fqn, new ComparableClassType(this, origClass, updatedClass, displayFqn)); } for (const [origIface, updatedIface] of this.typePairs(original.allInterfaces, updated)) { @@ -78,13 +79,15 @@ export class AssemblyComparison { continue; } - this.types.set(origIface.fqn, origIface.datatype - ? new ComparableStructType(this, origIface, updatedIface) - : new ComparableInterfaceType(this, origIface, updatedIface)); + const { fqn, displayFqn } = this.resolveFqn(origIface); + this.types.set(fqn, origIface.datatype + ? new ComparableStructType(this, origIface, updatedIface, displayFqn) + : new ComparableInterfaceType(this, origIface, updatedIface, displayFqn)); } for (const [origEnum, updatedEnum] of this.typePairs(original.allEnums, updated)) { - this.types.set(origEnum.fqn, new ComparableEnumType(this, origEnum, updatedEnum)); + const { fqn, displayFqn } = this.resolveFqn(origEnum); + this.types.set(fqn, new ComparableEnumType(this, origEnum, updatedEnum, displayFqn)); } /* eslint-enable prettier/prettier */ } @@ -105,7 +108,7 @@ export class AssemblyComparison { const ret = new Array>(); for (const fqn of fqnsFrom(ref)) { - const t = this.types.get(fqn); + const t = this.types.get(this.resolveFqn(fqn).fqn); if (t) { ret.push(t); } @@ -113,6 +116,21 @@ export class AssemblyComparison { return ret; } + /** + * Return the type's FQN, running it through the translation table if present. + */ + private resolveFqn(x: string | reflect.Type): { + fqn: string; + displayFqn: string; + } { + const fqn = typeof x === 'string' ? x : x.fqn; + const finalFqn = this.options.fqnRemapping?.[fqn] ?? fqn; + if (fqn !== finalFqn) { + return { fqn: finalFqn, displayFqn: `${fqn} -> ${finalFqn}` }; + } + return { fqn, displayFqn: fqn }; + } + /** * All ComparableType<>s */ @@ -128,9 +146,10 @@ export class AssemblyComparison { updatedAssembly: reflect.Assembly, ): IterableIterator<[T, T]> { for (const origType of xs) { - LOG.trace(origType.fqn); + const { fqn, displayFqn } = this.resolveFqn(origType); + LOG.trace(displayFqn); - const updatedType = updatedAssembly.tryFindType(origType.fqn); + const updatedType = updatedAssembly.tryFindType(fqn); if (!updatedType) { this.mismatches.report({ ruleKey: 'removed', @@ -171,8 +190,13 @@ export abstract class ComparableType { protected readonly assemblyComparison: AssemblyComparison, protected readonly oldType: T, protected readonly newType: T, + protected readonly displayFqn: string, ) {} + public get fqnRemapping(): Record { + return this.assemblyComparison.options.fqnRemapping ?? {}; + } + /** * Does this type occur in an input role? */ @@ -276,10 +300,15 @@ export abstract class ComparableReferenceType< * Compare members of the reference types */ public compare() { - LOG.debug(`Reference type ${this.oldType.fqn}`); + LOG.debug(`Reference type ${this.displayFqn}`); validateStabilities(this.oldType, this.newType, this.mismatches); - validateBaseTypeAssignability(this.oldType, this.newType, this.mismatches); + validateBaseTypeAssignability( + this.oldType, + this.newType, + this.fqnRemapping, + this.mismatches, + ); validateSubclassableNotRemoved(this.oldType, this.newType, this.mismatches); if (this.subclassableType) { @@ -332,7 +361,12 @@ export abstract class ComparableReferenceType< this.mismatches.withMotivation('type is @subclassable'), ); } else { - validateReturnTypeNotWeakened(original, updated, this.mismatches); + validateReturnTypeNotWeakened( + original, + updated, + this.fqnRemapping, + this.mismatches, + ); } this.validateCallable(original, updated); @@ -366,6 +400,7 @@ export abstract class ComparableReferenceType< original, oldParam, newParam, + this.fqnRemapping, this.mismatches, ); } @@ -419,7 +454,12 @@ export abstract class ComparableReferenceType< this.mismatches.withMotivation('mutable property cannot change type'), ); } else { - validatePropertyTypeNotWeakened(original, updated, this.mismatches); + validatePropertyTypeNotWeakened( + original, + updated, + this.fqnRemapping, + this.mismatches, + ); } } @@ -445,6 +485,7 @@ export class ComparableClassType extends ComparableReferenceType { public compare() { - LOG.debug(`Struct type ${this.oldType.fqn}`); + LOG.debug(`Struct type ${this.displayFqn}`); validateStabilities(this.oldType, this.newType, this.mismatches); - validateBaseTypeAssignability(this.oldType, this.newType, this.mismatches); + validateBaseTypeAssignability( + this.oldType, + this.newType, + this.fqnRemapping, + this.mismatches, + ); this.validateNoPropertiesRemoved(); if (this.inputType) { @@ -564,7 +610,10 @@ export class ComparableStructType extends ComparableType b: reflect.InterfaceType, ): Analysis { try { - return isStructuralSuperType(a, b, this.newType.system); + return new TypeAnalysis( + this.newType.system, + this.fqnRemapping, + ).isStructuralSuperType(a, b); } catch (e: any) { // We might get an exception if the type is supposed to come from a different // assembly and the lookup fails. @@ -581,7 +630,7 @@ export class ComparableEnumType extends ComparableType { * Perform comparisons on enum members */ public compare() { - LOG.debug(`Enum type ${this.oldType.fqn}`); + LOG.debug(`Enum type ${this.displayFqn}`); validateStabilities(this.oldType, this.newType, this.mismatches); diff --git a/packages/jsii-diff/lib/types.ts b/packages/jsii-diff/lib/types.ts index 09f35e9445..0d252c258b 100644 --- a/packages/jsii-diff/lib/types.ts +++ b/packages/jsii-diff/lib/types.ts @@ -8,6 +8,11 @@ export interface ComparisonOptions { * @default Treat as stable */ defaultExperimental?: boolean; + + /** + * Mapping old FQNs to new ones + */ + fqnRemapping?: Record; } export interface ComparisonContext extends ComparisonOptions { diff --git a/packages/jsii-diff/lib/validations.ts b/packages/jsii-diff/lib/validations.ts index ca74f27429..e631ccb5fa 100644 --- a/packages/jsii-diff/lib/validations.ts +++ b/packages/jsii-diff/lib/validations.ts @@ -2,12 +2,7 @@ import * as reflect from 'jsii-reflect'; import * as log4js from 'log4js'; import { validateStabilities } from './stability'; -import { - Analysis, - FailedAnalysis, - isSuperType, - isNominalSuperType, -} from './type-analysis'; +import { Analysis, FailedAnalysis, TypeAnalysis } from './type-analysis'; import { IReport } from './types'; const LOG = log4js.getLogger('jsii-diff'); @@ -26,9 +21,10 @@ const LOG = log4js.getLogger('jsii-diff'); export function validateBaseTypeAssignability( original: T, updated: T, + fqnRemapping: Record, mismatches: IReport, ) { - const ana = assignableToAllBaseTypes(original, updated); + const ana = assignableToAllBaseTypes(original, updated, fqnRemapping); if (!ana.success) { mismatches.report({ ruleKey: 'base-types', @@ -175,9 +171,14 @@ export function validateNoNewAbstractMembers( export function validateReturnTypeNotWeakened( original: reflect.Method, updated: reflect.Method, + fqnRemapping: Record, mismatches: IReport, ) { - const retAna = isCompatibleReturnType(original.returns, updated.returns); + const retAna = isCompatibleReturnType( + original.returns, + updated.returns, + fqnRemapping, + ); if (!retAna.success) { mismatches.report({ ruleKey: 'change-return-type', @@ -226,9 +227,10 @@ export function validateReturnTypeSame( export function validatePropertyTypeNotWeakened( original: reflect.Property, updated: reflect.Property, + fqnRemapping: Record, mismatches: IReport, ) { - const ana = isCompatibleReturnType(original, updated); + const ana = isCompatibleReturnType(original, updated, fqnRemapping); if (!ana.success) { mismatches.report({ ruleKey: 'changed-type', @@ -280,9 +282,14 @@ export function validateParameterTypeWeakened( method: reflect.Method | reflect.Initializer, original: reflect.Parameter, updated: reflect.Parameter, + fqnRemapping: Record, mismatches: IReport, ) { - const argAna = isCompatibleArgumentType(original.type, updated.type); + const argAna = isCompatibleArgumentType( + original.type, + updated.type, + fqnRemapping, + ); if (!argAna.success) { mismatches.report({ ruleKey: 'incompatible-argument', @@ -405,14 +412,19 @@ export function validateNoNewRequiredParams< export function validateMethodCompatible< T extends reflect.Method | reflect.Initializer, ->(original: T, updated: T, mismatches: IReport) { +>( + original: T, + updated: T, + fqnRemapping: Record, + mismatches: IReport, +) { validateStabilities(original, updated, mismatches); // Type guards on original are duplicated on updated to help tsc... They are required to be the same type by the declaration. if (reflect.isMethod(original) && reflect.isMethod(updated)) { validateStaticSame(original, updated, mismatches); validateAsyncSame(original, updated, mismatches); - validateReturnTypeNotWeakened(original, updated, mismatches); + validateReturnTypeNotWeakened(original, updated, fqnRemapping, mismatches); } validateNotMadeNonVariadic(original, updated, mismatches); @@ -423,7 +435,13 @@ export function validateMethodCompatible< updated, mismatches, (oldParam, newParam) => { - validateParameterTypeWeakened(original, oldParam, newParam, mismatches); + validateParameterTypeWeakened( + original, + oldParam, + newParam, + fqnRemapping, + mismatches, + ); }, ); @@ -532,6 +550,7 @@ export function* memberPairs< function isCompatibleReturnType( original: reflect.OptionalValue, updated: reflect.OptionalValue, + fqnRemapping: Record, ): Analysis { if (original.type.void) { return { success: true }; @@ -542,7 +561,10 @@ function isCompatibleReturnType( if (!original.optional && updated.optional) { return { success: false, reasons: ['output type is now optional'] }; } - return isSuperType(original.type, updated.type, updated.system); + return new TypeAnalysis(updated.system, fqnRemapping).isSuperType( + original.type, + updated.type, + ); } /** @@ -553,9 +575,13 @@ function isCompatibleReturnType( function isCompatibleArgumentType( original: reflect.TypeReference, updated: reflect.TypeReference, + fqnRemapping: Record, ): Analysis { // Input can never be void, so no need to check - return isSuperType(updated, original, updated.system); + return new TypeAnalysis(updated.system, fqnRemapping).isSuperType( + updated, + original, + ); } /** @@ -574,13 +600,13 @@ function isCompatibleArgumentType( function assignableToAllBaseTypes( original: reflect.ReferenceType, updated: reflect.ReferenceType, + fqnRemapping: Record, ): Analysis { for (const B of baseTypes(original)) { - const result = isNominalSuperType( - B.reference, - updated.reference, + const result = new TypeAnalysis( updated.system, - ); + fqnRemapping, + ).isNominalSuperType(B.reference, updated.reference); if (!result.success) { return result; } diff --git a/packages/jsii-diff/test/classes.test.ts b/packages/jsii-diff/test/classes.test.ts index c1e85722e9..2f9c5dd1d1 100644 --- a/packages/jsii-diff/test/classes.test.ts +++ b/packages/jsii-diff/test/classes.test.ts @@ -1,4 +1,4 @@ -import { expectError, expectNoError } from './util'; +import { compare, expectError, expectNoError } from './util'; // ---------------------------------------------------------------------- @@ -766,3 +766,16 @@ test('will find mismatches in submodules', () => 'export class Foo { public static readonly PROP = 42; }', }, )); + +// ---------------------------------------------------------------------- + +test('allow remapping of FQNs', () => { + const original = `export class Foo1 { }`; + const updated = `export class Foo2 { }`; + + // This should give no error because we remapped Foo1 to Foo2 + const mms = compare(original, updated, { + fqnRemapping: { 'testpkg.Foo1': 'testpkg.Foo2' }, + }); + expect(Array.from(mms.messages())).toEqual([]); +}); diff --git a/packages/jsii-diff/test/util.ts b/packages/jsii-diff/test/util.ts index 31a269c298..546361a64b 100644 --- a/packages/jsii-diff/test/util.ts +++ b/packages/jsii-diff/test/util.ts @@ -2,7 +2,7 @@ import { MultipleSourceFiles, sourceToAssemblyHelper } from 'jsii'; import * as reflect from 'jsii-reflect'; import { compareAssemblies } from '../lib'; -import { Mismatches } from '../lib/types'; +import { ComparisonOptions, Mismatches } from '../lib/types'; export function expectNoError( original: string | MultipleSourceFiles, @@ -38,6 +38,7 @@ export function expectError( export function compare( original: string | MultipleSourceFiles, updated: string | MultipleSourceFiles, + options: ComparisonOptions = {}, ): Mismatches { const ass1 = sourceToAssemblyHelper(original); const ts1 = new reflect.TypeSystem(); @@ -47,5 +48,5 @@ export function compare( const ts2 = new reflect.TypeSystem(); const updatedAssembly = ts2.addAssembly(new reflect.Assembly(ts2, ass2)); - return compareAssemblies(originalAssembly, updatedAssembly); + return compareAssemblies(originalAssembly, updatedAssembly, options); }