From 9c37f08f24f719332b9d47b7f2fd5a00ffbcfd9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 17 Apr 2020 14:51:10 +0200 Subject: [PATCH 1/5] feat(spec): model submodules in the Assembly schema Adds a `submodules` property on the `Assembly` structure, and carry it forward in the `dependencyClosure`, so the information can later be used to improve code generation for languages such as Python where the submodule structure is modeled as first-class entity that needs to be propertly dealt with. It can also help with properly adjusting the submodule names so they look more "native" in target languages, without facing problems when trying to generate type names in dependent packages. The forwarding of dependent submodules is not exercized in the current regression test suite because of a pair of other bugs (#1528, #1557) that need to be addressed before the generated Python code can successfully load. The last of those PRs to be merged will incldude the necessary test coverage. --- packages/@jsii/spec/lib/assembly.ts | 18 +++++ packages/jsii-calc/test/assembly.jsii | 13 +++- packages/jsii/lib/assembler.ts | 99 ++++++++++++++++++++++----- 3 files changed, 111 insertions(+), 19 deletions(-) diff --git a/packages/@jsii/spec/lib/assembly.ts b/packages/@jsii/spec/lib/assembly.ts index 779fff7d16..6685d52a1d 100644 --- a/packages/@jsii/spec/lib/assembly.ts +++ b/packages/@jsii/spec/lib/assembly.ts @@ -149,6 +149,23 @@ export interface Assembly extends AssemblyConfiguration, Documentable { * Shareable configuration of a jsii Assembly. */ export interface AssemblyConfiguration { + /** + * Submodules declared in this assembly. + * + * @default none + */ + submodules?: { + [fqn: string]: { + /** + * The specific code generation configuration for this submodule, if it + * differs from it's parent. + * + * @default - the parent submodule or assembly's configuration. + */ + targets?: AssemblyTargets; + }; + }; + /** * A map of target name to configuration, which is used when generating * packages for various languages. @@ -158,6 +175,7 @@ export interface AssemblyConfiguration { targets?: AssemblyTargets; } + /** * Versions of the JSII Assembly Specification. */ diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii index 11f2d2e72e..dcdf7ebe4c 100644 --- a/packages/jsii-calc/test/assembly.jsii +++ b/packages/jsii-calc/test/assembly.jsii @@ -137,6 +137,17 @@ "url": "https://github.com/aws/jsii.git" }, "schema": "jsii/0.10.0", + "submodules": { + "jsii-calc.DerivedClassHasNoProperties": {}, + "jsii-calc.InterfaceInNamespaceIncludesClasses": {}, + "jsii-calc.InterfaceInNamespaceOnlyInterface": {}, + "jsii-calc.composition": {}, + "jsii-calc.submodule": {}, + "jsii-calc.submodule.back_references": {}, + "jsii-calc.submodule.child": {}, + "jsii-calc.submodule.nested_submodule": {}, + "jsii-calc.submodule.nested_submodule.deeplyNested": {} + }, "targets": { "dotnet": { "iconUrl": "https://sdk-for-net.amazonwebservices.com/images/AWSLogo128x128.png", @@ -12442,5 +12453,5 @@ } }, "version": "0.0.0", - "fingerprint": "XNjL7+hJ5KwFtBCVEz4TZH4c2JtVW/gTPS7UNrw02Cg=" + "fingerprint": "4Q+a0fGKWl6wIrpV76N462Ux3HDgzTeZ3NQrjw3x0Mo=" } diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index b22353feed..ca155796eb 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -35,7 +35,7 @@ export class Assembler implements Emitter { /** Map of Symbol to namespace export Symbol */ private readonly _submoduleMap = new Map(); - private readonly _submodules = new Set(); + private readonly _submodules = new Map(); /** * @param projectInfo information about the package being assembled @@ -153,6 +153,7 @@ export class Assembler implements Emitter { dependencyClosure: noEmptyDict(toDependencyClosure(this.projectInfo.dependencyClosure)), bundled: this.projectInfo.bundleDependencies, types: this._types, + submodules: noEmptyDict(toSubmoduleDeclarations(this._submodules.values())), targets: this.projectInfo.targets, metadata: this.projectInfo.metadata, docs, @@ -345,17 +346,13 @@ export class Assembler implements Emitter { return `unknown.${typeName}`; } - let submodule = this._submoduleMap.get( type.symbol); - let submoduleNs = submodule?.name; - // Submodules can be in submodules themselves, so we crawl up the tree... - while (submodule != null && this._submoduleMap.has(submodule)) { - submodule = this._submoduleMap.get(submodule)!; - submoduleNs = `${submodule.name}.${submoduleNs}`; + const submodule = this._submoduleMap.get(type.symbol); + if (submodule != null) { + const submoduleNs = this._submodules.get(submodule)!.fqnResolutionPrefix; + return `${submoduleNs}.${typeName}`; } - const fqn = submoduleNs != null - ? `${pkg.name}.${submoduleNs}.${typeName}` - : `${pkg.name}.${typeName}`; + const fqn = `${pkg.name}.${typeName}`; if (pkg.name !== this.projectInfo.name && !this._dereference({ fqn }, type.symbol.valueDeclaration)) { this._diagnostic(node, ts.DiagnosticCategory.Error, @@ -376,10 +373,23 @@ export class Assembler implements Emitter { private _registerNamespaces(symbol: ts.Symbol): void { const declaration = symbol.valueDeclaration ?? symbol.declarations[0]; - if (declaration == null || !ts.isNamespaceExport(declaration)) { + if (declaration == null) { // Nothing to do here... return; } + if (ts.isModuleDeclaration(declaration)) { + const { fqn, fqnResolutionPrefix } = qualifiedNameOf.call(this, symbol, true); + const targets = undefined; // This will be configurable in the future. + + this._submodules.set(symbol, { fqn, fqnResolutionPrefix, targets }); + this._addToSubmodule(symbol, symbol); + return; + } + if (!ts.isNamespaceExport(declaration)) { + // Nothing to do here... + return; + } + const moduleSpecifier = declaration.parent.moduleSpecifier; if (moduleSpecifier == null || !ts.isStringLiteral(moduleSpecifier)) { // There is a grammar error here, so we'll let tsc report this for us. @@ -409,9 +419,29 @@ export class Assembler implements Emitter { this._diagnostic(declaration, ts.DiagnosticCategory.Error, `Submodule namespaces must be camelCased or snake_cased. Consider renaming to "${Case.camel(symbol.name)}".`); } - this._submodules.add(symbol); + + const { fqn, fqnResolutionPrefix } = qualifiedNameOf.call(this, symbol); + const targets = undefined; // This will be configurable in the future. + + this._submodules.set(symbol, { fqn, fqnResolutionPrefix, targets }); this._addToSubmodule(symbol, sourceModule); } + + function qualifiedNameOf(this: Assembler, sym: ts.Symbol, inlineNamespace = false): { fqn: string, fqnResolutionPrefix: string } { + if (this._submoduleMap.has(sym)) { + const parent = this._submodules.get(this._submoduleMap.get(sym)!)!; + const fqn = `${parent.fqn}.${sym.name}`; + return { + fqn, + fqnResolutionPrefix: inlineNamespace ? parent.fqnResolutionPrefix : fqn, + }; + } + const fqn = `${this.projectInfo.name}.${sym.name}`; + return { + fqn, + fqnResolutionPrefix: inlineNamespace ? this.projectInfo.name : fqn, + }; + } } /** @@ -479,9 +509,8 @@ export class Assembler implements Emitter { this._addToSubmodule(ns, symbol); } } else if (ts.isModuleDeclaration(decl)) { - this._addToSubmodule(ns, symbol); + this._registerNamespaces(symbol); } else if (ts.isNamespaceExport(decl)) { - this._submoduleMap.set(symbol, ns); this._registerNamespaces(symbol); } } @@ -548,7 +577,7 @@ export class Assembler implements Emitter { // Let's quickly verify the declaration does not collide with a submodule. Submodules get case-adjusted for each // target language separately, so names cannot collide with case-variations. - for (const submodule of this._submodules) { + for (const submodule of this._submodules.keys()) { const candidates = Array.from(new Set([ submodule.name, Case.camel(submodule.name), @@ -1566,6 +1595,25 @@ export class Assembler implements Emitter { } } +interface SubmoduleSpec { + /** + * The submodule's fully qualified name. + */ + readonly fqn: string; + + /** + * The submodule's fully qualified name prefix to use when resolving type FQNs. This does not + * include "inline namespace" names as those are already represented in the TypeCheckers' view of + * the type names. + */ + readonly fqnResolutionPrefix: string; + + /** + * Any customized configuration for the currentl submodule. + */ + readonly targets?: spec.AssemblyTargets; +} + function _fingerprint(assembly: spec.Assembly): spec.Assembly { delete assembly.fingerprint; assembly = sortJson(assembly); @@ -1782,8 +1830,8 @@ function* intersect(xs: Set, ys: Set) { } } -function noEmptyDict(xs: {[key: string]: T}): {[key: string]: T} | undefined { - if (Object.keys(xs).length === 0) { return undefined; } +function noEmptyDict(xs: Record | undefined): Record | undefined { + if (xs == null || Object.keys(xs).length === 0) { return undefined; } return xs; } @@ -1791,8 +1839,23 @@ function toDependencyClosure(assemblies: readonly spec.Assembly[]): { [name: str const result: { [name: string]: spec.AssemblyTargets } = {}; for (const assembly of assemblies) { if (!assembly.targets) { continue; } - result[assembly.name] = { targets: assembly.targets }; + result[assembly.name] = { + submodules: assembly.submodules, + targets: assembly.targets, + }; + } + return result; +} + +function toSubmoduleDeclarations(submodules: IterableIterator): spec.Assembly['submodules'] { + const result: spec.Assembly['submodules'] = {}; + + for (const submodule of submodules) { + result[submodule.fqn] = { + targets: submodule.targets, + }; } + return result; } From d0d887e4b734d52f230c2ce278431c8c4bed1176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 17 Apr 2020 14:59:18 +0200 Subject: [PATCH 2/5] remove hint to future configurability of inline namespaces (it likely won't happen) --- packages/jsii/lib/assembler.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index ca155796eb..47f7d33e30 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -379,9 +379,8 @@ export class Assembler implements Emitter { } if (ts.isModuleDeclaration(declaration)) { const { fqn, fqnResolutionPrefix } = qualifiedNameOf.call(this, symbol, true); - const targets = undefined; // This will be configurable in the future. - this._submodules.set(symbol, { fqn, fqnResolutionPrefix, targets }); + this._submodules.set(symbol, { fqn, fqnResolutionPrefix }); this._addToSubmodule(symbol, symbol); return; } From 14f65ed8377b6edb8c0741ef24060d1be338a0ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 17 Apr 2020 15:01:37 +0200 Subject: [PATCH 3/5] remove spurious line feed --- packages/@jsii/spec/lib/assembly.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@jsii/spec/lib/assembly.ts b/packages/@jsii/spec/lib/assembly.ts index 6685d52a1d..5c65e62ec4 100644 --- a/packages/@jsii/spec/lib/assembly.ts +++ b/packages/@jsii/spec/lib/assembly.ts @@ -175,7 +175,6 @@ export interface AssemblyConfiguration { targets?: AssemblyTargets; } - /** * Versions of the JSII Assembly Specification. */ From 40763515004fd8df39ff6e8d723b6113609d61d0 Mon Sep 17 00:00:00 2001 From: Romain Marcadier-Muller Date: Fri, 17 Apr 2020 15:02:45 +0200 Subject: [PATCH 4/5] fixup test expectations --- .../Amazon.JSII.Tests.CalculatorPackageId/.jsii | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii index 11f2d2e72e..dcdf7ebe4c 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii +++ b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii @@ -137,6 +137,17 @@ "url": "https://github.com/aws/jsii.git" }, "schema": "jsii/0.10.0", + "submodules": { + "jsii-calc.DerivedClassHasNoProperties": {}, + "jsii-calc.InterfaceInNamespaceIncludesClasses": {}, + "jsii-calc.InterfaceInNamespaceOnlyInterface": {}, + "jsii-calc.composition": {}, + "jsii-calc.submodule": {}, + "jsii-calc.submodule.back_references": {}, + "jsii-calc.submodule.child": {}, + "jsii-calc.submodule.nested_submodule": {}, + "jsii-calc.submodule.nested_submodule.deeplyNested": {} + }, "targets": { "dotnet": { "iconUrl": "https://sdk-for-net.amazonwebservices.com/images/AWSLogo128x128.png", @@ -12442,5 +12453,5 @@ } }, "version": "0.0.0", - "fingerprint": "XNjL7+hJ5KwFtBCVEz4TZH4c2JtVW/gTPS7UNrw02Cg=" + "fingerprint": "4Q+a0fGKWl6wIrpV76N462Ux3HDgzTeZ3NQrjw3x0Mo=" } From c3213af5b072cb62852e5db477ed3de86ca84445 Mon Sep 17 00:00:00 2001 From: Romain Marcadier-Muller Date: Mon, 20 Apr 2020 11:44:04 +0200 Subject: [PATCH 5/5] add source location of submodule declarations --- packages/@jsii/spec/lib/assembly.ts | 19 ++---- packages/jsii-calc/test/assembly.jsii | 65 ++++++++++++++++--- .../.jsii | 65 ++++++++++++++++--- packages/jsii/lib/assembler.ts | 10 ++- 4 files changed, 125 insertions(+), 34 deletions(-) diff --git a/packages/@jsii/spec/lib/assembly.ts b/packages/@jsii/spec/lib/assembly.ts index 5c65e62ec4..6a73bfd91a 100644 --- a/packages/@jsii/spec/lib/assembly.ts +++ b/packages/@jsii/spec/lib/assembly.ts @@ -148,24 +148,19 @@ export interface Assembly extends AssemblyConfiguration, Documentable { /** * Shareable configuration of a jsii Assembly. */ -export interface AssemblyConfiguration { +export interface AssemblyConfiguration extends Targetable { /** * Submodules declared in this assembly. * * @default none */ - submodules?: { - [fqn: string]: { - /** - * The specific code generation configuration for this submodule, if it - * differs from it's parent. - * - * @default - the parent submodule or assembly's configuration. - */ - targets?: AssemblyTargets; - }; - }; + submodules?: { [fqn: string]: SourceLocatable & Targetable }; +} +/** + * An entity on which targets may be configured. + */ +export interface Targetable { /** * A map of target name to configuration, which is used when generating * packages for various languages. diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii index dcdf7ebe4c..328b65012d 100644 --- a/packages/jsii-calc/test/assembly.jsii +++ b/packages/jsii-calc/test/assembly.jsii @@ -138,15 +138,60 @@ }, "schema": "jsii/0.10.0", "submodules": { - "jsii-calc.DerivedClassHasNoProperties": {}, - "jsii-calc.InterfaceInNamespaceIncludesClasses": {}, - "jsii-calc.InterfaceInNamespaceOnlyInterface": {}, - "jsii-calc.composition": {}, - "jsii-calc.submodule": {}, - "jsii-calc.submodule.back_references": {}, - "jsii-calc.submodule.child": {}, - "jsii-calc.submodule.nested_submodule": {}, - "jsii-calc.submodule.nested_submodule.deeplyNested": {} + "jsii-calc.DerivedClassHasNoProperties": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 309 + } + }, + "jsii-calc.InterfaceInNamespaceIncludesClasses": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1057 + } + }, + "jsii-calc.InterfaceInNamespaceOnlyInterface": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1048 + } + }, + "jsii-calc.composition": { + "locationInModule": { + "filename": "lib/calculator.ts", + "line": 127 + } + }, + "jsii-calc.submodule": { + "locationInModule": { + "filename": "lib/index.ts", + "line": 7 + } + }, + "jsii-calc.submodule.back_references": { + "locationInModule": { + "filename": "lib/submodule/index.ts", + "line": 4 + } + }, + "jsii-calc.submodule.child": { + "locationInModule": { + "filename": "lib/submodule/index.ts", + "line": 1 + } + }, + "jsii-calc.submodule.nested_submodule": { + "locationInModule": { + "filename": "lib/submodule/nested_submodule.ts", + "line": 3 + } + }, + "jsii-calc.submodule.nested_submodule.deeplyNested": { + "locationInModule": { + "filename": "lib/submodule/nested_submodule.ts", + "line": 4 + } + } }, "targets": { "dotnet": { @@ -12453,5 +12498,5 @@ } }, "version": "0.0.0", - "fingerprint": "4Q+a0fGKWl6wIrpV76N462Ux3HDgzTeZ3NQrjw3x0Mo=" + "fingerprint": "LPI1XbecdL/VPbXVTmbAHHV0ox1k1cAxzrWF/f0rIxI=" } diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii index dcdf7ebe4c..328b65012d 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii +++ b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii @@ -138,15 +138,60 @@ }, "schema": "jsii/0.10.0", "submodules": { - "jsii-calc.DerivedClassHasNoProperties": {}, - "jsii-calc.InterfaceInNamespaceIncludesClasses": {}, - "jsii-calc.InterfaceInNamespaceOnlyInterface": {}, - "jsii-calc.composition": {}, - "jsii-calc.submodule": {}, - "jsii-calc.submodule.back_references": {}, - "jsii-calc.submodule.child": {}, - "jsii-calc.submodule.nested_submodule": {}, - "jsii-calc.submodule.nested_submodule.deeplyNested": {} + "jsii-calc.DerivedClassHasNoProperties": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 309 + } + }, + "jsii-calc.InterfaceInNamespaceIncludesClasses": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1057 + } + }, + "jsii-calc.InterfaceInNamespaceOnlyInterface": { + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1048 + } + }, + "jsii-calc.composition": { + "locationInModule": { + "filename": "lib/calculator.ts", + "line": 127 + } + }, + "jsii-calc.submodule": { + "locationInModule": { + "filename": "lib/index.ts", + "line": 7 + } + }, + "jsii-calc.submodule.back_references": { + "locationInModule": { + "filename": "lib/submodule/index.ts", + "line": 4 + } + }, + "jsii-calc.submodule.child": { + "locationInModule": { + "filename": "lib/submodule/index.ts", + "line": 1 + } + }, + "jsii-calc.submodule.nested_submodule": { + "locationInModule": { + "filename": "lib/submodule/nested_submodule.ts", + "line": 3 + } + }, + "jsii-calc.submodule.nested_submodule.deeplyNested": { + "locationInModule": { + "filename": "lib/submodule/nested_submodule.ts", + "line": 4 + } + } }, "targets": { "dotnet": { @@ -12453,5 +12498,5 @@ } }, "version": "0.0.0", - "fingerprint": "4Q+a0fGKWl6wIrpV76N462Ux3HDgzTeZ3NQrjw3x0Mo=" + "fingerprint": "LPI1XbecdL/VPbXVTmbAHHV0ox1k1cAxzrWF/f0rIxI=" } diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 47f7d33e30..099f95b035 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -380,7 +380,7 @@ export class Assembler implements Emitter { if (ts.isModuleDeclaration(declaration)) { const { fqn, fqnResolutionPrefix } = qualifiedNameOf.call(this, symbol, true); - this._submodules.set(symbol, { fqn, fqnResolutionPrefix }); + this._submodules.set(symbol, { fqn, fqnResolutionPrefix, locationInModule: this.declarationLocation(declaration) }); this._addToSubmodule(symbol, symbol); return; } @@ -422,7 +422,7 @@ export class Assembler implements Emitter { const { fqn, fqnResolutionPrefix } = qualifiedNameOf.call(this, symbol); const targets = undefined; // This will be configurable in the future. - this._submodules.set(symbol, { fqn, fqnResolutionPrefix, targets }); + this._submodules.set(symbol, { fqn, fqnResolutionPrefix, targets, locationInModule: this.declarationLocation(declaration) }); this._addToSubmodule(symbol, sourceModule); } @@ -1607,6 +1607,11 @@ interface SubmoduleSpec { */ readonly fqnResolutionPrefix: string; + /** + * The location of the submodule definition in the source. + */ + readonly locationInModule: spec.SourceLocation; + /** * Any customized configuration for the currentl submodule. */ @@ -1851,6 +1856,7 @@ function toSubmoduleDeclarations(submodules: IterableIterator): s for (const submodule of submodules) { result[submodule.fqn] = { + locationInModule: submodule.locationInModule, targets: submodule.targets, }; }