-
Notifications
You must be signed in to change notification settings - Fork 528
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
[Xamarin..Android.Tools.Aidl] Handle out and inout Parameters in CodeGenerator properly #5726
Conversation
@mfranke-moba: how "interested" are you in a "larger project"? :-D If you're interested, The question is whether or now you are interested in writing such a unit test framework. We'll get around to it…some day… But in the meantime, |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I already noticed there are almost no unit tests for |
|
||
string GetWriteOutStatements (TypeName type, string parcel, string arg) | ||
{ | ||
if (type.ArrayDimension > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ditto here: is int[][]
possible?
A quick gander at the AIDL documentation suggests it isn't possible. However, it does suggest that List
:
must be one of the supported data types in this list or one of the other AIDL-generated interfaces or parcelables you've declared
which implies that a List<List<List<String>>>
may be viable…and I have no idea what we'd do with that. (Wouldn't impact type.ArrayDimension
, though!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I understand the AIDL dokumentation is that multidimensional arrays like int[][]
are not supported. At the moment the CSharpCodeGenerator
or maybe the AidlParser
just ignores higher dimensions and int[][] is treated like int[]. However throwing some NotSupportedException might not be wrong.
For List
I think - but not 100% sure - you can put any supported object in. And one object can be a List which can contain other Lists and so on. I don't know why (maybe a bug) but the Android Studio does not support parametrization like List<List<List<String>>>>
, just List<String>
. So you can use List
and put Lists in but List<List>>
does not work!?
The CSharpCodeGenerator might not cover all cases for Lists correct and should definetely be unit tested also in this perspect.
Context: #5726 We have someone willing to contribute fixes for `Xamarin.Android.Tools.Aidl.dll` (yay!). However, we no longer have any expertise with AIDL available on the team, and there are no unit tests for this area. This makes us very reluctant to make changes which could potentially break the currently working aspects of AIDL. Add a new `tests/Xamarin.Android.Tools.Aidl-Tests` unit test project and seed it with a few examples from our repo and a few large examples from other OSS repos. Note: this does *not* mean that the behavior tested by these tests is necessarily *correct*. It simply reflects what the code does *today*, in order that we can see any changes made by future PR's. ~~ Note ~~ The format for test data files is delimited by 4 pound signs as: <input> #### <output> Test results can be generated by: - Placing the input in a test file - Enabling the `GENERATE_OUTPUT` define at the top of `AidlCompilerTestBase.cs` - Running the test - The test file in `bin/TestData` will now contain the test output which can be copied back to the original test data file.
This is to get unit tests for Xamarin.Android.Tools.Aidl.dll.
TODO: no verification/validation has been done to check that these changes are *correct*. These are the changes necessary in order for the unit tests to *pass* given the changes in 4ec8c09.
With the merging of PR #5756, I've merged main into the mfranke-moba/out_parameter_handling branch and updated the expected test output from 81d37a1 so that the unit tests pass. I have not in any way validated the the "changes required for unit tests to pass" are in any way correct… |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
string GetWriteOutStatements (TypeName type, string parcel, string arg) | ||
{ | ||
if (type.ArrayDimension > 0) { | ||
return "if (" + arg + " == null) { " + parcel + ".WriteInt(-1); } else { " + parcel + ".WriteInt(" + arg + ".Length); }"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that .WriteInt(-1)
doesn't show up anywhere in our current expected test output, we're going to need a new unit test which ensures this code path is hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfranke-moba : please add a unit test which causes this line to be triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already covered by Test11
. The WriteInt(-1)
will be written for out array parameters like out byte[]
. In this case no byte array is written with WriteXyzArray
like for in byte[]
or inout byte[]
as no data has to be transmitted, just the size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I had missed that. :-(
@@ -289,8 +303,8 @@ protected override bool OnTransact (int code, global::Android.OS.Parcel data, gl | |||
w.WriteLine ("\t\t\t\t{0}", GetWriteStatements (method.ReturnType, "reply", "result", "global::Android.OS.ParcelableWriteFlags.ReturnValue")); | |||
for (int i = 0; method.Arguments != null && i < method.Arguments.Length; i++) { | |||
var a = method.Arguments [i]; | |||
if (a.Modifier == null || a.Modifier.Contains ("out")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this change/removal is why data.WriteLong (arg0);
is removed in the unit test diff.
I think this is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was necessary because before for primitive input parameters like in void Authenticate (long operationId)
the input parameter after function execution was written back to the reply
Parcel which is only reqired for out and inout parameters.
Fixes #5079 and #4717
The CodeGenerator responible for producing C# Code for AIDL files so far had the problem that function parameters of some array type (like int[]) marked with out or inout were not handled properly.
For such parameters the legth of the array has to be written to the data parcelable in the Proxy function. See https://cs.android.com/android/platform/superproject/+/master:system/tools/aidl/generate_java_binder.cpp (lines 646 - 656).
Now just like in the linked sources something like:
will be produced.
The counterpart which is the corresponding Stub function will contain:
See https://cs.android.com/android/platform/superproject/+/master:system/tools/aidl/generate_java_binder.cpp (line 486 and lines 358 - 372)
This is comparable to the java code the Android Studio produces.
Now also return values will be written to _result instead of _data which was wrong before.
The improvements of this commit show for interprocess communication between different Apps/Services. Now also Communication betwen Xamarin and Java Apps/Services works for array parameters marked with out/inout.
I really hope this bugfix will find its way into the nex release of Visual Studio.