Skip to content

Commit

Permalink
fix(jsii-diff): does not check types in submodules (#3808)
Browse files Browse the repository at this point in the history
`jsii-diff` was ignoring types in submodules, meaning no compatibility checks were being done on CDK v2 at all anymore.



---

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
  • Loading branch information
rix0rrr authored Oct 20, 2022
1 parent 977e81b commit 12ea38e
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 14 deletions.
13 changes: 10 additions & 3 deletions packages/jsii-diff/lib/type-comparison.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ export class AssemblyComparison {
*/
public load(original: reflect.Assembly, updated: reflect.Assembly) {
/* eslint-disable prettier/prettier */
for (const [origClass, updatedClass] of this.typePairs(original.classes, updated)) {
for (const [origClass, updatedClass] of this.typePairs(original.allClasses, updated)) {
this.types.set(origClass.fqn, new ComparableClassType(this, origClass, updatedClass));
}

for (const [origIface, updatedIface] of this.typePairs(original.interfaces, updated)) {
for (const [origIface, updatedIface] of this.typePairs(original.allInterfaces, updated)) {
if (origIface.datatype !== updatedIface.datatype) {
this.mismatches.report({
ruleKey: 'iface-type',
Expand All @@ -83,7 +83,7 @@ export class AssemblyComparison {
: new ComparableInterfaceType(this, origIface, updatedIface));
}

for (const [origEnum, updatedEnum] of this.typePairs(original.enums, updated)) {
for (const [origEnum, updatedEnum] of this.typePairs(original.allEnums, updated)) {
this.types.set(origEnum.fqn, new ComparableEnumType(this, origEnum, updatedEnum));
}
/* eslint-enable prettier/prettier */
Expand All @@ -93,6 +93,7 @@ export class AssemblyComparison {
* Perform the comparison for all loaded types
*/
public compare() {
LOG.debug(`Comparing ${this.comparableTypes.length} types`);
this.comparableTypes.forEach((t) => t.markTypeRoles());
this.comparableTypes.forEach((t) => t.compare());
}
Expand Down Expand Up @@ -275,6 +276,8 @@ export abstract class ComparableReferenceType<
* Compare members of the reference types
*/
public compare() {
LOG.debug(`Reference type ${this.oldType.fqn}`);

validateStabilities(this.oldType, this.newType, this.mismatches);
validateBaseTypeAssignability(this.oldType, this.newType, this.mismatches);

Expand Down Expand Up @@ -476,6 +479,8 @@ export class ComparableInterfaceType extends ComparableReferenceType<reflect.Int
*/
export class ComparableStructType extends ComparableType<reflect.InterfaceType> {
public compare() {
LOG.debug(`Struct type ${this.oldType.fqn}`);

validateStabilities(this.oldType, this.newType, this.mismatches);
validateBaseTypeAssignability(this.oldType, this.newType, this.mismatches);
this.validateNoPropertiesRemoved();
Expand Down Expand Up @@ -576,6 +581,8 @@ export class ComparableEnumType extends ComparableType<reflect.EnumType> {
* Perform comparisons on enum members
*/
public compare() {
LOG.debug(`Enum type ${this.oldType.fqn}`);

validateStabilities(this.oldType, this.newType, this.mismatches);

validateExistingMembers(
Expand Down
17 changes: 17 additions & 0 deletions packages/jsii-diff/test/classes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -749,3 +749,20 @@ test.each([
`,
),
);

// ----------------------------------------------------------------------

test('will find mismatches in submodules', () =>
expectError(
/number is not assignable to string/,
{
'index.ts': 'export * as submodule from "./subdir"',
'subdir/index.ts':
'export class Foo { public static readonly PROP = "abc"; }',
},
{
'index.ts': 'export * as submodule from "./subdir"',
'subdir/index.ts':
'export class Foo { public static readonly PROP = 42; }',
},
));
16 changes: 11 additions & 5 deletions packages/jsii-diff/test/util.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { sourceToAssemblyHelper } from 'jsii';
import { MultipleSourceFiles, sourceToAssemblyHelper } from 'jsii';
import * as reflect from 'jsii-reflect';

import { compareAssemblies } from '../lib';
import { Mismatches } from '../lib/types';

export function expectNoError(original: string, updated: string) {
export function expectNoError(
original: string | MultipleSourceFiles,
updated: string | MultipleSourceFiles,
) {
const mms = compare(original, updated);
for (const msg of mms.messages()) {
console.error(`- ${msg}`);
Expand All @@ -14,8 +17,8 @@ export function expectNoError(original: string, updated: string) {

export function expectError(
error: RegExp | undefined,
original: string,
updated: string,
original: string | MultipleSourceFiles,
updated: string | MultipleSourceFiles,
) {
if (error == null) {
expectNoError(original, updated);
Expand All @@ -32,7 +35,10 @@ export function expectError(
}
}

export function compare(original: string, updated: string): Mismatches {
export function compare(
original: string | MultipleSourceFiles,
updated: string | MultipleSourceFiles,
): Mismatches {
const ass1 = sourceToAssemblyHelper(original);
const ts1 = new reflect.TypeSystem();
const originalAssembly = ts1.addAssembly(new reflect.Assembly(ts1, ass1));
Expand Down
32 changes: 26 additions & 6 deletions packages/jsii-reflect/lib/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,37 @@ export class Assembly extends ModuleLike {
}

/**
* All types, even those in submodules and nested submodules.
* All types in the assembly and all of its submodules
*/
public get types(): readonly Type[] {
return Array.from(this.typeMap.values());
public get allTypes(): readonly Type[] {
return [...this.types, ...this.allSubmodules.flatMap((s) => s.types)];
}

/**
* Return all types in the current assembly/submodule and all submodules underneath
* All classes in the assembly and all of its submodules
*/
public get allTypes(): readonly Type[] {
return [...this.types, ...this.allSubmodules.flatMap((s) => s.types)];
public get allClasses(): readonly ClassType[] {
return this.allTypes
.filter((t) => t instanceof ClassType)
.map((t) => t as ClassType);
}

/**
* All interfaces in the assembly and all of its submodules
*/
public get allInterfaces(): readonly InterfaceType[] {
return this.allTypes
.filter((t) => t instanceof InterfaceType)
.map((t) => t as InterfaceType);
}

/**
* All interfaces in the assembly and all of its submodules
*/
public get allEnums(): readonly EnumType[] {
return this.allTypes
.filter((t) => t instanceof EnumType)
.map((t) => t as EnumType);
}

public findType(fqn: string) {
Expand Down
12 changes: 12 additions & 0 deletions packages/jsii-reflect/lib/module-like.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,34 @@ export abstract class ModuleLike {
return Array.from(this.submoduleMap.values());
}

/**
* All types in this module/namespace (not submodules)
*/
public get types(): readonly Type[] {
return Array.from(this.typeMap.values());
}

/**
* All classes in this module/namespace (not submodules)
*/
public get classes(): readonly ClassType[] {
return this.types
.filter((t) => t instanceof ClassType)
.map((t) => t as ClassType);
}

/**
* All interfaces in this module/namespace (not submodules)
*/
public get interfaces(): readonly InterfaceType[] {
return this.types
.filter((t) => t instanceof InterfaceType)
.map((t) => t as InterfaceType);
}

/**
* All enums in this module/namespace (not submodules)
*/
public get enums(): readonly EnumType[] {
return this.types
.filter((t) => t instanceof EnumType)
Expand Down

0 comments on commit 12ea38e

Please sign in to comment.