fix(rosetta): submodule-qualified names produce invalid Go, Java

Java and Go do not support transitive access to a submodule's items via a qualified reference and instead require a dedicated import to be emitted. This adds code to analyze use of imported symbols to detect when a namespace traversal occurs, to inject new imports & replace property access expressions as necessary. In order to facilitate this work, these extra elements were done: - Replaced if-tree in the dispatcher with a switch statement, making the dispatch a lot faster, and also a lot nice to run through in a debugger - Emit `var` instead of a type name for variable declarations with an initializer in C#, to reduce "invalid" code generated in cases where the type name needs some qualification that we are missing - A couple of bug fixes here and there as the tests discovered them Fixes #3551
aws · Jan 25, 2023 · 4a4af55 · 4a4af55
1 parent a9f025f
commit 4a4af55
Show file tree

Hide file tree

Showing 40 changed files with 719 additions and 253 deletions.
diff --git a/packages/jsii-rosetta/jest.config.mjs b/packages/jsii-rosetta/jest.config.mjs
@@ -4,4 +4,5 @@ import { default as defaultConfig, overriddenConfig } from '../../jest.config.mj
 export default overriddenConfig({
   setupFiles: [createRequire(import.meta.url).resolve('./jestsetup.js')],
   testTimeout: process.env.CI === 'true' ? 30_000 : defaultConfig.testTimeout,
+  watchPathIgnorePatterns: ['(\\.d)?\\.tsx?$'],
 });
diff --git a/packages/jsii-rosetta/lib/languages/csharp.ts b/packages/jsii-rosetta/lib/languages/csharp.ts
@@ -140,7 +140,7 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
     const namespace = fmap(importStatement.moduleSymbol, findDotnetName) ?? guessedNamespace;
 
     if (importStatement.imports.import === 'full') {
-      this.dropPropertyAccesses.add(importStatement.imports.alias);
+      this.dropPropertyAccesses.add(importStatement.imports.sourceName);
       this.alreadyImportedNamespaces.add(namespace);
       return new OTree([`using ${namespace};`], [], { canBreakLine: true });
     }
@@ -563,34 +563,34 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
   }
 
   public variableDeclaration(node: ts.VariableDeclaration, renderer: CSharpRenderer): OTree {
-    let fallback = 'var';
-    if (node.type) {
-      fallback = node.type.getText();
-    }
+    let typeOrVar = 'var';
 
+    const fallback = node.type?.getText() ?? 'var';
     const type =
-      (node.type && renderer.typeOfType(node.type)) ||
+      (node.type && renderer.typeOfType(node.type)) ??
       (node.initializer && renderer.typeOfExpression(node.initializer));
 
-    let renderedType = this.renderType(node, type, false, fallback, renderer);
-    if (renderedType === 'object') {
-      renderedType = 'var';
+    const varType = this.renderType(node, type, false, fallback, renderer);
+    // If there is an initializer, and the value isn't "IDictionary<...", we always use var, as this is the
+    // recommendation from Roslyn.
+    if (varType !== 'object' && (varType.startsWith('IDictionary<') || node.initializer == null)) {
+      typeOrVar = varType;
     }
 
     if (!node.initializer) {
-      return new OTree([renderedType, ' ', renderer.convert(node.name), ';'], []);
+      return new OTree([typeOrVar, ' ', renderer.convert(node.name), ';']);
     }
 
     return new OTree(
       [
-        renderedType,
+        typeOrVar,
         ' ',
         renderer.convert(node.name),
         ' = ',
         renderer.updateContext({ preferObjectLiteralAsStruct: false }).convert(node.initializer),
         ';',
       ],
-      [],
+      undefined,
       { canBreakLine: true },
     );
   }
@@ -715,7 +715,7 @@ function findDotnetName(jsiiSymbol: JsiiSymbol): string | undefined {
       }
     }
 
-    return `${recurse(namespaceName(fqn))}.${simpleName(jsiiSymbol.fqn)}`;
+    return `${recurse(namespaceName(fqn))}.${ucFirst(simpleName(jsiiSymbol.fqn))}`;
   }
 }
 

diff --git a/packages/jsii-rosetta/lib/languages/default.ts b/packages/jsii-rosetta/lib/languages/default.ts
@@ -4,10 +4,12 @@ import { analyzeObjectLiteral, ObjectLiteralStruct } from '../jsii/jsii-types';
 import { isNamedLikeStruct, isJsiiProtocolType } from '../jsii/jsii-utils';
 import { OTree, NO_SYNTAX } from '../o-tree';
 import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
+import { SubmoduleReference } from '../submodule-reference';
 import { voidExpressionString } from '../typescript/ast-utils';
 import { ImportStatement } from '../typescript/imports';
 
 import { TargetLanguage } from '.';
+import { inferredTypeOfExpression } from '../typescript/types';
 
 /**
  * A basic visitor that applies for most curly-braces-based languages
@@ -98,7 +100,11 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
     return this.notImplemented(node, context);
   }
 
-  public propertyAccessExpression(node: ts.PropertyAccessExpression, context: AstRenderer<C>): OTree {
+  public propertyAccessExpression(
+    node: ts.PropertyAccessExpression,
+    context: AstRenderer<C>,
+    _submoduleReference: SubmoduleReference | undefined,
+  ): OTree {
     return new OTree([context.convert(node.expression), '.', context.convert(node.name)]);
   }
 
@@ -169,7 +175,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
         : false,
     );
 
-    const inferredType = context.inferredTypeOfExpression(node);
+    const inferredType = inferredTypeOfExpression(context.typeChecker, node);
     if ((inferredType && isJsiiProtocolType(context.typeChecker, inferredType)) || anyMembersFunctions) {
       context.report(
         node,

diff --git a/packages/jsii-rosetta/lib/languages/go.ts b/packages/jsii-rosetta/lib/languages/go.ts
@@ -4,8 +4,10 @@ import { AssertionError } from 'assert';
 import * as ts from 'typescript';
 
 import { analyzeObjectLiteral, determineJsiiType, JsiiType, ObjectLiteralStruct } from '../jsii/jsii-types';
+import { lookupJsiiSymbolFromNode } from '../jsii/jsii-utils';
 import { OTree } from '../o-tree';
 import { AstRenderer } from '../renderer';
+import { SubmoduleReference } from '../submodule-reference';
 import { isExported, isPublic, isPrivate, isReadOnly, isStatic } from '../typescript/ast-utils';
 import { analyzeImportDeclaration, ImportStatement } from '../typescript/imports';
 import {
@@ -228,6 +230,15 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
             className: ucFirst(expr.name.text),
             classNamespace: renderer.updateContext({ isExported: false }).convert(expr.expression),
           };
+        } else if (
+          ts.isPropertyAccessExpression(expr.expression) &&
+          renderer.submoduleReferences.has(expr.expression)
+        ) {
+          const submodule = renderer.submoduleReferences.get(expr.expression)!;
+          return {
+            className: ucFirst(expr.name.text),
+            classNamespace: renderer.updateContext({ isExported: false }).convert(submodule.lastNode),
+          };
         }
         renderer.reportUnsupported(expr.expression, TargetLanguage.GO);
         return {
@@ -277,15 +288,17 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
           ? JSON.stringify(node.name.text)
           : this.goName(node.name.text, renderer, renderer.typeChecker.getSymbolAtLocation(node.name))
         : renderer.convert(node.name);
-    // Struct member values are always pointers...
     return new OTree(
       [
         key,
         ': ',
         renderer
           .updateContext({
-            wrapPtr: renderer.currentContext.isStruct || renderer.currentContext.inMapLiteral,
+            // Reset isExported, as this was intended for the key name translation...
+            isExported: undefined,
+            // Struct member values are always pointers...
             isPtr: renderer.currentContext.isStruct,
+            wrapPtr: renderer.currentContext.isStruct || renderer.currentContext.inMapLiteral,
           })
           .convert(node.initializer),
       ],
@@ -389,13 +402,14 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
     structType: ObjectLiteralStruct,
     renderer: GoRenderer,
   ): OTree {
+    const isExported = structType.kind === 'struct';
     return new OTree(
       [
         '&',
-        this.goName(structType.type.symbol.name, renderer.updateContext({ isPtr: false }), structType.type.symbol),
+        this.goName(structType.type.symbol.name, renderer.updateContext({ isExported, isPtr: false }), structType.type.symbol),
         '{',
       ],
-      renderer.updateContext({ isStruct: true }).convertAll(node.properties),
+      renderer.updateContext({ isExported, isStruct: true }).convertAll(node.properties),
       {
         suffix: '}',
         separator: ',',
@@ -444,16 +458,25 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
     return new OTree(['fmt.Println(', renderedArgs, ')']);
   }
 
-  public propertyAccessExpression(node: ts.PropertyAccessExpression, renderer: GoRenderer): OTree {
+  public propertyAccessExpression(
+    node: ts.PropertyAccessExpression,
+    renderer: GoRenderer,
+    submoduleReference?: SubmoduleReference,
+  ): OTree {
+    if (submoduleReference != null) {
+      return new OTree([renderer.updateContext({ isExported: false, isPtr: false, wrapPtr: false }).convert(submoduleReference.lastNode)]);
+    }
+
     const expressionType = typeOfExpression(renderer.typeChecker, node.expression);
     const valueSymbol = renderer.typeChecker.getSymbolAtLocation(node.name);
 
-    const isClassStaticMember =
+    const isStaticMember = valueSymbol?.valueDeclaration != null && isStatic(valueSymbol.valueDeclaration);
+    const isClassStaticPropertyAccess = isStaticMember &&
       expressionType?.symbol?.valueDeclaration != null &&
       ts.isClassDeclaration(expressionType.symbol.valueDeclaration) &&
-      valueSymbol?.valueDeclaration != null &&
-      ts.isPropertyDeclaration(valueSymbol.valueDeclaration) &&
-      isStatic(valueSymbol.valueDeclaration);
+      (ts.isPropertyDeclaration(valueSymbol.valueDeclaration) || ts.isAccessor(valueSymbol.valueDeclaration));
+    const isClassStaticMethodAccess = isStaticMember && !isClassStaticPropertyAccess &&
+      ts.isMethodDeclaration(valueSymbol.valueDeclaration);
 
     // When the expression has an unknown type (unresolved symbol), and has an upper-case first
     // letter, we assume it's a type name... In such cases, what comes after can be considered a
@@ -463,18 +486,22 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
       expressionType.symbol == null &&
       /(?:\.|^)[A-Z][^.]*$/.exec(node.expression.getText(node.expression.getSourceFile())) != null;
 
-    const isEnum =
+    // Whether the node is an enum member reference.
+    const isEnumMember =
       expressionType?.symbol?.valueDeclaration != null && ts.isEnumDeclaration(expressionType.symbol.valueDeclaration);
 
-    const delimiter = isEnum || isClassStaticMember || expressionLooksLikeTypeReference ? '_' : '.';
+    const jsiiSymbol = lookupJsiiSymbolFromNode(renderer.typeChecker, node.name);
+    const isExportedTypeName = jsiiSymbol != null && jsiiSymbol.symbolType !== 'module';
+
+    const delimiter = isEnumMember || isClassStaticPropertyAccess|| isClassStaticMethodAccess || expressionLooksLikeTypeReference ? '_' : '.';
 
     return new OTree([
       renderer.convert(node.expression),
       delimiter,
       renderer
-        .updateContext({ isExported: isClassStaticMember || expressionLooksLikeTypeReference || isEnum })
+        .updateContext({ isExported: isClassStaticPropertyAccess || isClassStaticMethodAccess || expressionLooksLikeTypeReference || isEnumMember || isExportedTypeName })
         .convert(node.name),
-      ...(isClassStaticMember
+      ...(isClassStaticPropertyAccess
         ? ['()']
         : // If the parent's not a call-like expression, and it's an inferred static property access, we need to put call
         // parentheses at the end, as static properties are accessed via synthetic readers.
@@ -578,8 +605,12 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
     return wrapPtrExpression(renderer.typeChecker, node, output);
   }
 
-  public stringLiteral(node: ts.StringLiteral, renderer: GoRenderer): OTree {
-    const text = JSON.stringify(node.text);
+  public stringLiteral(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral, renderer: GoRenderer): OTree {
+    // Go supports backtick-delimited multi-line string literals, similar/same as JavaScript no-substitution templates.
+    // We only use this trick if the literal includes actual new line characters (otherwise it just looks weird in go).
+    const text = ts.isNoSubstitutionTemplateLiteral(node) && /[\n\r]/m.test(node.text)
+      ? node.getText(node.getSourceFile())
+      : JSON.stringify(node.text);
 
     return new OTree([`${renderer.currentContext.wrapPtr ? jsiiStr(text) : text}`]);
   }
@@ -801,25 +832,36 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
   public importStatement(node: ImportStatement, renderer: AstRenderer<GoLanguageContext>): OTree {
     const packageName =
       node.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.packageName ??
-      this.goName(node.packageName, renderer, undefined);
+      node.packageName
+        // Special case namespaced npm package names, so they are mangled the same way pacmak does.
+        .replace(/@([a-z0-9_-]+)\/([a-z0-9_-])/, "$1$2")
+        .split('/')
+        .map((txt) => this.goName(txt, renderer, undefined))
+        .filter((txt) => txt !== '')
+        .join('/');
     const moduleName = node.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.moduleName
       ? `${node.moduleSymbol.sourceAssembly.packageJson.jsii.targets.go.moduleName}/${packageName}`
       : `github.com/aws-samples/dummy/${packageName}`;
 
     if (node.imports.import === 'full') {
+      // We don't emit the alias if it matches the last path segment (conventionally this is the package name)
+      const maybeAlias = node.imports.alias
+        ? `${this.goName(node.imports.alias, renderer, undefined)} `
+        : '';
+
       return new OTree(
-        ['import ', this.goName(node.imports.alias, renderer, undefined), ' "', moduleName, '"'],
+        [`import ${maybeAlias}${JSON.stringify(moduleName)}`],
         undefined,
         { canBreakLine: true },
       );
     }
 
     if (node.imports.elements.length === 0) {
       // This is a blank import (for side-effects only)
-      return new OTree(['import _ "', moduleName, '"'], undefined, { canBreakLine: true });
+      return new OTree([`import _ ${JSON.stringify(moduleName)}`], undefined, { canBreakLine: true });
     }
 
-    return new OTree(['import "', moduleName, '"'], undefined, { canBreakLine: true });
+    return new OTree([`import ${JSON.stringify(moduleName)}`], undefined, { canBreakLine: true });
   }
 
   public variableDeclaration(node: ts.VariableDeclaration, renderer: AstRenderer<GoLanguageContext>): OTree {

diff --git a/packages/jsii-rosetta/lib/languages/java.ts b/packages/jsii-rosetta/lib/languages/java.ts
@@ -6,6 +6,7 @@ import { jsiiTargetParameter } from '../jsii/packages';
 import { TargetLanguage } from '../languages/target-language';
 import { OTree, NO_SYNTAX } from '../o-tree';
 import { AstRenderer } from '../renderer';
+import { SubmoduleReference } from '../submodule-reference';
 import { isReadOnly, matchAst, nodeOfType, quoteStringLiteral, visibility } from '../typescript/ast-utils';
 import { ImportStatement } from '../typescript/imports';
 import { isEnumAccess, isStaticReadonlyAccess, determineReturnType } from '../typescript/types';
@@ -121,9 +122,8 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
 
   public importStatement(importStatement: ImportStatement): OTree {
     const guessedNamespace = guessJavaNamespaceName(importStatement.packageName);
-
     if (importStatement.imports.import === 'full') {
-      this.dropPropertyAccesses.add(importStatement.imports.alias);
+      this.dropPropertyAccesses.add(importStatement.imports.sourceName);
       const namespace = fmap(importStatement.moduleSymbol, findJavaName) ?? guessedNamespace;
 
       return new OTree([`import ${namespace}.*;`], [], { canBreakLine: true });
@@ -132,7 +132,12 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
     const imports = importStatement.imports.elements.map((e) => {
       const fqn = fmap(e.importedSymbol, findJavaName) ?? `${guessedNamespace}.${e.sourceName}`;
 
-      return e.importedSymbol?.symbolType === 'module' ? `import ${fqn}.*;` : `import ${fqn};`;
+      // If there is no imported symbol, we check if there is anything looking like a type name in
+      // the source name (that is, any segment that starts with an upper case letter), and if none
+      // is found, assume this refers to a namespace/module.
+      return (e.importedSymbol?.symbolType == null && !e.sourceName.split('.').some((segment) => /^[A-Z]/.test(segment))) || e.importedSymbol?.symbolType === 'module'
+        ? `import ${fqn}.*;`
+        : `import ${fqn};`;
     });
 
     const localNames = importStatement.imports.elements
@@ -548,8 +553,13 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
       : this.singlePropertyInJavaScriptObjectLiteralToFluentSetters(node.name, node.name, renderer);
   }
 
-  public propertyAccessExpression(node: ts.PropertyAccessExpression, renderer: JavaRenderer): OTree {
+  public propertyAccessExpression(node: ts.PropertyAccessExpression, renderer: JavaRenderer, submoduleRef: SubmoduleReference | undefined): OTree {
     const rightHandSide = renderer.convert(node.name);
+    // If a submodule access, then just render the name, we emitted a * import of the expression segment already.
+    if (submoduleRef != null) {
+      return rightHandSide;
+    }
+
     let parts: Array<OTree | string | undefined>;
 
     const leftHandSide = renderer.textOf(node.expression);
@@ -860,7 +870,10 @@ function findJavaName(jsiiSymbol: JsiiSymbol): string | undefined {
       }
     }
 
-    return `${recurse(namespaceName(fqn))}.${simpleName(jsiiSymbol.fqn)}`;
+    const ns = namespaceName(fqn);
+    const nsJavaName = recurse(ns);
+    const leaf = simpleName(fqn);
+    return `${nsJavaName}.${leaf}`;
   }
 }
 

diff --git a/packages/jsii-rosetta/lib/languages/python.ts b/packages/jsii-rosetta/lib/languages/python.ts
@@ -15,6 +15,7 @@ import { jsiiTargetParameter } from '../jsii/packages';
 import { TargetLanguage } from '../languages/target-language';
 import { NO_SYNTAX, OTree, renderTree } from '../o-tree';
 import { AstRenderer, nimpl, CommentSyntax } from '../renderer';
+import { SubmoduleReference } from '../submodule-reference';
 import {
   matchAst,
   nodeOfType,
@@ -168,12 +169,13 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
     if (node.imports.import === 'full') {
       const moduleName = fmap(node.moduleSymbol, findPythonName) ?? guessPythonPackageName(node.packageName);
 
+      const importName = node.imports.alias ?? node.imports.sourceName;
       this.addImport({
         importedFqn: node.moduleSymbol?.fqn ?? node.packageName,
-        importName: node.imports.alias,
+        importName,
       });
 
-      return new OTree([`import ${moduleName} as ${mangleIdentifier(node.imports.alias)}`], [], {
+      return new OTree([`import ${moduleName} as ${mangleIdentifier(importName)}`], [], {
         canBreakLine: true,
       });
     }
@@ -325,7 +327,11 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
     );
   }
 
-  public propertyAccessExpression(node: ts.PropertyAccessExpression, context: PythonVisitorContext) {
+  public propertyAccessExpression(
+    node: ts.PropertyAccessExpression,
+    context: PythonVisitorContext,
+    submoduleReference: SubmoduleReference | undefined,
+  ) {
     const fullText = context.textOf(node);
     if (fullText in BUILTIN_FUNCTIONS) {
       return new OTree([BUILTIN_FUNCTIONS[fullText]]);
@@ -339,7 +345,11 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
       return context.convert(node.name);
     }
 
-    return super.propertyAccessExpression(node, context);
+    if (submoduleReference != null) {
+      return context.convert(submoduleReference.lastNode);
+    }
+
+    return super.propertyAccessExpression(node, context, submoduleReference);
   }
 
   public parameterDeclaration(node: ts.ParameterDeclaration, context: PythonVisitorContext): OTree {