Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Apr 19, 2022

[generator] Fix xamarin-android/src/Mono.Android build

Context: a65d6fb
Context: dotnet/android#6939

dotnet/android#6939 attempted to bump to 05eddd9, which
promptly broke the build of src/Mono.Android, e.g.
src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs:

/*       */ partial class GridLayout {
/*       */     partial class partial class LayoutParams {
/*       */         [Register ("rowSpec")]
/*       */         public Android.Widget.GridLayout.Spec? RowSpec {
/*       */             get {
/*       */                 const string __id = "rowSpec.Landroid/widget/GridLayout$Spec;";
/*       */                 var __v = _members.InstanceFields.GetObjectValue (__id, this);
/*       */                 return global::Java.Lang.Object.GetObject<Android.Widget.GridLayout.Spec> (__v.Handle, JniHandleOwnership.TransferLocalRef);
/*       */             }
/*       */             set {
/*       */                 const string __id = "rowSpec.Landroid/widget/GridLayout$Spec;";
/*       */                 IntPtr native_value = global::Android.Runtime.JNIEnv.ToLocalJniHandle (value);
/*       */                 try {
/* L 239 */                     _members.InstanceFields.SetValue (__id, this, new JniObjectReference (value));
/*       */                 } finally {
/* L 242 */                     global::Android.Runtime.JNIEnv.DeleteLocalRef (value);
/*       */                 }
/*       */             }
/*       */         }
/*       */     }
/*       */ }

due to compilation errors:

src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs(239,77):
  error CS1503: Argument 1: cannot convert from 'Android.Widget.GridLayout.Spec' to 'System.IntPtr'
src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs(242,54):
  error CS1503: Argument 1: cannot convert from 'Android.Widget.GridLayout.Spec' to 'System.IntPtr'

This was caused by BoundFieldAsProperty.cs not appropriately
setting arg to native_arg, so that the correct variable would be
cleaned up in the finally block.

There was another set of errors:

src/Mono.Android/obj/Release/net6.0/android-32/mcw/Android.Widget.ArrayAdapter.cs(525,53):
  error CS1503: Argument 1: cannot convert from 'Java.Lang.ICharSequence[]' to 'string[]?'

which was also caused by a65d6fb mis-refactoring
SourceWriterExtensions.cs, and overlooking the entire existence of
the CharSequence.ArrayToStringArray() method (oops).

Update generator so that xamarin-android once again builds, and add
unit tests to hit these particular code paths.

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Apr 19, 2022
} else {
writer.WriteLine ($"{(invokeType != "Object" ? arg : "new JniObjectReference (" + arg + ")")});");
}
writer.WriteLine ();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I added this in the first place, and it's presence causes extra whitespace changes when comparing src/Mono.Android build output from pre- a65d6fb vs. post- a65d6fb

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

With a forthcoming unit test and a successful test merge to XA (dotnet/android#6939), this should be fine.

@jonpryor jonpryor force-pushed the jonp-fix-xa-codegen branch from d9a087b to 5cf0473 Compare April 20, 2022 17:59
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Apr 20, 2022
public string GetCharSequenceArrayToStringArrayMethodName ()
{
return CodeGenerationTarget == CodeGenerationTarget.JavaInterop1
? "ICharSequenceExtensions.ToStringArray"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ICharSequenceExtensions.ToStringArray used? It doesn't show up in the unit test output, whereas CharSequence.ArrayToStringArray does show up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpobst asked:

Is ICharSequenceExtensions.ToStringArray used?

No. Not only is it not used, the entire "context" is "wrong"; compare the XAJavaInterop1-NRT binding of CharSequence[] Echo(CharSequence[]):

[Register ("Echo", "([Ljava/lang/CharSequence;)[Ljava/lang/CharSequence;", "")]
public static unsafe Java.Lang.ICharSequence[]? EchoFormatted (Java.Lang.ICharSequence[]? messages) => …

public static string[]? Echo (string[]? messages)
{
	var jlca_messages = CharSequence.ArrayFromStringArray (messages);
	Java.Lang.ICharSequence[]? __result = EchoFormatted (jlca_messages);
	var __rsval = CharSequence.ArrayToStringArray (__result);
	if (jlca_messages != null) foreach (var s in jlca_messages) s?.Dispose ();
	return __rsval;
}

(Aside: notice that XAJavaInterop1 uses both CharSequence.ArrayFromStringArray() and CharSequence.ArrayToStringArray().)

Compare to the JavaInterop1 binding of CharSequence[] Echo(CharSequence[]):

public static unsafe Java.Interop.JavaObjectArray<Java.Lang.ICharSequence>? Echo (Java.Lang.ICharSequence[]? messages) => …

public static Java.Interop.JavaObjectArray<string>? Echo (string[]? messages)
{
	var jlca_messages = ICharSequenceExtensions.ToCharSequenceArray (messages);
	Java.Interop.JavaObjectArray<Java.Lang.ICharSequence>? __result = Echo (jlca_messages);
	var __rsval = __result;
	if (jlca_messages != null) foreach (var s in jlca_messages) s?.Dispose ();
	return __rsval;
}

It's all manner of "wrong":

  • XAJavaInterop1 has EchoFormatted() and Echo(); JavaInterop1 has Echo() overloads. Presumably the first method should be EchoFormatted()?

  • As per a65d6fb, the messages argument should be a JavaObjectArray<ICharSequence>, not ICharSequence[]! (See also src/Java.Base-ref.cs from 968e0f5, for which the only occurrence of [] is as part of a params array.)

    Which implies that I'm doing something "wrong" with my Parameter creation?

  • Shouldn't the return type of Echo() be string[]?

I think the fundamental problem here is that my attempted unit test "setup" is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"For comparison", if I update tests/generator-Tests/expected/StaticMethods/StaticMethod.xml:

diff --git a/tests/generator-Tests/expected/StaticMethods/StaticMethod.xml b/tests/generator-Tests/expected/StaticMethods/StaticMethod.xml
index 913f5e69..294ba933 100644
--- a/tests/generator-Tests/expected/StaticMethods/StaticMethod.xml
+++ b/tests/generator-Tests/expected/StaticMethods/StaticMethod.xml
@@ -13,6 +13,9 @@
       		</method>
       		<method abstract="false" deprecated="Deprecated please use methodAsString" final="false" name="Obsoletemethod" native="false" return="java.lang.String" static="true" synchronized="false" visibility="public">
       		</method>
+      		<method abstract="false" deprecated="not deprecated" final="false" name="echo" native="false" return="java.lang.CharSequence[]" static="true" synchronized="false" visibility="public">
+				<parameter name="m" type="java.lang.CharSequence[]" />
+      		</method>
 	    </class>
 	</package>
 </api>

and then run:

mkdir _x
dotnet bin/Debug-net6.0/generator.dll --codegen-target=JavaInterop1 -o _x tests/generator-Tests/expected/StaticMethods/StaticMethod.xml

then _x/Xamarin.Test.SomeObject.cs contains:

// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/class[@name='SomeObject']/method[@name='echo' and count(parameter)=1 and parameter[1][@type='java.lang.CharSequence[]']]"
public static unsafe Java.Interop.JavaObjectArray<Java.Lang.ICharSequence> Echo (Java.Interop.JavaObjectArray<Java.Lang.ICharSequence> m)
{
	const string __id = "echo.([Ljava/lang/CharSequence;)[Ljava/lang/CharSequence;";
	var native_m = global::Java.Interop.JniEnvironment.Arrays.CreateMarshalObjectArray<global::Java.Lang.ICharSequence> (m);
	try {
		JniArgumentValue* __args = stackalloc JniArgumentValue [1];
		__args [0] = new JniArgumentValue (native_m);
		var __rm = _members.StaticMethods.InvokeObjectMethod (__id, __args);
		return global::Java.Interop.JniEnvironment.Runtime.ValueManager.GetValue<global::Java.Interop.JavaObjectArray<Java.Lang.ICharSequence>>(ref __rm, JniObjectReferenceOptions.CopyAndDispose);
	} finally {
		if (native_m != null) {
			native_m.DisposeUnlessReferenced ();
		}
		global::System.GC.KeepAlive (m);
	}
}
public static Java.Interop.JavaObjectArray<string> Echo (Java.Interop.JavaObjectArray<string> m)
{
	Java.Interop.JavaObjectArray<Java.Lang.ICharSequence> __result = Echo (m);
	var __rsval = __result;
	return __rsval;
}

which is closer to what I expected, in that the java.lang.CharSequence[] parameter was bound as JavaObjectArray<ICharSequence> (and then the method body fumbles things by using CreateMarshalObjectArray(), for no reason?!).

However, it's still not named EchoFormatted().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summarizing my previous two comments:

I think that the WriteMethodWithCharSequenceArrays() unit test is "wrong". It's not "close enough" to what the "full" XML pipeline does, and thus the output is kinda non-sensical, particularly with JavaInterop1 output.

I don't know how to fix it, offhand.

I should probably update StaticMethod.xml for this use case, if only to have a "full integration" pass. (Which still won't use ICharSequenceExtensions.ToStringArray(), but at least the output will be saner. Not sane yet, but saner.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed GetCharSequenceArrayToStringArrayMethodName() from the most recent update, and update WriteMethodWithCharSequenceArrays() to use ParseApiDefinition().

Context: a65d6fb
Context: dotnet/android#6939

dotnet/android#6939 attempted to bump to 05eddd9, which
promptly broke the build of src/Mono.Android, e.g.
`src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs`:

	/*       */ partial class GridLayout {
	/*       */     partial class partial class LayoutParams {
	/*       */         [Register ("rowSpec")]
	/*       */         public Android.Widget.GridLayout.Spec? RowSpec {
	/*       */             get {
	/*       */                 const string __id = "rowSpec.Landroid/widget/GridLayout$Spec;";
	/*       */                 var __v = _members.InstanceFields.GetObjectValue (__id, this);
	/*       */                 return global::Java.Lang.Object.GetObject<Android.Widget.GridLayout.Spec> (__v.Handle, JniHandleOwnership.TransferLocalRef);
	/*       */             }
	/*       */             set {
	/*       */                 const string __id = "rowSpec.Landroid/widget/GridLayout$Spec;";
	/*       */                 IntPtr native_value = global::Android.Runtime.JNIEnv.ToLocalJniHandle (value);
	/*       */                 try {
	/* L 239 */                     _members.InstanceFields.SetValue (__id, this, new JniObjectReference (value));
	/*       */                 } finally {
	/* L 242 */                     global::Android.Runtime.JNIEnv.DeleteLocalRef (value);
	/*       */                 }
	/*       */             }
	/*       */         }
	/*       */     }
	/*       */ }

due to compilation errors:

	src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs(239,77):
	  error CS1503: Argument 1: cannot convert from 'Android.Widget.GridLayout.Spec' to 'System.IntPtr'
	src/Mono.Android/obj/Debug/net6.0/android-32/mcw/Android.Widget.GridLayout.cs(242,54):
	  error CS1503: Argument 1: cannot convert from 'Android.Widget.GridLayout.Spec' to 'System.IntPtr'

This was caused by `BoundFieldAsProperty.cs` not appropriately
setting `arg` to `native_arg`, so that the correct variable would be
cleaned up in the `finally` block.

There was another set of errors:

	src/Mono.Android/obj/Release/net6.0/android-32/mcw/Android.Widget.ArrayAdapter.cs(525,53):
	  error CS1503: Argument 1: cannot convert from 'Java.Lang.ICharSequence[]' to 'string[]?'

which was also caused by a65d6fb mis-refactoring
`SourceWriterExtensions.cs`, and overlooking the entire existence of
the `CharSequence.ArrayToStringArray()` method (oops).

Update `generator` so that xamarin-android once again builds, and add
unit tests to hit these particular code paths.
@jonpryor jonpryor force-pushed the jonp-fix-xa-codegen branch from 5cf0473 to d7d317d Compare April 21, 2022 16:14
@jonpryor jonpryor merged commit 2a882d2 into dotnet:main Apr 21, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants