feat(kernel): annotate implemented interfaces on "ObjRef"s #825
Conversation
Annotating the interfaces implemented by an object instance with the outbound Object Reference, and allowing the kernel's "create" call to receive a list of interfaces allows the kernel to use more run-time type information and operate safer.
RomainMuller
added
effort/large
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiEngine.java
Show resolved
Hide resolved
| fqn: string; | ||
| /** | ||
| * The FQNs of interfaces the instance implements, if any. Declaring |
There was a problem hiding this comment.
Declaring interfaces that the class denoted by
fqnimplements is not necessary.
Maybe describe these as "additional" interfaces then? In addition to the interfaces implemented by the class.
There was a problem hiding this comment.
Yeah that's how I have started to name them. I'll have to go on the comments & make sure it's homogeneously used.
|
@rix0rrr - annotating from the active side is needed as well. This is still a draft PR as I'm working through the front-ends now... I wanted to have it out there at least so I don't lose anything if my hard disk crashes 😅 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
RomainMuller
requested review from
bmaizels,
garnaat and
a team
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
The target java changes are pretty benign and not worth commenting on as it's only annotation and some minor constructor implementation changes mainly.
Wrt/ the JsiiEngine changes I didn't see any unit tests for the changed/new functionality. Is there coverage for that code already, if so I'm surprised we didn't even break any existing unit tests.
|
@bmaizels - yup, the code was already mostly covered, and the tests that broke were un-broken by the updated runtime code. I'll still have a second pass over that to make sure the particular edge case this aims to solve is... indeed covered. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
There was indeed a bug hiding in there (both in Java & .NET) due to insufficient test coverage of the feature. This is now fixed (including test) |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
RomainMuller
removed
the
pr/do-not-merge
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
The change of behavior in `any` serialization introduced in #825 risks breaking some emerging use-cases of the AWS CDK that are likely to be prevalent in the installed user base. As a consequence, surgically reverting this particular change in order to restore the behavior that users might be dependent on. This somehow uncovered an outstanding bug in the Python runtime that was not triggering with the "reverted" behavior (it broke unit tests). The runtime was fixed in order to address this bug.
The change of behavior in `any` serialization introduced in #825 risks breaking some emerging use-cases of the AWS CDK that are likely to be prevalent in the installed user base. As a consequence, surgically reverting this particular change in order to restore the behavior that users might be dependent on. This somehow uncovered an outstanding bug in the Python runtime that was not triggering with the "reverted" behavior (it broke unit tests). The runtime was fixed in order to address this bug.
Annotating the interfaces implemented by an object instance with the
outbound Object Reference, and allowing the kernel's "create" call to
receive a list of interfaces allows the kernel to use more run-time type
information and operate safer.
All object references sent from JS to other languages are now annotated
with the relevant list of interfaces, and runtimes can dynamically convert
instances to the needed type (based on the static type that is needed at
the transfer point between the runtime library and user code).
In addition, a bug was fixed that prevented the Java runtime from
successfully receiving a reference that was typed
Object, which is thecase for anonymous objects returned from methods returning
any.Fixes #818
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.