Skip to content

Commit

Permalink
fix(rosetta): submodule-qualified names produce invalid Go, Java
Browse files Browse the repository at this point in the history
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
  • Loading branch information
RomainMuller committed Jan 25, 2023
1 parent a9f025f commit d1e0025
Show file tree
Hide file tree
Showing 40 changed files with 780 additions and 265 deletions.
1 change: 1 addition & 0 deletions packages/jsii-rosetta/jest.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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?$'],
});
26 changes: 13 additions & 13 deletions packages/jsii-rosetta/lib/languages/csharp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}
Expand Down Expand Up @@ -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 },
);
}
Expand Down Expand Up @@ -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))}`;
}
}

Expand Down
10 changes: 8 additions & 2 deletions packages/jsii-rosetta/lib/languages/default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ 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 { inferredTypeOfExpression } from '../typescript/types';

import { TargetLanguage } from '.';

Expand Down Expand Up @@ -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)]);
}

Expand Down Expand Up @@ -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,
Expand Down
102 changes: 79 additions & 23 deletions packages/jsii-rosetta/lib/languages/go.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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),
],
Expand Down Expand Up @@ -389,13 +402,18 @@ 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: ',',
Expand Down Expand Up @@ -444,16 +462,30 @@ 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
Expand All @@ -463,18 +495,32 @@ 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.
Expand Down Expand Up @@ -578,8 +624,13 @@ 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}`]);
}
Expand Down Expand Up @@ -801,25 +852,30 @@ 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') {
return new OTree(
['import ', this.goName(node.imports.alias, renderer, undefined), ' "', moduleName, '"'],
undefined,
{ canBreakLine: true },
);
// 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 ${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 {
Expand Down
29 changes: 24 additions & 5 deletions packages/jsii-rosetta/lib/languages/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 });
Expand All @@ -132,7 +132,14 @@ 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
Expand Down Expand Up @@ -548,8 +555,17 @@ 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);
Expand Down Expand Up @@ -860,7 +876,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}`;
}
}

Expand Down
Loading

0 comments on commit d1e0025

Please sign in to comment.