Optimize System.String overloads

[jonpryor]

trafficstars

generator emits System.String overloads for Java members which accept or return java.lang.ICharSequence. For example, consider TextView.setText(), which is combined with getText() into a TextFormatted property:

namespace Android.Widget {
	public partial class TextView {
		public unsafe Java.Lang.ICharSequence? TextFormatted {
			get => …,
			set {
				const string __id = "setText.(Ljava/lang/CharSequence;)V";
				IntPtr native_value = CharSequence.ToLocalJniHandle (value);
				try {
					JniArgumentValue* __args = stackalloc JniArgumentValue [1];
					__args [0] = new JniArgumentValue (native_value);
					_members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
				} finally {
					JNIEnv.DeleteLocalRef (native_value);
					global::System.GC.KeepAlive (value);
				}
			}
		}
	}
}

To simplify use, generator "overloads" this property to accept System.String:

namespace Android.Widget {
	public partial class TextView {
		public string? Text {
			get =>  TextFormatted == null ? null : TextFormatted.ToString (),
			set {
				var jls = value == null ? null : new global::Java.Lang.String (value);
				TextFormatted = jls;
				if (jls != null) jls.Dispose ();
			}
		}
	}
}

This works, but has some implicit overheads: creating Java.Lang.String instances requires registering them with the JniRuntime.JniValueManager, which is "silly" when we will immediately Dispose() of the value. This can show up when profiling apps which call TextView.set_Text() frequently.

We could optimize these "overloads" by skipping Java.Lang.String instances, and instead using JniObjectReference and JniEnvironment.Strings.NewString(string?).

Unfortunately, just because we can doesn't mean that it will be straightforward. There are two "reasonable" ways to implement this:

Copy most of the code from e.g. set_TextFormatted() into set_Text():

namespace Android.Widget {
	public partial class TextView {
		public unsafe string? Text {
			set {
				const string __id = "setText.(Ljava/lang/CharSequence;)V";
				JniObjectReference native_value = JniEnvironment.Strings.NewString (value);
				try {
					JniArgumentValue* __args = stackalloc JniArgumentValue [1];
					__args [0] = new JniArgumentValue (native_value);
					_members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
				} finally {
					JniObjectReference.Dispose (ref native_value);
				}
			}
		}
	}
}

Alternatively, we can look into Issue #795, and move the _members.InstanceMethods.InvokeNonvirtualVoidMethod() invocation into an "ABI method." Instead of fully adopting #795, we could put the "ABI method" into the same scope, and use that from both TextFormatted and Text:

namespace Android.Widget {
	public partial class TextView {
		public ICharSequence? TextFormatted {
			set {
				IntPtr native_value = CharSequence.ToLocalJniHandle (value);
				try {
					abi_setText (this, native_value);
				} finally {
					JNIEnv.DeleteLocalRef (native_value);
					global::System.GC.KeepAlive (value);
				}
			}
		}

		public string? Text {
			set {
				JniObjectReference native_value = JniEnvironment.Strings.NewString (value);
				try {
					abi_setText (this, native_value);
				} finally {
					JniObjectReference.Dispose (ref native_value);
				}
			}
		}

		private static void abi_setText (IJavaPeerable self, JniObjectReference text)
		{
			const string __id = "setText.(Ljava/lang/CharSequence;)V";
			try {
				JniArgumentValue* __args = stackalloc JniArgumentValue [1];
				__args [0] = new JniArgumentValue (text);
				_members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
			} finally {
				global::System.GC.KeepAlive (self);
			}
		}
	}
}

This has the benefit of reducing code duplication.

[jonpryor]

See also: https://github.com/xamarin/java.interop/pull/1101#issuecomment-1518136752

The optimization described in this issue is only valid if the *Formatted property is not virtual. This is the case for some properties, such as TextView.TextFormatted, but not for all properties. TextClock.Format12HourFormatted is an example of a *Formatted property which is virtual, and for which this optimization could result in a semantic break.

The semantic break scenario is:

class MyTextClock : Android.Widget.TextClock {
    public override Java.Lang.ICharSequence? Format12HourFormatted {
        get => …;
        set => …;
    }
}

// …
TextClock myTextClock = new MyTextClock ();
myTextClock.Format12Hour = "whatever";

The current expectation is that when myTextClock.Format12Hour is set, myTextClock.Format12HourFormatted will be set, and as that property is overridden, the expectation is that the MyTextClock.Format12HourFormatted property is set, not the base class' TextClock.setFormat12Hour() method.

The approaches proposed in Issue #994 break that semantic. That's Not Good™.

The only time the proposals in #994 are valid is when the "underlying" *Formatted property is not a virtual or override property.

Fortunately that restriction is (partially) the case for TextView.setText(CharSequence), but not for TextView.getText(), but we bind TextView.TextFormatted as a non-virtual property (because setText() isn't virtual), so in the context of TextView.TextFormatted, this is "fine".

It's not fine for every possible scenario.

---

For reference, the Android.Widget.TextClock binding is:

namespace Android.Widget {

	public partial class TextClock : Android.Widget.TextView {
		public virtual unsafe Java.Lang.ICharSequence? Format12HourFormatted {
			// Metadata.xml XPath method reference: path="/api/package[@name='android.widget']/class[@name='TextClock']/method[@name='getFormat12Hour' and count(parameter)=0]"
			[Register ("getFormat12Hour", "()Ljava/lang/CharSequence;", "GetGetFormat12HourHandler")]
			get {
				const string __id = "getFormat12Hour.()Ljava/lang/CharSequence;";
				try {
					var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, null);
					return global::Java.Lang.Object.GetObject<Java.Lang.ICharSequence> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
				} finally {
				}
			}
			// Metadata.xml XPath method reference: path="/api/package[@name='android.widget']/class[@name='TextClock']/method[@name='setFormat12Hour' and count(parameter)=1 and parameter[1][@type='java.lang.CharSequence']]"
			[Register ("setFormat12Hour", "(Ljava/lang/CharSequence;)V", "GetSetFormat12Hour_Ljava_lang_CharSequence_Handler")]
			set {
				const string __id = "setFormat12Hour.(Ljava/lang/CharSequence;)V";
				IntPtr native_value = CharSequence.ToLocalJniHandle (value);
				try {
					JniArgumentValue* __args = stackalloc JniArgumentValue [1];
					__args [0] = new JniArgumentValue (native_value);
					_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
				} finally {
					JNIEnv.DeleteLocalRef (native_value);
					global::System.GC.KeepAlive (value);
				}
			}
		}

		public string? Format12Hour {
			get { return Format12HourFormatted == null ? null : Format12HourFormatted.ToString (); }
			set {
				var jls = value == null ? null : new global::Java.Lang.String (value);
				Format12HourFormatted = jls;
				if (jls != null) jls.Dispose ();
			}
		}
	}
}

[jonpryor]

Repro of the concern: formatted-semantics.zip

The repro has three parts:

1. A Java class which will generate a C# TextFormatted property, backed by a field value:

   public class ContainsTextFormattedProperty
       CharSequence text;
       public CharSeqeuence getText() {return text;}
       public void setText(CharSequence value) {text = value;}
   
       public static String getText(ContainsTextFormattedProperty self) {return self.getText().toString();}
   }
   

   The static getText(ContainsTextFormattedProperty) method allows us to do a Java-side method invocation.

2. A C# subclass:

   class MyType : ContainsTextFormattedProperty {
       string? text;
   
       public override Java.Lang.ICharSequence? TextFormatted {
           get => new Java.Lang.String ("From C#! " + text);
           set => text = value?.ToString();
       }
   }
   

3. The setup:

   var c = new MyType {
       Text = "Default Value",
   };
   

Thus, the "punch":

var s = MyType.GetText(c);
Console.WriteLine ($"           c.Text={c.Text}");
Console.WriteLine ($"MyType.GetText(c)={s}");

What should happen? What should happen is:

1. c.Text prints the same value as MyType.GetText(c)
2. The value is From C#: Default Value

We expect (1) because c.get_Text() calls the overriding c.get_TextFormatted(), and MyType.GetText() calls ContainsTextFormattedProperty.getText(), which virtually calls getText(), which is overridden by c.get_TextFormatted().

Now imagine what happens with the original PR #1101:

1. c.Text = "Default Value" would not virtually invoke the c.TextFormatted setter; it would instead non-virtually invoke ContainsTextFormattedProperty.setText().
2. The c.Text property getter would not virtually invoke the c.TextFormatted getter; it would instead non-virtually invoke ContainsTextFormattedProperty.getText(), which would be Default Value.
3. MyType.GetText() would cause Java to virtually invoke getText(), which is overridden to return From C#! Default Value
4. c.Text and MyType.GetText(c) would thus return different values.

This behavior makes no sense, and would confuse everyone. It's not only a semantic break; it's bad semantics all around!

---

That said, there is an issue with the *Formatted infrastructure, which didn't occur to me until creating this sample. What happens if MyText doesn't store strings?

class MyType : ContainsTextFormattedProperty {
    Java.Lang.ICharSequence? text;

    public override Java.Lang.ICharSequence? TextFormatted {
        get => new Java.Lang.String ("From C#! " + text?.ToString ());
        set => text = value;
    }
}

The answer is that c.Text returns From C#! . The substring Default Value is not present! The reason for this is that the Text property setter disposes of the instance!

	public partial class ContainsTextFormattedProperty : global::Java.Lang.Object {
		public string? Text {
			get { return TextFormatted == null ? null : TextFormatted.ToString (); }
			set {
				var jls = value == null ? null : new global::Java.Lang.String (value);
				TextFormatted = jls;
				if (jls != null) jls.Dispose ();
			}
		}
	}

Thus after MyType.set_TextFormatted() is called, MyType.text is referencing a disposed String instance, and <disposed-value>.ToString() returns the empty string instead of throwing an exception (?!).

This is decidedly a leaky abstraction. Kinda surprising nobody has tripped up against this.

[jonpryor]

For a brief moment, I thought that this entire question might be "moot" in Java.Base (#858), because I thought that C# 8+ -- which allows static members on interfaces -- would thus also allow implicit conversion operators.

It does not. This is not valid:

partial interface ICharSequence {
    public static implicit operator ICharSequence? (string? value) =>
        value == null ? null : new Java.Lang.String (value);
    // CS0567, CS0552
}

…which makes me sad. :-(