Skip to content

Commit 1ff230d

Browse files
authored
fix(rosetta): submodule-qualified names produce invalid Go, Java (#3928)
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 --- > **Note**: > There are still some awkward issues with the rendered code, but those are significantly more difficult to tackle without profoundly refactoring `jsii-rosetta`... The current work is already chunky enough and I didn't want to make it worse. > - Some synthetic type references lack qualification (e.g: struct type names in Go & C#) > - There might still be edge cases where duplicated or unused imports are emitted > - Import paths (in Go) may not be computed correctly (this might not be too hard to address, though) --- 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
1 parent 49136ac commit 1ff230d

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

42 files changed

+796
-279
lines changed

packages/jsii-pacmak/test/generated-code/__snapshots__/target-dotnet.test.js.snap

Lines changed: 8 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.js.snap

Lines changed: 8 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/jsii-rosetta/jest.config.mjs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,5 @@ import { default as defaultConfig, overriddenConfig } from '../../jest.config.mj
44
export default overriddenConfig({
55
setupFiles: [createRequire(import.meta.url).resolve('./jestsetup.js')],
66
testTimeout: process.env.CI === 'true' ? 30_000 : defaultConfig.testTimeout,
7+
watchPathIgnorePatterns: ['(\\.d)?\\.tsx?$'],
78
});

packages/jsii-rosetta/lib/languages/csharp.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
140140
const namespace = fmap(importStatement.moduleSymbol, findDotnetName) ?? guessedNamespace;
141141

142142
if (importStatement.imports.import === 'full') {
143-
this.dropPropertyAccesses.add(importStatement.imports.alias);
143+
this.dropPropertyAccesses.add(importStatement.imports.sourceName);
144144
this.alreadyImportedNamespaces.add(namespace);
145145
return new OTree([`using ${namespace};`], [], { canBreakLine: true });
146146
}
@@ -563,34 +563,34 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
563563
}
564564

565565
public variableDeclaration(node: ts.VariableDeclaration, renderer: CSharpRenderer): OTree {
566-
let fallback = 'var';
567-
if (node.type) {
568-
fallback = node.type.getText();
569-
}
566+
let typeOrVar = 'var';
570567

568+
const fallback = node.type?.getText() ?? 'var';
571569
const type =
572-
(node.type && renderer.typeOfType(node.type)) ||
570+
(node.type && renderer.typeOfType(node.type)) ??
573571
(node.initializer && renderer.typeOfExpression(node.initializer));
574572

575-
let renderedType = this.renderType(node, type, false, fallback, renderer);
576-
if (renderedType === 'object') {
577-
renderedType = 'var';
573+
const varType = this.renderType(node, type, false, fallback, renderer);
574+
// If there is an initializer, and the value isn't "IDictionary<...", we always use var, as this is the
575+
// recommendation from Roslyn.
576+
if (varType !== 'object' && (varType.startsWith('IDictionary<') || node.initializer == null)) {
577+
typeOrVar = varType;
578578
}
579579

580580
if (!node.initializer) {
581-
return new OTree([renderedType, ' ', renderer.convert(node.name), ';'], []);
581+
return new OTree([typeOrVar, ' ', renderer.convert(node.name), ';']);
582582
}
583583

584584
return new OTree(
585585
[
586-
renderedType,
586+
typeOrVar,
587587
' ',
588588
renderer.convert(node.name),
589589
' = ',
590590
renderer.updateContext({ preferObjectLiteralAsStruct: false }).convert(node.initializer),
591591
';',
592592
],
593-
[],
593+
undefined,
594594
{ canBreakLine: true },
595595
);
596596
}
@@ -715,7 +715,7 @@ function findDotnetName(jsiiSymbol: JsiiSymbol): string | undefined {
715715
}
716716
}
717717

718-
return `${recurse(namespaceName(fqn))}.${simpleName(jsiiSymbol.fqn)}`;
718+
return `${recurse(namespaceName(fqn))}.${ucFirst(simpleName(jsiiSymbol.fqn))}`;
719719
}
720720
}
721721

packages/jsii-rosetta/lib/languages/default.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import { analyzeObjectLiteral, ObjectLiteralStruct } from '../jsii/jsii-types';
44
import { isNamedLikeStruct, isJsiiProtocolType } from '../jsii/jsii-utils';
55
import { OTree, NO_SYNTAX } from '../o-tree';
66
import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
7+
import { SubmoduleReference } from '../submodule-reference';
78
import { voidExpressionString } from '../typescript/ast-utils';
89
import { ImportStatement } from '../typescript/imports';
10+
import { inferredTypeOfExpression } from '../typescript/types';
911

1012
import { TargetLanguage } from '.';
1113

@@ -98,7 +100,11 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
98100
return this.notImplemented(node, context);
99101
}
100102

101-
public propertyAccessExpression(node: ts.PropertyAccessExpression, context: AstRenderer<C>): OTree {
103+
public propertyAccessExpression(
104+
node: ts.PropertyAccessExpression,
105+
context: AstRenderer<C>,
106+
_submoduleReference: SubmoduleReference | undefined,
107+
): OTree {
102108
return new OTree([context.convert(node.expression), '.', context.convert(node.name)]);
103109
}
104110

@@ -169,7 +175,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
169175
: false,
170176
);
171177

172-
const inferredType = context.inferredTypeOfExpression(node);
178+
const inferredType = inferredTypeOfExpression(context.typeChecker, node);
173179
if ((inferredType && isJsiiProtocolType(context.typeChecker, inferredType)) || anyMembersFunctions) {
174180
context.report(
175181
node,

packages/jsii-rosetta/lib/languages/go.ts

Lines changed: 79 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import { AssertionError } from 'assert';
44
import * as ts from 'typescript';
55

66
import { analyzeObjectLiteral, determineJsiiType, JsiiType, ObjectLiteralStruct } from '../jsii/jsii-types';
7+
import { lookupJsiiSymbolFromNode } from '../jsii/jsii-utils';
78
import { OTree } from '../o-tree';
89
import { AstRenderer } from '../renderer';
10+
import { SubmoduleReference } from '../submodule-reference';
911
import { isExported, isPublic, isPrivate, isReadOnly, isStatic } from '../typescript/ast-utils';
1012
import { analyzeImportDeclaration, ImportStatement } from '../typescript/imports';
1113
import {
@@ -228,6 +230,15 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
228230
className: ucFirst(expr.name.text),
229231
classNamespace: renderer.updateContext({ isExported: false }).convert(expr.expression),
230232
};
233+
} else if (
234+
ts.isPropertyAccessExpression(expr.expression) &&
235+
renderer.submoduleReferences.has(expr.expression)
236+
) {
237+
const submodule = renderer.submoduleReferences.get(expr.expression)!;
238+
return {
239+
className: ucFirst(expr.name.text),
240+
classNamespace: renderer.updateContext({ isExported: false }).convert(submodule.lastNode),
241+
};
231242
}
232243
renderer.reportUnsupported(expr.expression, TargetLanguage.GO);
233244
return {
@@ -277,15 +288,17 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
277288
? JSON.stringify(node.name.text)
278289
: this.goName(node.name.text, renderer, renderer.typeChecker.getSymbolAtLocation(node.name))
279290
: renderer.convert(node.name);
280-
// Struct member values are always pointers...
281291
return new OTree(
282292
[
283293
key,
284294
': ',
285295
renderer
286296
.updateContext({
287-
wrapPtr: renderer.currentContext.isStruct || renderer.currentContext.inMapLiteral,
297+
// Reset isExported, as this was intended for the key name translation...
298+
isExported: undefined,
299+
// Struct member values are always pointers...
288300
isPtr: renderer.currentContext.isStruct,
301+
wrapPtr: renderer.currentContext.isStruct || renderer.currentContext.inMapLiteral,
289302
})
290303
.convert(node.initializer),
291304
],
@@ -389,13 +402,18 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
389402
structType: ObjectLiteralStruct,
390403
renderer: GoRenderer,
391404
): OTree {
405+
const isExported = structType.kind === 'struct';
392406
return new OTree(
393407
[
394408
'&',
395-
this.goName(structType.type.symbol.name, renderer.updateContext({ isPtr: false }), structType.type.symbol),
409+
this.goName(
410+
structType.type.symbol.name,
411+
renderer.updateContext({ isExported, isPtr: false }),
412+
structType.type.symbol,
413+
),
396414
'{',
397415
],
398-
renderer.updateContext({ isStruct: true }).convertAll(node.properties),
416+
renderer.updateContext({ isExported, isStruct: true }).convertAll(node.properties),
399417
{
400418
suffix: '}',
401419
separator: ',',
@@ -444,16 +462,30 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
444462
return new OTree(['fmt.Println(', renderedArgs, ')']);
445463
}
446464

447-
public propertyAccessExpression(node: ts.PropertyAccessExpression, renderer: GoRenderer): OTree {
465+
public propertyAccessExpression(
466+
node: ts.PropertyAccessExpression,
467+
renderer: GoRenderer,
468+
submoduleReference?: SubmoduleReference,
469+
): OTree {
470+
if (submoduleReference != null) {
471+
return new OTree([
472+
renderer
473+
.updateContext({ isExported: false, isPtr: false, wrapPtr: false })
474+
.convert(submoduleReference.lastNode),
475+
]);
476+
}
477+
448478
const expressionType = typeOfExpression(renderer.typeChecker, node.expression);
449479
const valueSymbol = renderer.typeChecker.getSymbolAtLocation(node.name);
450480

451-
const isClassStaticMember =
481+
const isStaticMember = valueSymbol?.valueDeclaration != null && isStatic(valueSymbol.valueDeclaration);
482+
const isClassStaticPropertyAccess =
483+
isStaticMember &&
452484
expressionType?.symbol?.valueDeclaration != null &&
453485
ts.isClassDeclaration(expressionType.symbol.valueDeclaration) &&
454-
valueSymbol?.valueDeclaration != null &&
455-
ts.isPropertyDeclaration(valueSymbol.valueDeclaration) &&
456-
isStatic(valueSymbol.valueDeclaration);
486+
(ts.isPropertyDeclaration(valueSymbol.valueDeclaration) || ts.isAccessor(valueSymbol.valueDeclaration));
487+
const isClassStaticMethodAccess =
488+
isStaticMember && !isClassStaticPropertyAccess && ts.isMethodDeclaration(valueSymbol.valueDeclaration);
457489

458490
// When the expression has an unknown type (unresolved symbol), and has an upper-case first
459491
// letter, we assume it's a type name... In such cases, what comes after can be considered a
@@ -463,18 +495,32 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
463495
expressionType.symbol == null &&
464496
/(?:\.|^)[A-Z][^.]*$/.exec(node.expression.getText(node.expression.getSourceFile())) != null;
465497

466-
const isEnum =
498+
// Whether the node is an enum member reference.
499+
const isEnumMember =
467500
expressionType?.symbol?.valueDeclaration != null && ts.isEnumDeclaration(expressionType.symbol.valueDeclaration);
468501

469-
const delimiter = isEnum || isClassStaticMember || expressionLooksLikeTypeReference ? '_' : '.';
502+
const jsiiSymbol = lookupJsiiSymbolFromNode(renderer.typeChecker, node.name);
503+
const isExportedTypeName = jsiiSymbol != null && jsiiSymbol.symbolType !== 'module';
504+
505+
const delimiter =
506+
isEnumMember || isClassStaticPropertyAccess || isClassStaticMethodAccess || expressionLooksLikeTypeReference
507+
? '_'
508+
: '.';
470509

471510
return new OTree([
472511
renderer.convert(node.expression),
473512
delimiter,
474513
renderer
475-
.updateContext({ isExported: isClassStaticMember || expressionLooksLikeTypeReference || isEnum })
514+
.updateContext({
515+
isExported:
516+
isClassStaticPropertyAccess ||
517+
isClassStaticMethodAccess ||
518+
expressionLooksLikeTypeReference ||
519+
isEnumMember ||
520+
isExportedTypeName,
521+
})
476522
.convert(node.name),
477-
...(isClassStaticMember
523+
...(isClassStaticPropertyAccess
478524
? ['()']
479525
: // If the parent's not a call-like expression, and it's an inferred static property access, we need to put call
480526
// parentheses at the end, as static properties are accessed via synthetic readers.
@@ -578,8 +624,13 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
578624
return wrapPtrExpression(renderer.typeChecker, node, output);
579625
}
580626

581-
public stringLiteral(node: ts.StringLiteral, renderer: GoRenderer): OTree {
582-
const text = JSON.stringify(node.text);
627+
public stringLiteral(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral, renderer: GoRenderer): OTree {
628+
// Go supports backtick-delimited multi-line string literals, similar/same as JavaScript no-substitution templates.
629+
// We only use this trick if the literal includes actual new line characters (otherwise it just looks weird in go).
630+
const text =
631+
ts.isNoSubstitutionTemplateLiteral(node) && /[\n\r]/m.test(node.text)
632+
? node.getText(node.getSourceFile())
633+
: JSON.stringify(node.text);
583634

584635
return new OTree([`${renderer.currentContext.wrapPtr ? jsiiStr(text) : text}`]);
585636
}
@@ -801,25 +852,30 @@ export class GoVisitor extends DefaultVisitor<GoLanguageContext> {
801852
public importStatement(node: ImportStatement, renderer: AstRenderer<GoLanguageContext>): OTree {
802853
const packageName =
803854
node.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.packageName ??
804-
this.goName(node.packageName, renderer, undefined);
855+
node.packageName
856+
// Special case namespaced npm package names, so they are mangled the same way pacmak does.
857+
.replace(/@([a-z0-9_-]+)\/([a-z0-9_-])/, '$1$2')
858+
.split('/')
859+
.map((txt) => this.goName(txt, renderer, undefined))
860+
.filter((txt) => txt !== '')
861+
.join('/');
805862
const moduleName = node.moduleSymbol?.sourceAssembly?.packageJson.jsii?.targets?.go?.moduleName
806863
? `${node.moduleSymbol.sourceAssembly.packageJson.jsii.targets.go.moduleName}/${packageName}`
807864
: `github.com/aws-samples/dummy/${packageName}`;
808865

809866
if (node.imports.import === 'full') {
810-
return new OTree(
811-
['import ', this.goName(node.imports.alias, renderer, undefined), ' "', moduleName, '"'],
812-
undefined,
813-
{ canBreakLine: true },
814-
);
867+
// We don't emit the alias if it matches the last path segment (conventionally this is the package name)
868+
const maybeAlias = node.imports.alias ? `${this.goName(node.imports.alias, renderer, undefined)} ` : '';
869+
870+
return new OTree([`import ${maybeAlias}${JSON.stringify(moduleName)}`], undefined, { canBreakLine: true });
815871
}
816872

817873
if (node.imports.elements.length === 0) {
818874
// This is a blank import (for side-effects only)
819-
return new OTree(['import _ "', moduleName, '"'], undefined, { canBreakLine: true });
875+
return new OTree([`import _ ${JSON.stringify(moduleName)}`], undefined, { canBreakLine: true });
820876
}
821877

822-
return new OTree(['import "', moduleName, '"'], undefined, { canBreakLine: true });
878+
return new OTree([`import ${JSON.stringify(moduleName)}`], undefined, { canBreakLine: true });
823879
}
824880

825881
public variableDeclaration(node: ts.VariableDeclaration, renderer: AstRenderer<GoLanguageContext>): OTree {

0 commit comments

Comments
 (0)