fix(dotnet): fix callback issues #953
Conversation
Corrects an issue with handling callbacks in various situations: - When a callback for a method with optional parameters was invoked with some of the optional parameters not specified, an argument count mismatch error would occur. This is because dotnet reflection requires all arguments to be provided (with `null` if needed). - When a callback for a variadic method was invoked, a type mismatch or a parameter count mismatch error would occur. This is because the variadic parameter is an array of it's value type, and has to be passed this way out to the reflection call. - Objects would be created in Javascript without interface annotations, causing type information to be lost and incorrect callback argument serialization could happen in the Kernel. - When a class implements members from an interface (and only from one), the overrides would not be correctly identified, as the Attributes on interface members are *not* visible on the implementors (whereas they are visible when they were on an extended class member!). - Invalid type coertion could happen within the callback handling logic in certain edge cases. The complete type information is available, so switched to using the full conversion gear form the jsii runtime to achieve correct results. All of those issues are fairly inter-linked and it was difficult to fix them individually & issue a test that would not break due to another of the issues... Hence the fixes are grouped in a single commit.
|
I reckon the two added tests actually cover all the fixed up behavior; however if you disagree & have an idea of use-cases that aren't tested, please let me know what those are. I've been heads down into this for long enough that I may be blind-sighted at this point 🤨 |
| @@ -1197,13 +1239,11 @@ public SubclassNativeFriendlyRandom() | |||
| _nextNumber = 100; | |||
| } | |||
|
|
|||
| [JsiiMethod(name: "hello", returnsJson: "{\"type\":{\"primitive\":\"string\"}}", isOverride: true)] | |||
There was a problem hiding this comment.
I am amazed that this was how these tests were written... Did we really expect users to annotate all their methods like this??
There was a problem hiding this comment.
🤷🏻♂️ but to be honest figuring out how to get annotations from interfaces was... a challenge. But I do agree. In terms of ergonomics it was... terrible.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
RomainMuller
removed
the
pr/do-not-merge
There was a problem hiding this comment.
Are we testing the case where the callback gets passed an object in JSII-land and calls it?
I suggest you implement this set of tests as well, should be quick: https://github.com/aws/jsii/blob/master/packages/jsii-python-runtime/tests/test_compliance.py#L979
| @@ -509,10 +510,8 @@ class InterfaceWithProperties : DeputyBase, IInterfaceWithProperties | |||
| { | |||
| string _x; | |||
|
|
|||
| [JsiiProperty(name: "readOnlyString", typeJson: "{\"primitive\":\"string\"}", isOverride: true)] | |||
There was a problem hiding this comment.
Ah yeah. How did we miss this in the first place?
|
|
||
| var converter = ServiceContainer.ServiceProvider.GetRequiredService<IJsiiToFrameworkConverter>(); | ||
|
|
||
| var rehydratedArgs = Enumerable.Range(0, request.Arguments.Length) |
There was a problem hiding this comment.
All this work should be in its own function, no? I have a hard time grokking it and I trust you that it's correct, but I'd like to see it factored out into more readable code units.
Seems like the same hydration/dehydration should happen in multiple places, so why isn't the code shared?
| return value; | ||
| }).ToArray(); | ||
|
|
||
| var invokeParameters = Enumerable.Range(0, parameters.Length) |
| /// <typeparam name="T">The attribute type that is desired</typeparam> | ||
| /// <returns>The attribute if one was found, or null</returns> | ||
| [return: MaybeNull] | ||
| internal static T GetAttribute<T>(this PropertyInfo property, bool inherit = true) where T : Attribute |
There was a problem hiding this comment.
These 2 methods look way too similar to me. Can this not be factored out with some generics?
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! |
Corrects an issue with handling callbacks in various situations:
some of the optional parameters not specified, an argument count
mismatch error would occur. This is because dotnet reflection requires
all arguments to be provided (with
nullif needed).a parameter count mismatch error would occur. This is because the
variadic parameter is an array of it's value type, and has to be
passed this way out to the reflection call.
causing type information to be lost and incorrect callback argument
serialization could happen in the Kernel.
the overrides would not be correctly identified, as the Attributes on
interface members are not visible on the implementors (whereas they
are visible when they were on an extended class member!).
in certain edge cases. The complete type information is available, so
switched to using the full conversion gear form the jsii runtime to
achieve correct results.
All of those issues are fairly inter-linked and it was difficult to fix
them individually & issue a test that would not break due to another of
the issues... Hence the fixes are grouped in a single commit.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.