Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rosetta): Python translation for implements is wrong #3280

Merged
merged 5 commits into from
Dec 31, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/jsii-rosetta/lib/jsii/jsii-utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as spec from '@jsii/spec';
import { symbolIdentifier } from 'jsii';
import * as ts from 'typescript';

Expand Down Expand Up @@ -29,6 +30,25 @@ export function analyzeStructType(typeChecker: ts.TypeChecker, type: ts.Type): O
return { kind: 'local-struct', type };
}

export function isJsiiProtocolType(typeChecker: ts.TypeChecker, type: ts.Type): boolean | undefined {
if (!type.isClassOrInterface() || !hasAllFlags(type.objectFlags, ts.ObjectFlags.Interface)) {
return false;
}

const sym = lookupJsiiSymbol(typeChecker, type.symbol);
if (!sym) {
return false;
}

if (!sym.sourceAssembly) {
// No source assembly, so this is a 'fake-from-jsii' type
return !isNamedLikeStruct(type.symbol.name);
}

const jsiiType = resolveJsiiSymbolType(sym);
return spec.isInterfaceType(jsiiType) && !jsiiType.datatype;
}

export function hasAllFlags<A extends number>(flags: A, test: A) {
// tslint:disable-next-line:no-bitwise
return test !== 0 && (flags & test) === test;
Expand Down Expand Up @@ -100,6 +120,26 @@ export function lookupJsiiSymbolFromNode(typeChecker: ts.TypeChecker, node: ts.N
return fmap(typeChecker.getSymbolAtLocation(node), (s) => lookupJsiiSymbol(typeChecker, s));
}

export function resolveJsiiSymbolType(jsiiSymbol: JsiiSymbol): spec.Type {
if (jsiiSymbol.symbolType !== 'type') {
throw new Error(
`Expected symbol to refer to a 'type', got '${jsiiSymbol.fqn}' which is a '${jsiiSymbol.symbolType}'`,
);
}

if (!jsiiSymbol.sourceAssembly) {
throw new Error('`resolveJsiiSymbolType: requires an actual source assembly');
}

const type = jsiiSymbol.sourceAssembly?.assembly.types?.[jsiiSymbol.fqn];
if (!type) {
throw new Error(
`resolveJsiiSymbolType: ${jsiiSymbol.fqn} not found in assembly ${jsiiSymbol.sourceAssembly.assembly.name}`,
);
}
return type;
}

/**
* Returns the jsii FQN for a TypeScript (class or type) symbol
*
Expand Down
23 changes: 22 additions & 1 deletion packages/jsii-rosetta/lib/languages/default.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as ts from 'typescript';

import { analyzeObjectLiteral, ObjectLiteralStruct } from '../jsii/jsii-types';
import { isNamedLikeStruct } from '../jsii/jsii-utils';
import { isNamedLikeStruct, isJsiiProtocolType } from '../jsii/jsii-utils';
import { OTree, NO_SYNTAX } from '../o-tree';
import { AstRenderer, AstHandler, nimpl, CommentSyntax } from '../renderer';
import { voidExpressionString } from '../typescript/ast-utils';
Expand Down Expand Up @@ -153,6 +153,22 @@ export abstract class DefaultVisitor<C> implements AstHandler<C> {
context.report(unsup, `Use of ${ts.SyntaxKind[unsup.kind]} in an object literal is not supported.`);
}

const anyMembersFunctions = node.properties.some((p) =>
ts.isPropertyAssignment(p)
? isExpressionOfFunctionType(context.typeChecker, p.initializer)
: ts.isShorthandPropertyAssignment(p)
? isExpressionOfFunctionType(context.typeChecker, p.name)
: false,
);

const inferredType = context.inferredTypeOfExpression(node);
if ((inferredType && isJsiiProtocolType(context.typeChecker, inferredType)) || anyMembersFunctions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this checking for? I understand the anyMembersFunctions piece, but don't understand what isJsiiProtocolType is checking for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Is this type a non-struct interface type that comes from jsii"

  • non-struct interface: we use interfaces for 2 purposes, and we constantly have to make distinctions between them. The terms I commonly use are "structs" and "protocols" (though we also use data types for the first, and "behavioral interfaces" for the second).
  • From jsii: you can also define interfaces straight up in the example itself, and those are not subject to the rewrite. Therefore this code also checks if the interface comes from a jsii-ified library.

context.report(
node,
`You cannot use an object literal to make an instance of an interface. Define a class instead.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for most languages, but do python users know to use the decorator @jsii.implements(IXxx) in these situations? Perhaps they do, but I wonder if we should be documenting this Python-specific use case more.

I guess a correct example translation in Python will go a long ways in documenting that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is intended for example authors, who are authoring in TypeScript. At that point, we're not even talking about Python yet.

(By the way, the same limitations [sorta] holds for C#, and technically it would be possible to define and instantiate an anonymous class implementing an interface in Java, but Rosetta doesn't support it. There's also no point to supporting it in Java as C# and Python won't be able to do the same trick)

);
}

const lit = analyzeObjectLiteral(context.typeChecker, node);

switch (lit.kind) {
Expand Down Expand Up @@ -342,3 +358,8 @@ const UNARY_OPS: { [op in ts.PrefixUnaryOperator]: string } = {
[ts.SyntaxKind.TildeToken]: '~',
[ts.SyntaxKind.ExclamationToken]: '!',
};

function isExpressionOfFunctionType(typeChecker: ts.TypeChecker, expr: ts.Expression) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document this function and isJsiiProtocolType with an example? On the one hand I can reason about the function name and what kind of type it is checking for. On the other hand, it would be super helpful to see an example of what evaluates to an expressionOfFunction or a jsiiProtocol. There are a lot of different types and minutiae involved in evaluating types, and I think adding examples for some of the less obvious checks would be great.

For example, an expression of function is something like const fn = () => 42; right? And you defined what a Jsii protocol is in an earlier comment. I think its helpful to be that explicit in the code documentation too.

const type = typeChecker.getTypeAtLocation(expr).getNonNullableType();
return type.getCallSignatures().length > 0;
}
22 changes: 16 additions & 6 deletions packages/jsii-rosetta/lib/languages/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
JsiiSymbol,
simpleName,
namespaceName,
isJsiiProtocolType,
} from '../jsii/jsii-utils';
import { jsiiTargetParameter } from '../jsii/packages';
import { TargetLanguage } from '../languages/target-language';
Expand All @@ -23,7 +24,7 @@ import {
} from '../typescript/ast-utils';
import { ImportStatement } from '../typescript/imports';
import { parameterAcceptsUndefined } from '../typescript/types';
import { startsWithUppercase, flat, sortBy, groupBy, fmap } from '../util';
import { startsWithUppercase, sortBy, groupBy, fmap } from '../util';
import { DefaultVisitor } from './default';

interface StructVar {
Expand Down Expand Up @@ -102,7 +103,7 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
* Bump this when you change something in the implementation to invalidate
* existing cached translations.
*/
public static readonly VERSION = '1';
public static readonly VERSION = '2';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just like a sanity check here for something I think is okay. We ignore language versioning for infused snippets in #3291. But that is okay because we infuse each time and there is no way the version changes between the time we infuse and extract in run-rosetta, right? Or am I missing some edge case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, the version is not going to change.


public readonly language = TargetLanguage.PYTHON;
public readonly defaultContext = {};
Expand Down Expand Up @@ -532,10 +533,18 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {
}

public classDeclaration(node: ts.ClassDeclaration, context: PythonVisitorContext): OTree {
const heritage = flat(Array.from(node.heritageClauses ?? []).map((h) => Array.from(h.types))).map((t) =>
context.convert(t.expression),
const allHeritageClauses = Array.from(node.heritageClauses ?? []).flatMap((h) => Array.from(h.types));

// List of booleans matching `allHeritage` array
const isJsii = allHeritageClauses.map(
(e) =>
fmap(context.typeOfExpression(e.expression), (type) => isJsiiProtocolType(context.typeChecker, type)) ?? false,
);
const hasHeritage = heritage.length > 0;

const jsiiImplements = allHeritageClauses.filter((_, i) => isJsii[i]);

const inlineHeritage = allHeritageClauses.filter((_, i) => !isJsii[i]);
const hasHeritage = inlineHeritage.length > 0;

const members = context.updateContext({ inClass: true }).convertAll(node.members);
if (members.length === 0) {
Expand All @@ -544,10 +553,11 @@ export class PythonVisitor extends DefaultVisitor<PythonLanguageContext> {

const ret = new OTree(
[
...jsiiImplements.flatMap((i) => ['@jsii.implements(', context.convert(i.expression), ')\n']),
'class ',
node.name ? context.textOf(node.name) : '???',
hasHeritage ? '(' : '',
...heritage,
...inlineHeritage.map((t) => context.convert(t.expression)),
hasHeritage ? ')' : '',
': ',
],
Expand Down
38 changes: 38 additions & 0 deletions packages/jsii-rosetta/test/translate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,41 @@ test('didSuccessfullyCompile is undefined when compilation is not attempted', ()

expect(subject.didSuccessfullyCompile).toBeUndefined();
});

test('refuse to translate object literal with function member', () => {
const visibleSource = 'const x: any = { mem: () => 42 };';

const snippet: TypeScriptSnippet = {
visibleSource,
location: { api: { api: 'type', fqn: 'my.class' } },
};

// WHEN
const subject = new SnippetTranslator(snippet);
subject.renderUsing(new PythonVisitor());

expect(subject.diagnostics).toContainEqual(
expect.objectContaining({
messageText: expect.stringMatching(/You cannot use an object literal/),
}),
);
});

test('refuse to translate object literal with function member in shorthand property', () => {
const visibleSource = 'const mem = () => 42; const x: any = { mem };';

const snippet: TypeScriptSnippet = {
visibleSource,
location: { api: { api: 'type', fqn: 'my.class' } },
};

// WHEN
const subject = new SnippetTranslator(snippet);
subject.renderUsing(new PythonVisitor());

expect(subject.diagnostics).toContainEqual(
expect.objectContaining({
messageText: expect.stringMatching(/You cannot use an object literal/),
}),
);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class MyClass : IResolvable
{
public object Resolve()
{
return 42;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
public class MyClass implements IResolvable {
public Object resolve() {
return 42;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@jsii.implements(IResolvable)
class MyClass:
def resolve(self):
return 42
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// !hide
/// fake-from-jsii
interface IResolvable {
resolve(): any;
}
/// !show

class MyClass implements IResolvable {
public resolve(): any {
return 42;
}
}