Skip to content

Commit

Permalink
fix(rosetta): literal map type is rendered as __object in C sharp (#3047
Browse files Browse the repository at this point in the history
)

(The diff of this PR will clean up after #3044 has been merged)

The error was in conflating "detecting a map type but not knowing
what the element type was" and "not detecting a map type" (these
cases could not be distinguished because both would result in
`undefined`).

Also remove an unnecessary argument to
`keyValueObjectLiteralExpression`.

Fixes #3026, fixes #3027.


---

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
  • Loading branch information
rix0rrr authored Oct 11, 2021
1 parent da55a1e commit e2843be
Show file tree
Hide file tree
Showing 10 changed files with 57 additions and 51 deletions.
9 changes: 7 additions & 2 deletions packages/jsii-rosetta/lib/jsii/jsii-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@ export function determineJsiiType(typeChecker: ts.TypeChecker, type: ts.Type): J
}

const mapValuesType = mapElementType(type, typeChecker);
if (mapValuesType) {
return { kind: 'map', elementType: determineJsiiType(typeChecker, mapValuesType) };
if (mapValuesType.result === 'map') {
return {
kind: 'map',
elementType: mapValuesType.elementType
? determineJsiiType(typeChecker, mapValuesType.elementType)
: { kind: 'builtIn', builtIn: 'any' },
};
}

// User-defined or aliased type
Expand Down
12 changes: 3 additions & 9 deletions packages/jsii-rosetta/lib/languages/csharp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
});
}
// Type information missing and from context we prefer a map
return this.keyValueObjectLiteralExpression(node, undefined, renderer);
return this.keyValueObjectLiteralExpression(node, renderer);
}

public knownStructObjectLiteralExpression(
Expand All @@ -424,15 +424,9 @@ export class CSharpVisitor extends DefaultVisitor<CSharpLanguageContext> {
});
}

public keyValueObjectLiteralExpression(
node: ts.ObjectLiteralExpression,
valueType: ts.Type | undefined,
renderer: CSharpRenderer,
): OTree {
public keyValueObjectLiteralExpression(node: ts.ObjectLiteralExpression, renderer: CSharpRenderer): OTree {
// Try to infer an element type from the elements
if (valueType === undefined) {
valueType = inferMapElementType(node.properties, renderer);
}
const valueType = inferMapElementType(node.properties, renderer.typeChecker);

return new OTree(
['new Dictionary<string, ', this.renderType(node, valueType, false, 'object', renderer), '> { '],
Expand Down
9 changes: 2 additions & 7 deletions packages/jsii-rosetta/lib/languages/default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { OTree, NO_SYNTAX } from '../o-tree';
import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
import { voidExpressionString } from '../typescript/ast-utils';
import { ImportStatement } from '../typescript/imports';
import { mapElementType } from '../typescript/types';

import { TargetLanguage } from '.';

Expand Down Expand Up @@ -162,7 +161,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
return this.knownStructObjectLiteralExpression(node, type!, context);
}
return this.keyValueObjectLiteralExpression(node, type && mapElementType(type, context.typeChecker), context);
return this.keyValueObjectLiteralExpression(node, context);
}

public unknownTypeObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: AstRenderer<C>): OTree {
Expand All @@ -177,11 +176,7 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
return this.notImplemented(node, context);
}

public keyValueObjectLiteralExpression(
node: ts.ObjectLiteralExpression,
_valueType: ts.Type | undefined,
context: AstRenderer<C>,
): OTree {
public keyValueObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: AstRenderer<C>): OTree {
return this.notImplemented(node, context);
}

Expand Down
8 changes: 2 additions & 6 deletions packages/jsii-rosetta/lib/languages/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,14 +452,10 @@ export class JavaVisitor extends DefaultVisitor<JavaContext> {
public unknownTypeObjectLiteralExpression(node: ts.ObjectLiteralExpression, renderer: JavaRenderer): OTree {
return renderer.currentContext.inNewExprWithObjectLiteralAsLastArg
? this.renderObjectLiteralAsBuilder(node, renderer)
: this.keyValueObjectLiteralExpression(node, undefined, renderer);
: this.keyValueObjectLiteralExpression(node, renderer);
}

public keyValueObjectLiteralExpression(
node: ts.ObjectLiteralExpression,
_valueType: ts.Type | undefined,
renderer: JavaRenderer,
): OTree {
public keyValueObjectLiteralExpression(node: ts.ObjectLiteralExpression, renderer: JavaRenderer): OTree {
return new OTree(['Map.of('], renderer.updateContext({ inKeyValueList: true }).convertAll(node.properties), {
suffix: ')',
separator: ', ',
Expand Down
6 changes: 1 addition & 5 deletions packages/jsii-rosetta/lib/languages/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,7 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
return this.renderObjectLiteralExpression(`${structType.symbol.name}(`, ')', true, node, context);
}

public keyValueObjectLiteralExpression(
node: ts.ObjectLiteralExpression,
_valueType: ts.Type | undefined,
context: PythonVisitorContext,
): OTree {
public keyValueObjectLiteralExpression(node: ts.ObjectLiteralExpression, context: PythonVisitorContext): OTree {
return this.renderObjectLiteralExpression('{', '}', false, node, context);
}

Expand Down
45 changes: 23 additions & 22 deletions packages/jsii-rosetta/lib/typescript/types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as ts from 'typescript';

import { hasAllFlags, hasAnyFlag } from '../jsii/jsii-utils';
import { AstRenderer } from '../renderer';

/**
* Return the first non-undefined type from a union
Expand Down Expand Up @@ -72,53 +71,55 @@ export function renderTypeFlags(type: ts.Type): string {
return renderFlags(type.flags, ts.TypeFlags);
}

export type MapAnalysis = { result: 'nonMap' } | { result: 'map'; elementType: ts.Type | undefined };

/**
* If this is a map type, return the type mapped *to* (key must always be `string` anyway).
*/
export function mapElementType(type: ts.Type, typeChecker: ts.TypeChecker): ts.Type | undefined {
if (type.flags & ts.TypeFlags.Object && type.symbol) {
export function mapElementType(type: ts.Type, typeChecker: ts.TypeChecker): MapAnalysis {
if (hasAnyFlag(type.flags, ts.TypeFlags.Object) && type.symbol) {
if (type.symbol.name === '__type') {
// Declared map type: {[k: string]: A}
return type.getStringIndexType();
return { result: 'map', elementType: type.getStringIndexType() };
}

if (type.symbol.name === '__object') {
// Derived map type from object literal: typeof({ k: "value" })
// For every property, get the node that created it (PropertyAssignment), and get the type of the initializer of that node
const initializerTypes = type.getProperties().map((p) => {
if (ts.isPropertyAssignment(p.valueDeclaration)) {
return typeOfExpression(typeChecker, p.valueDeclaration.initializer);
}
return undefined;
const expression = p.valueDeclaration;
return typeOfObjectLiteralProperty(typeChecker, expression);
});
return typeIfSame([...initializerTypes, type.getStringIndexType()]);
return {
result: 'map',
elementType: typeIfSame([...initializerTypes, type.getStringIndexType()].filter(isDefined)),
};
}
}

return undefined;
return { result: 'nonMap' };
}

/**
* Try to infer the map element type from the properties if they're all the same
*/
export function inferMapElementType(
elements: ts.NodeArray<ts.ObjectLiteralElementLike>,
renderer: AstRenderer<any>,
elements: readonly ts.ObjectLiteralElementLike[],
typeChecker: ts.TypeChecker,
): ts.Type | undefined {
const nodes = elements.map(elementValueNode).filter(isDefined);
const types = nodes.map((x) => renderer.typeOfExpression(x));
const types = elements.map((e) => typeOfObjectLiteralProperty(typeChecker, e)).filter(isDefined);

return types.every((t) => isSameType(types[0], t)) ? types[0] : undefined;
}

function elementValueNode(el: ts.ObjectLiteralElementLike): ts.Expression | undefined {
if (ts.isPropertyAssignment(el)) {
return el.initializer;
}
if (ts.isShorthandPropertyAssignment(el)) {
return el.name;
}
return undefined;
function typeOfObjectLiteralProperty(typeChecker: ts.TypeChecker, el: ts.Node): ts.Type | undefined {
if (ts.isPropertyAssignment(el)) {
return typeOfExpression(typeChecker, el.initializer);
}
if (ts.isShorthandPropertyAssignment(el)) {
return typeOfExpression(typeChecker, el.name);
}
return undefined;
}

function isSameType(a: ts.Type, b: ts.Type) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
IDictionary<string, object> expected = new Dictionary<string, object> {
{ "Foo", "Bar" },
{ "Baz", 5 },
{ "Qux", new [] { "Waldo", "Fred" } }
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Map<String, Object> expected = Map.of(
"Foo", "Bar",
"Baz", 5,
"Qux", List.of("Waldo", "Fred"));
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
expected = {
"Foo": "Bar",
"Baz": 5,
"Qux": ["Waldo", "Fred"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const expected = {
Foo: 'Bar',
Baz: 5,
Qux: [ 'Waldo', 'Fred' ],
};

0 comments on commit e2843be

Please sign in to comment.