From a833a1d2f1335e9447fd5702b9e4f9c6166abe67 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 31 Dec 2021 10:54:03 +0100 Subject: [PATCH] fix(rosetta): Python translation for `implements` is wrong (#3280) For Python, interface implementation needs to be written as `@jsii.implements(Iface)`. Also add a check that people don't put functions in object literals. Fixes aws/aws-cdk#17928 --- 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 --- packages/jsii-rosetta/lib/jsii/jsii-utils.ts | 75 +++++++++++++++++++ .../jsii-rosetta/lib/languages/default.ts | 39 +++++++++- packages/jsii-rosetta/lib/languages/python.ts | 22 ++++-- packages/jsii-rosetta/test/translate.test.ts | 38 ++++++++++ .../class_implementing_jsii_interface.cs | 7 ++ .../class_implementing_jsii_interface.java | 5 ++ .../class_implementing_jsii_interface.py | 4 + .../class_implementing_jsii_interface.ts | 12 +++ 8 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.cs create mode 100644 packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.java create mode 100644 packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.py create mode 100644 packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.ts diff --git a/packages/jsii-rosetta/lib/jsii/jsii-utils.ts b/packages/jsii-rosetta/lib/jsii/jsii-utils.ts index ec5e845093..45e94100a2 100644 --- a/packages/jsii-rosetta/lib/jsii/jsii-utils.ts +++ b/packages/jsii-rosetta/lib/jsii/jsii-utils.ts @@ -1,3 +1,4 @@ +import * as spec from '@jsii/spec'; import { symbolIdentifier } from 'jsii'; import * as ts from 'typescript'; @@ -29,6 +30,60 @@ export function analyzeStructType(typeChecker: ts.TypeChecker, type: ts.Type): O return { kind: 'local-struct', type }; } +/** + * Whether the given type is a protocol AND comes from jsii + * + * - Protocol: a TypeScript interface that is *not* a "struct" type. + * A.k.a. "behavioral interface". + * - From jsii: whether the interface type is defined in and exported + * via a jsii assembly. There can be literal interfaces defined + * in an example, and they will not be mangled in the same way + * as a jsii interface would be. + * + * + * Examples: + * + * ```ts + * // isJsiiProtocolType() -> false: not a protocol + * interface Banana { + * readonly arc: number; + * } + * + * // isJsiiProtocolType() -> might be true: depends on whether it was defined + * // in a jsii assembly. + * interface IHello { + * sayIt(): void; + * } + * + * // isJsiiProtocolType() -> false: declared to not be a protocol, even though + * // it has the naming scheme of one + * /** + * * @struct + * * / + * interface IPAddress { + * readonly octets: number[]; + * } + * ``` + */ +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(flags: A, test: A) { // tslint:disable-next-line:no-bitwise return test !== 0 && (flags & test) === test; @@ -100,6 +155,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 * diff --git a/packages/jsii-rosetta/lib/languages/default.ts b/packages/jsii-rosetta/lib/languages/default.ts index dc6a83fd2f..bc82fde164 100644 --- a/packages/jsii-rosetta/lib/languages/default.ts +++ b/packages/jsii-rosetta/lib/languages/default.ts @@ -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'; @@ -153,6 +153,22 @@ export abstract class DefaultVisitor implements AstHandler { 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) { + context.report( + node, + `You cannot use an object literal to make an instance of an interface. Define a class instead.`, + ); + } + const lit = analyzeObjectLiteral(context.typeChecker, node); switch (lit.kind) { @@ -342,3 +358,24 @@ const UNARY_OPS: { [op in ts.PrefixUnaryOperator]: string } = { [ts.SyntaxKind.TildeToken]: '~', [ts.SyntaxKind.ExclamationToken]: '!', }; + +/** + * Whether the given expression evaluates to a value that is of type "function" + * + * Examples of function types: + * + * ```ts + * // GIVEN + * function someFunction() { } + * + * // THEN + * const x = someFunction; // <- function type + * const y = () => 42; // <- function type + * const z = x; // <- function type + * Array.isArray; // <- function type + * ``` + */ +function isExpressionOfFunctionType(typeChecker: ts.TypeChecker, expr: ts.Expression) { + const type = typeChecker.getTypeAtLocation(expr).getNonNullableType(); + return type.getCallSignatures().length > 0; +} diff --git a/packages/jsii-rosetta/lib/languages/python.ts b/packages/jsii-rosetta/lib/languages/python.ts index 73b0c434dc..24d903469c 100644 --- a/packages/jsii-rosetta/lib/languages/python.ts +++ b/packages/jsii-rosetta/lib/languages/python.ts @@ -9,6 +9,7 @@ import { JsiiSymbol, simpleName, namespaceName, + isJsiiProtocolType, } from '../jsii/jsii-utils'; import { jsiiTargetParameter } from '../jsii/packages'; import { TargetLanguage } from '../languages/target-language'; @@ -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 { @@ -102,7 +103,7 @@ export class PythonVisitor extends DefaultVisitor { * Bump this when you change something in the implementation to invalidate * existing cached translations. */ - public static readonly VERSION = '1'; + public static readonly VERSION = '2'; public readonly language = TargetLanguage.PYTHON; public readonly defaultContext = {}; @@ -532,10 +533,18 @@ export class PythonVisitor extends DefaultVisitor { } 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) { @@ -544,10 +553,11 @@ export class PythonVisitor extends DefaultVisitor { 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 ? ')' : '', ': ', ], diff --git a/packages/jsii-rosetta/test/translate.test.ts b/packages/jsii-rosetta/test/translate.test.ts index 7643b9caab..a42b6b7bf7 100644 --- a/packages/jsii-rosetta/test/translate.test.ts +++ b/packages/jsii-rosetta/test/translate.test.ts @@ -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/), + }), + ); +}); diff --git a/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.cs b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.cs new file mode 100644 index 0000000000..4b59e581bf --- /dev/null +++ b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.cs @@ -0,0 +1,7 @@ +class MyClass : IResolvable +{ + public object Resolve() + { + return 42; + } +} \ No newline at end of file diff --git a/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.java b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.java new file mode 100644 index 0000000000..fd8db8e96b --- /dev/null +++ b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.java @@ -0,0 +1,5 @@ +public class MyClass implements IResolvable { + public Object resolve() { + return 42; + } +} \ No newline at end of file diff --git a/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.py b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.py new file mode 100644 index 0000000000..a51997c53d --- /dev/null +++ b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.py @@ -0,0 +1,4 @@ +@jsii.implements(IResolvable) +class MyClass: + def resolve(self): + return 42 \ No newline at end of file diff --git a/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.ts b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.ts new file mode 100644 index 0000000000..611ec90dd8 --- /dev/null +++ b/packages/jsii-rosetta/test/translations/classes/class_implementing_jsii_interface.ts @@ -0,0 +1,12 @@ +/// !hide +/// fake-from-jsii +interface IResolvable { + resolve(): any; +} +/// !show + +class MyClass implements IResolvable { + public resolve(): any { + return 42; + } +} \ No newline at end of file