fix(rosetta): Python translation for implements is wrong#3280
fix(rosetta): Python translation for implements is wrong#3280mergify[bot] merged 5 commits intomainfrom
implements is wrong#3280Conversation
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
corymhall
left a comment
There was a problem hiding this comment.
So based on the linked issue the documentation for local bundling should be written as:
class LocalBundling implements cdk.ILocalBundling {
public tryBundle(outputDir: string, options: BundlingOptions) {
if (canRunLocally) {
// perform local bundling here
return true;
}
return false;
}
}
new assets.Asset(this, 'BundledAsset', {
path: '/path/to/asset',
bundling: {
local: LocalBundling,
},
});| ); | ||
|
|
||
| const inferredType = context.inferredTypeOfExpression(node); | ||
| if ((inferredType && isJsiiProtocolType(context.typeChecker, inferredType)) || anyMembersFunctions) { |
There was a problem hiding this comment.
What is this checking for? I understand the anyMembersFunctions piece, but don't understand what isJsiiProtocolType is checking for.
There was a problem hiding this comment.
"Is this type a non-struct interface type that comes from jsii"
- non-struct
interface: we useinterfaces 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.
Almost: new assets.Asset(this, 'BundledAsset', {
path: '/path/to/asset',
bundling: {
- local: LocalBundling,
+ local: new LocalBundling(),
},
}); |
kaizencc
left a comment
There was a problem hiding this comment.
I couldn't find anything to be up in arms about, so conditional approval it is. But I do think there is a lack of documentation as a whole that needs to be addressed.
| * existing cached translations. | ||
| */ | ||
| public static readonly VERSION = '1'; | ||
| public static readonly VERSION = '2'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Exactly, the version is not going to change.
| 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.`, |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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)
| [ts.SyntaxKind.ExclamationToken]: '!', | ||
| }; | ||
|
|
||
| function isExpressionOfFunctionType(typeChecker: ts.TypeChecker, expr: ts.Expression) { |
There was a problem hiding this comment.
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.
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
Merging (with squash)... |
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.