From b2fd0bd0f6db88dc520fbe9c57774148c68c3888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=A7=91=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Mon, 2 May 2022 12:09:53 +0200 Subject: [PATCH 1/2] fix(rosetta): incorrect transliteration for selective imports Go does nto support "selective" imports, and has limited symbol aliasing support. Instead of attempting to transliterate selective imports with a one-to-one semantic mapping, resolve the imported symbols and emit the correct qualified original name instead. This results in more idiomatic (and likely more correct) go code. For example: ```ts import { Foo as RenamedFoo, Bar } from 'baz'; const foo = new RenamedFoo(); Bar.baz(foo); ``` Should transliterate to something looking like: ```go import "github.com/aws-samples/dummy/baz" foo := baz.NewFoo() Bar_baz(foo) ``` --- .../lib/targets/go/documentation.ts | 11 +++ .../jsii-pacmak/lib/targets/go/package.ts | 31 +++----- .../jsii-pacmak/lib/targets/go/readme-file.ts | 38 ++------- .../__snapshots__/target-go.test.ts.snap | 42 +++++++--- packages/jsii-rosetta/lib/languages/go.ts | 79 +++++++++++++------ .../translations/imports/multiple-imports.cs | 2 + .../translations/imports/multiple-imports.go | 2 + .../imports/multiple-imports.java | 3 + .../translations/imports/multiple-imports.py | 2 + .../translations/imports/multiple-imports.ts | 2 + .../translations/imports/selective_import.go | 3 + 11 files changed, 130 insertions(+), 85 deletions(-) create mode 100644 packages/jsii-rosetta/test/translations/imports/multiple-imports.cs create mode 100644 packages/jsii-rosetta/test/translations/imports/multiple-imports.go create mode 100644 packages/jsii-rosetta/test/translations/imports/multiple-imports.java create mode 100644 packages/jsii-rosetta/test/translations/imports/multiple-imports.py create mode 100644 packages/jsii-rosetta/test/translations/imports/multiple-imports.ts create mode 100644 packages/jsii-rosetta/test/translations/imports/selective_import.go diff --git a/packages/jsii-pacmak/lib/targets/go/documentation.ts b/packages/jsii-pacmak/lib/targets/go/documentation.ts index 497878f2b5..0c3f5ea386 100644 --- a/packages/jsii-pacmak/lib/targets/go/documentation.ts +++ b/packages/jsii-pacmak/lib/targets/go/documentation.ts @@ -75,6 +75,17 @@ export class Documentation { } } + public emitReadme(moduleFqn: string, readme: string, directory: string) { + const goReadme = this.rosetta.translateSnippetsInMarkdown({ api: 'moduleReadme', moduleFqn }, readme, TargetLanguage.GO, false); + + const readmeFile = `${directory}/README.md`; + this.code.openFile(readmeFile); + for (const line of goReadme.split('\n')) { + this.code.line(line); + } + this.code.closeFile(readmeFile); + } + private emitComment(text = '') { const lines = text.trim().split('\n'); diff --git a/packages/jsii-pacmak/lib/targets/go/package.ts b/packages/jsii-pacmak/lib/targets/go/package.ts index d5acbe5972..849974a3d4 100644 --- a/packages/jsii-pacmak/lib/targets/go/package.ts +++ b/packages/jsii-pacmak/lib/targets/go/package.ts @@ -1,5 +1,5 @@ import { CodeMaker } from 'codemaker'; -import { Assembly, Type, Submodule as JsiiSubmodule } from 'jsii-reflect'; +import { Assembly, ModuleLike as JsiiModuleLike, Type, Submodule as JsiiSubmodule } from 'jsii-reflect'; import { basename, dirname, join } from 'path'; import * as semver from 'semver'; @@ -42,10 +42,10 @@ export abstract class Package { public readonly types: GoType[]; private readonly embeddedTypes = new Map(); + private readonly readmeFile?: ReadmeFile; public constructor( - private readonly typeSpec: readonly Type[], - private readonly submoduleSpec: readonly JsiiSubmodule[], + private readonly jsiiModule: JsiiModuleLike, public readonly packageName: string, public readonly filePath: string, public readonly moduleName: string, @@ -56,11 +56,11 @@ export abstract class Package { this.directory = filePath; this.file = `${this.directory}/${packageName}.go`; this.root = root ?? this; - this.submodules = this.submoduleSpec.map( + this.submodules = this.jsiiModule.submodules.map( (sm) => new InternalPackage(this.root, this, sm), ); - this.types = this.typeSpec.map((type: Type): GoType => { + this.types = this.jsiiModule.types.map((type: Type): GoType => { if (type.isInterfaceType() && type.datatype) { return new Struct(this, type); } else if (type.isInterfaceType()) { @@ -74,6 +74,10 @@ export abstract class Package { `Type: ${type.name} with kind ${type.kind} is not a supported type`, ); }); + + if (this.jsiiModule.readme?.markdown) { + this.readmeFile = new ReadmeFile(this.jsiiModule.fqn, this.jsiiModule.readme.markdown, this.directory); + } } /* @@ -121,6 +125,8 @@ export abstract class Package { this.emitTypes(context); code.closeFile(this.file); + this.readmeFile?.emit(context); + this.emitGoInitFunction(context); this.emitSubmodules(context); @@ -318,7 +324,6 @@ export abstract class Package { export class RootPackage extends Package { public readonly assembly: Assembly; public readonly version: string; - private readonly readme?: ReadmeFile; private readonly versionFile: VersionFile; // This cache of root packages is shared across all root packages derived created by this one (via dependencies). @@ -335,8 +340,7 @@ export class RootPackage extends Package { const version = `${assembly.version}${goConfig.versionSuffix ?? ''}`; super( - assembly.types, - assembly.submodules, + assembly, packageName, filePath, moduleName, @@ -349,19 +353,11 @@ export class RootPackage extends Package { this.assembly = assembly; this.version = version; this.versionFile = new VersionFile(this.version); - - if (this.assembly.readme?.markdown) { - this.readme = new ReadmeFile( - this.packageName, - this.assembly.readme.markdown, - ); - } } public emit(context: EmitContext): void { super.emit(context); this.emitJsiiPackage(context); - this.readme?.emit(context); this.emitGomod(context.code); this.versionFile.emit(context.code); @@ -518,8 +514,7 @@ export class InternalPackage extends Package { parent === root ? packageName : `${parent.filePath}/${packageName}`; super( - assembly.types, - assembly.submodules, + assembly, packageName, filePath, root.moduleName, diff --git a/packages/jsii-pacmak/lib/targets/go/readme-file.ts b/packages/jsii-pacmak/lib/targets/go/readme-file.ts index 2ad5b4c21c..de7f49b104 100644 --- a/packages/jsii-pacmak/lib/targets/go/readme-file.ts +++ b/packages/jsii-pacmak/lib/targets/go/readme-file.ts @@ -1,44 +1,16 @@ -import { join } from 'path'; - import { EmitContext } from './emit-context'; -// import { Translation } from 'jsii-rosetta'; -// import { INCOMPLETE_DISCLAIMER_COMPILING, INCOMPLETE_DISCLAIMER_NONCOMPILING } from '..'; export class ReadmeFile { public constructor( private readonly packageName: string, private readonly document: string, + private readonly directory: string, ) {} - public emit({ code /*, rosetta */ }: EmitContext) { - const nameParts = this.packageName.split('.'); - const file = join( - ...nameParts.slice(0, nameParts.length - 11), - 'README.md', - ); - - code.openFile(file); - const translated = this.document; // rosetta.translateSnippetsInMarkdown(this.document, 'go', prependDisclaimer); - for (const line of translated.split('\n')) { - code.line(line); - } - code.closeFile(file); - } - - /* - function prependDisclaimer(translation: Translation) { - const source = addDisclaimerIfNeeded(); - return { language: translation.language, source }; - - function addDisclaimerIfNeeded(): string { - if (translation.didCompile && INCOMPLETE_DISCLAIMER_COMPILING) { - return `// ${INCOMPLETE_DISCLAIMER_COMPILING}\n\n${translation.source}`; - } - if (!translation.didCompile && INCOMPLETE_DISCLAIMER_NONCOMPILING) { - return `// ${INCOMPLETE_DISCLAIMER_NONCOMPILING}\n\n${translation.source}`; - } - return translation.source; + public emit({ documenter }: EmitContext) { + if (!this.document) { + return; } + documenter.emitReadme(this.packageName, this.document, this.directory) } - */ } diff --git a/packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.ts.snap b/packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.ts.snap index 10966f4860..634e7178da 100644 --- a/packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.ts.snap +++ b/packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.ts.snap @@ -883,7 +883,8 @@ exports[`Generated code for "@scope/jsii-calc-lib": / 1`] = ` ┗━ 📁 scopejsiicalclib ┣━ 📁 customsubmodulename ┃ ┣━ 📄 customsubmodulename.go - ┃ ┗━ 📄 customsubmodulename.init.go + ┃ ┣━ 📄 customsubmodulename.init.go + ┃ ┗━ 📄 README.md ┣━ 📄 go.mod ┣━ 📁 internal ┃ ┗━ 📄 types.go @@ -1108,6 +1109,13 @@ Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. `; +exports[`Generated code for "@scope/jsii-calc-lib": /go/scopejsiicalclib/customsubmodulename/README.md 1`] = ` +# Submodule Readme + +This is a submodule readme. + +`; + exports[`Generated code for "@scope/jsii-calc-lib": /go/scopejsiicalclib/customsubmodulename/customsubmodulename.go 1`] = ` package customsubmodulename @@ -2050,7 +2058,8 @@ exports[`Generated code for "jsii-calc": / 1`] = ` ┃ ┃ ┗━ 📄 types.go ┃ ┣━ 📁 isolated ┃ ┃ ┣━ 📄 isolated.go - ┃ ┃ ┗━ 📄 isolated.init.go + ┃ ┃ ┣━ 📄 isolated.init.go + ┃ ┃ ┗━ 📄 README.md ┃ ┣━ 📁 nestedsubmodule ┃ ┃ ┣━ 📁 deeplynested ┃ ┃ ┃ ┣━ 📄 deeplynested.go @@ -2062,6 +2071,7 @@ exports[`Generated code for "jsii-calc": / 1`] = ` ┃ ┣━ 📁 param ┃ ┃ ┣━ 📄 param.go ┃ ┃ ┗━ 📄 param.init.go + ┃ ┣━ 📄 README.md ┃ ┣━ 📁 returnsparam ┃ ┃ ┣━ 📄 returnsparam.go ┃ ┃ ┗━ 📄 returnsparam.init.go @@ -2290,25 +2300,23 @@ This library is used to demonstrate and test the features of JSII First, create a calculator: -\`\`\`ts -const calculator = new calc.Calculator(); +\`\`\`go +calculator := calc.NewCalculator() \`\`\` Then call some operations: - -\`\`\`ts fixture=with-calculator -calculator.add(10); +\`\`\`go +calculator.add(jsii.Number(10)) \`\`\` ## Code Samples -\`\`\`ts +\`\`\`go /* This is totes a magic comment in here, just you wait! */ -const foo = 'bar'; +foo := "bar" \`\`\` - `; exports[`Generated code for "jsii-calc": /go/jsiicalc/cdk16625/cdk16625.go 1`] = ` @@ -18958,6 +18966,13 @@ func init() { `; +exports[`Generated code for "jsii-calc": /go/jsiicalc/submodule/README.md 1`] = ` +# Read you, read me + +This is the readme of the \`jsii-calc.submodule\` module. + +`; + exports[`Generated code for "jsii-calc": /go/jsiicalc/submodule/backreferences/backreferences.go 1`] = ` package backreferences @@ -19204,6 +19219,13 @@ type Type__deeplynestedINamespaced = deeplynested.INamespaced `; +exports[`Generated code for "jsii-calc": /go/jsiicalc/submodule/isolated/README.md 1`] = ` +# Read you, read me + +This is the readme of the \`jsii-calc.submodule.isolated\` module. + +`; + exports[`Generated code for "jsii-calc": /go/jsiicalc/submodule/isolated/isolated.go 1`] = ` package isolated diff --git a/packages/jsii-rosetta/lib/languages/go.ts b/packages/jsii-rosetta/lib/languages/go.ts index bb0b80743b..9fcb50632d 100644 --- a/packages/jsii-rosetta/lib/languages/go.ts +++ b/packages/jsii-rosetta/lib/languages/go.ts @@ -7,7 +7,7 @@ import { analyzeObjectLiteral, determineJsiiType, JsiiType, ObjectLiteralStruct import { OTree } from '../o-tree'; import { AstRenderer } from '../renderer'; import { isExported, isPublic, isPrivate, isReadOnly, isStatic } from '../typescript/ast-utils'; -import { ImportStatement } from '../typescript/imports'; +import { analyzeImportDeclaration, ImportStatement } from '../typescript/imports'; import { determineReturnType, inferMapElementType, @@ -156,6 +156,32 @@ export class GoVisitor extends DefaultVisitor { public identifier(node: ts.Identifier | ts.StringLiteral | ts.NoSubstitutionTemplateLiteral, renderer: GoRenderer) { const symbol = renderer.typeChecker.getSymbolAtLocation(node); + + // If the identifier corresponds to a renamed imported symbol, we need to use the original symbol name, qualified + // with the import package name, since Go does not allow generalized symbol aliasing (we *could* alias types, but + // not static functions or constructors). + const declaration = symbol?.valueDeclaration ?? symbol?.declarations?.[0]; + if (declaration && ts.isImportSpecifier(declaration)) { + const importInfo = analyzeImportDeclaration(declaration.parent.parent.parent, renderer); + const packageName = + importInfo.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.packageName ?? + this.goName(importInfo.packageName, renderer, undefined); + + const importedSymbol = declaration.propertyName + ? renderer.typeChecker.getSymbolAtLocation(declaration.propertyName) + : symbol; + // Note: imported members are (by nature) always exported by the module they are imported from. + return new OTree([ + packageName, + '.', + this.goName( + (declaration.propertyName ?? declaration.name).text, + renderer.updateContext({ isExported: true }), + importedSymbol, + ), + ]); + } + return new OTree([this.goName(node.text, renderer, symbol)]); } @@ -174,6 +200,26 @@ export class GoVisitor extends DefaultVisitor { function determineClassName(this: GoVisitor, expr: ts.Expression): { classNamespace?: OTree; className: string } { if (ts.isIdentifier(expr)) { + // Imported names are referred to by the original (i.e: exported) name, qualified with the source module's go + // package name. + const symbol = renderer.typeChecker.getSymbolAtLocation(expr); + const declaration = symbol?.valueDeclaration ?? symbol?.declarations?.[0]; + if (declaration && ts.isImportSpecifier(declaration)) { + const importInfo = analyzeImportDeclaration(declaration.parent.parent.parent, renderer); + const packageName = + importInfo.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.packageName ?? + this.goName(importInfo.packageName, renderer, undefined); + + return { + classNamespace: new OTree([packageName]), + className: this.goName( + (declaration.propertyName ?? declaration.name).text, + renderer.updateContext({ isExported: true }), + symbol, + ), + }; + } + return { className: ucFirst(expr.text) }; } if (ts.isPropertyAccessExpression(expr)) { @@ -761,34 +807,19 @@ export class GoVisitor extends DefaultVisitor { : `github.com/aws-samples/dummy/${packageName}`; if (node.imports.import === 'full') { - return new OTree(['import ', this.goName(node.imports.alias, renderer, undefined), ' "', moduleName, '"']); - } - - // We'll just create local type aliases for all imported types. This is not very go-idiomatic, but simplifies things elsewhere... - const elements = node.imports.elements - .filter((element) => element.importedSymbol?.symbolType === 'type') - .map( - (element) => - new OTree(['type ', element.alias ?? element.sourceName, ' ', packageName, '.', element.sourceName]), - ); - - const submodules = node.imports.elements - .filter((element) => element.importedSymbol?.symbolType === 'module') - .map( - (element) => - new OTree(['import ', element.alias ?? element.sourceName, ' "', moduleName, '/', element.sourceName, '"']), + return new OTree( + ['import ', this.goName(node.imports.alias, renderer, undefined), ' "', moduleName, '"'], + undefined, + { canBreakLine: true }, ); + } - if (elements.length === 0 && submodules.length === 0) { + if (node.imports.elements.length === 0) { // This is a blank import (for side-effects only) - return new OTree(['import _ "', moduleName, '"']); + return new OTree(['import _ "', moduleName, '"'], undefined, { canBreakLine: true }); } - const mainImport = new OTree(['import ', packageName, ' "', moduleName, '"'], elements, { - canBreakLine: true, - separator: '\n', - }); - return new OTree([mainImport, ...submodules]); + return new OTree(['import "', moduleName, '"'], undefined, { canBreakLine: true }); } public variableDeclaration(node: ts.VariableDeclaration, renderer: AstRenderer): OTree { diff --git a/packages/jsii-rosetta/test/translations/imports/multiple-imports.cs b/packages/jsii-rosetta/test/translations/imports/multiple-imports.cs new file mode 100644 index 0000000000..4fa24d9a94 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/multiple-imports.cs @@ -0,0 +1,2 @@ +using Aws.Cdk.Lib; +using Constructs; diff --git a/packages/jsii-rosetta/test/translations/imports/multiple-imports.go b/packages/jsii-rosetta/test/translations/imports/multiple-imports.go new file mode 100644 index 0000000000..b7eef4802c --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/multiple-imports.go @@ -0,0 +1,2 @@ +import cdk "github.com/aws-samples/dummy/awscdklib" +import "github.com/aws-samples/dummy/constructs" diff --git a/packages/jsii-rosetta/test/translations/imports/multiple-imports.java b/packages/jsii-rosetta/test/translations/imports/multiple-imports.java new file mode 100644 index 0000000000..98da2d9413 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/multiple-imports.java @@ -0,0 +1,3 @@ +import aws.cdk.lib.*; +import constructs.Construct; +import constructs.IConstruct; diff --git a/packages/jsii-rosetta/test/translations/imports/multiple-imports.py b/packages/jsii-rosetta/test/translations/imports/multiple-imports.py new file mode 100644 index 0000000000..77648bf309 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/multiple-imports.py @@ -0,0 +1,2 @@ +import aws_cdk_lib as cdk +from constructs import Construct, IConstruct diff --git a/packages/jsii-rosetta/test/translations/imports/multiple-imports.ts b/packages/jsii-rosetta/test/translations/imports/multiple-imports.ts new file mode 100644 index 0000000000..70076f6880 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/multiple-imports.ts @@ -0,0 +1,2 @@ +import * as cdk from 'aws-cdk-lib'; +import { Construct, IConstruct } from 'constructs'; diff --git a/packages/jsii-rosetta/test/translations/imports/selective_import.go b/packages/jsii-rosetta/test/translations/imports/selective_import.go new file mode 100644 index 0000000000..e617f3d5a9 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/imports/selective_import.go @@ -0,0 +1,3 @@ +import "github.com/aws-samples/dummy/scopesomemodule" +scopesomemodule.NewTwo() +scopesomemodule.Four() From a1c7c58cee6b1be5c425b3f23852077fe44a2c12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=A7=91=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Fri, 6 May 2022 11:50:39 +0200 Subject: [PATCH 2/2] linter fix --- .../lib/targets/go/documentation.ts | 7 ++++- .../jsii-pacmak/lib/targets/go/package.ts | 30 ++++++++----------- .../jsii-pacmak/lib/targets/go/readme-file.ts | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/packages/jsii-pacmak/lib/targets/go/documentation.ts b/packages/jsii-pacmak/lib/targets/go/documentation.ts index 0c3f5ea386..99291ab226 100644 --- a/packages/jsii-pacmak/lib/targets/go/documentation.ts +++ b/packages/jsii-pacmak/lib/targets/go/documentation.ts @@ -76,7 +76,12 @@ export class Documentation { } public emitReadme(moduleFqn: string, readme: string, directory: string) { - const goReadme = this.rosetta.translateSnippetsInMarkdown({ api: 'moduleReadme', moduleFqn }, readme, TargetLanguage.GO, false); + const goReadme = this.rosetta.translateSnippetsInMarkdown( + { api: 'moduleReadme', moduleFqn }, + readme, + TargetLanguage.GO, + false, + ); const readmeFile = `${directory}/README.md`; this.code.openFile(readmeFile); diff --git a/packages/jsii-pacmak/lib/targets/go/package.ts b/packages/jsii-pacmak/lib/targets/go/package.ts index 849974a3d4..36c86198b3 100644 --- a/packages/jsii-pacmak/lib/targets/go/package.ts +++ b/packages/jsii-pacmak/lib/targets/go/package.ts @@ -1,5 +1,10 @@ import { CodeMaker } from 'codemaker'; -import { Assembly, ModuleLike as JsiiModuleLike, Type, Submodule as JsiiSubmodule } from 'jsii-reflect'; +import { + Assembly, + ModuleLike as JsiiModuleLike, + Type, + Submodule as JsiiSubmodule, +} from 'jsii-reflect'; import { basename, dirname, join } from 'path'; import * as semver from 'semver'; @@ -76,7 +81,11 @@ export abstract class Package { }); if (this.jsiiModule.readme?.markdown) { - this.readmeFile = new ReadmeFile(this.jsiiModule.fqn, this.jsiiModule.readme.markdown, this.directory); + this.readmeFile = new ReadmeFile( + this.jsiiModule.fqn, + this.jsiiModule.readme.markdown, + this.directory, + ); } } @@ -339,13 +348,7 @@ export class RootPackage extends Package { const moduleName = goConfig.moduleName ?? ''; const version = `${assembly.version}${goConfig.versionSuffix ?? ''}`; - super( - assembly, - packageName, - filePath, - moduleName, - version, - ); + super(assembly, packageName, filePath, moduleName, version); this.rootPackageCache = rootPackageCache; this.rootPackageCache.set(assembly.name, this); @@ -513,14 +516,7 @@ export class InternalPackage extends Package { const filePath = parent === root ? packageName : `${parent.filePath}/${packageName}`; - super( - assembly, - packageName, - filePath, - root.moduleName, - root.version, - root, - ); + super(assembly, packageName, filePath, root.moduleName, root.version, root); this.parent = parent; } diff --git a/packages/jsii-pacmak/lib/targets/go/readme-file.ts b/packages/jsii-pacmak/lib/targets/go/readme-file.ts index de7f49b104..c8a94b3661 100644 --- a/packages/jsii-pacmak/lib/targets/go/readme-file.ts +++ b/packages/jsii-pacmak/lib/targets/go/readme-file.ts @@ -11,6 +11,6 @@ export class ReadmeFile { if (!this.document) { return; } - documenter.emitReadme(this.packageName, this.document, this.directory) + documenter.emitReadme(this.packageName, this.document, this.directory); } }