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

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Dec 21, 2021

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.

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
@rix0rrr rix0rrr requested a review from a team December 21, 2021 14:54
@rix0rrr rix0rrr self-assigned this Dec 21, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 21, 2021
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

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) {
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.

@corymhall corymhall requested a review from a team December 23, 2021 14:22
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Dec 24, 2021

So based on the linked issue the documentation for local bundling should be written as:

Almost:

new assets.Asset(this, 'BundledAsset', {
  path: '/path/to/asset',
  bundling: {
-    local: LocalBundling,
+    local: new LocalBundling(),
  },
});

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

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.

@@ -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.

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.`,
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)

@@ -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.

@kaizencc kaizencc added the pr/do-not-merge This PR should not be merged at this time. label Dec 27, 2021
@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Dec 31, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 31, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Dec 31, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 31, 2021

Merging (with squash)...

@mergify mergify bot merged commit a833a1d into main Dec 31, 2021
@mergify mergify bot deleted the huijbers/jsii-implements branch December 31, 2021 09:54
@mergify
Copy link
Contributor

mergify bot commented Dec 31, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws_s3_assets): unable to define bundling with local (python)
3 participants