[generator] Make DIM invoking plumbing private (#581)

jpobst · web-flow · commit 4f47ec82c14a · 2020-02-21T13:09:46.000-05:00
Context: #509 We have never used explicit `private` modifiers on class members due to the [Mono Coding Guidelines][0], as `private` is the default. However, when working with Default Interface Methods on interfaces members, the default is `public`. This results in our plumbing members like `_members` and `static Delegate cb_DoSomething;` being exposed as part of the interface's public API. This commit makes private interface members explicitly `private`. [0]: https://www.mono-project.com/community/contributing/coding-guidelines/
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultMethod.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultMethod.txt
@@ -1,18 +1,18 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_DoSomething;
+	private static Delegate cb_DoSomething;
 #pragma warning disable 0169
-	static Delegate GetDoSomethingHandler ()
+	private static Delegate GetDoSomethingHandler ()
 	{
 		if (cb_DoSomething == null)
 			cb_DoSomething = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr>) n_DoSomething);
 		return cb_DoSomething;
 	}
 
-	static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
+	private static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		__this.DoSomething ();
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultProperty.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultProperty.txt
@@ -1,34 +1,34 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_get_Value;
+	private static Delegate cb_get_Value;
 #pragma warning disable 0169
-	static Delegate Getget_ValueHandler ()
+	private static Delegate Getget_ValueHandler ()
 	{
 		if (cb_get_Value == null)
 			cb_get_Value = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, int>) n_get_Value);
 		return cb_get_Value;
 	}
 
-	static int n_get_Value (IntPtr jnienv, IntPtr native__this)
+	private static int n_get_Value (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		return __this.Value;
 	}
 #pragma warning restore 0169
 
-	static Delegate cb_set_Value_I;
+	private static Delegate cb_set_Value_I;
 #pragma warning disable 0169
-	static Delegate Getset_Value_IHandler ()
+	private static Delegate Getset_Value_IHandler ()
 	{
 		if (cb_set_Value_I == null)
 			cb_set_Value_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, int>) n_set_Value_I);
 		return cb_set_Value_I;
 	}
 
-	static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)
+	private static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		__this.Value = value;
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultPropertyGetterOnly.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteInterfaceDefaultPropertyGetterOnly.txt
@@ -1,18 +1,18 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_get_Value;
+	private static Delegate cb_get_Value;
 #pragma warning disable 0169
-	static Delegate Getget_ValueHandler ()
+	private static Delegate Getget_ValueHandler ()
 	{
 		if (cb_get_Value == null)
 			cb_get_Value = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, int>) n_get_Value);
 		return cb_get_Value;
 	}
 
-	static int n_get_Value (IntPtr jnienv, IntPtr native__this)
+	private static int n_get_Value (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		return __this.Value;
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteStaticInterfaceMethod.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteStaticInterfaceMethod.txt
@@ -31,7 +31,7 @@ public abstract class MyInterfaceConsts : MyInterface {
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
 	// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='DoSomething' and count(parameter)=0]"
 	[Register ("DoSomething", "()V", "")]
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteStaticInterfaceProperty.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/JavaInterop1/WriteStaticInterfaceProperty.txt
@@ -1,7 +1,7 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
 	 static unsafe int Value {
 		// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='get_Value' and count(parameter)=0]"
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultMethod.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultMethod.txt
@@ -1,18 +1,18 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_DoSomething;
+	private static Delegate cb_DoSomething;
 #pragma warning disable 0169
-	static Delegate GetDoSomethingHandler ()
+	private static Delegate GetDoSomethingHandler ()
 	{
 		if (cb_DoSomething == null)
 			cb_DoSomething = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr>) n_DoSomething);
 		return cb_DoSomething;
 	}
 
-	static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
+	private static void n_DoSomething (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		__this.DoSomething ();
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultProperty.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultProperty.txt
@@ -1,34 +1,34 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_get_Value;
+	private static Delegate cb_get_Value;
 #pragma warning disable 0169
-	static Delegate Getget_ValueHandler ()
+	private static Delegate Getget_ValueHandler ()
 	{
 		if (cb_get_Value == null)
 			cb_get_Value = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, int>) n_get_Value);
 		return cb_get_Value;
 	}
 
-	static int n_get_Value (IntPtr jnienv, IntPtr native__this)
+	private static int n_get_Value (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		return __this.Value;
 	}
 #pragma warning restore 0169
 
-	static Delegate cb_set_Value_I;
+	private static Delegate cb_set_Value_I;
 #pragma warning disable 0169
-	static Delegate Getset_Value_IHandler ()
+	private static Delegate Getset_Value_IHandler ()
 	{
 		if (cb_set_Value_I == null)
 			cb_set_Value_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, int>) n_set_Value_I);
 		return cb_set_Value_I;
 	}
 
-	static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)
+	private static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		__this.Value = value;
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultPropertyGetterOnly.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteInterfaceDefaultPropertyGetterOnly.txt
@@ -1,18 +1,18 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
-	static Delegate cb_get_Value;
+	private static Delegate cb_get_Value;
 #pragma warning disable 0169
-	static Delegate Getget_ValueHandler ()
+	private static Delegate Getget_ValueHandler ()
 	{
 		if (cb_get_Value == null)
 			cb_get_Value = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, int>) n_get_Value);
 		return cb_get_Value;
 	}
 
-	static int n_get_Value (IntPtr jnienv, IntPtr native__this)
+	private static int n_get_Value (IntPtr jnienv, IntPtr native__this)
 	{
 		java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
 		return __this.Value;
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteStaticInterfaceMethod.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteStaticInterfaceMethod.txt
@@ -31,7 +31,7 @@ public abstract class MyInterfaceConsts : MyInterface {
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
 	// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='DoSomething' and count(parameter)=0]"
 	[Register ("DoSomething", "()V", "")]
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteStaticInterfaceProperty.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/XAJavaInterop1/WriteStaticInterfaceProperty.txt
@@ -1,7 +1,7 @@
 // Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"
 [Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]
 public partial interface IMyInterface : IJavaObject, IJavaPeerable {
-	static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
+	private static readonly JniPeerMembers _members = new XAPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);
 
 	 static unsafe int Value {
 		// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']/method[@name='get_Value' and count(parameter)=0]"
diff --git a/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs b/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs
@@ -1028,12 +1028,14 @@ public void WriteInterfacePropertyInvokers (InterfaceGen @interface, IEnumerable
 		#region "if you're changing this part, also change method in https://github.com/xamarin/xamarin-android/blob/master/src/Mono.Android.Export/CallbackCode.cs"
 		public virtual void WriteMethodCallback (Method method, string indent, GenBase type, string property_name, bool as_formatted = false)
 		{
+			var is_private = method.IsInterfaceDefaultMethod ? "private " : string.Empty;
+
 			string delegate_type = method.GetDelegateType ();
-			writer.WriteLine ("{0}static Delegate {1};", indent, method.EscapedCallbackName);
+			writer.WriteLine ("{0}{2}static Delegate {1};", indent, method.EscapedCallbackName, is_private);
 			writer.WriteLine ("#pragma warning disable 0169");
 			if (method.Deprecated != null)
 				writer.WriteLine ($"{indent}[Obsolete]");
-			writer.WriteLine ("{0}static Delegate {1} ()", indent, method.ConnectorName);
+			writer.WriteLine ("{0}{2}static Delegate {1} ()", indent, method.ConnectorName, is_private);
 			writer.WriteLine ("{0}{{", indent);
 			writer.WriteLine ("{0}\tif ({1} == null)", indent, method.EscapedCallbackName);
 			writer.WriteLine ("{0}\t\t{1} = JNINativeWrapper.CreateDelegate (({2}) n_{3});", indent, method.EscapedCallbackName, delegate_type, method.Name + method.IDSignature);
@@ -1042,7 +1044,7 @@ public virtual void WriteMethodCallback (Method method, string indent, GenBase t
 			writer.WriteLine ();
 			if (method.Deprecated != null)
 				writer.WriteLine ($"{indent}[Obsolete]");
-			writer.WriteLine ("{0}static {1} n_{2} (IntPtr jnienv, IntPtr native__this{3})", indent, method.RetVal.NativeType, method.Name + method.IDSignature, method.Parameters.GetCallbackSignature (opt));
+			writer.WriteLine ("{0}{4}static {1} n_{2} (IntPtr jnienv, IntPtr native__this{3})", indent, method.RetVal.NativeType, method.Name + method.IDSignature, method.Parameters.GetCallbackSignature (opt), is_private);
 			writer.WriteLine ("{0}{{", indent);
 			writer.WriteLine ("{0}\t{1} __this = global::Java.Lang.Object.GetObject<{1}> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);", indent, opt.GetOutputName (type.FullName));
 			foreach (string s in method.Parameters.GetCallbackPrep (opt))
diff --git a/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/JavaInteropCodeGenerator.cs b/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/JavaInteropCodeGenerator.cs
@@ -270,7 +270,7 @@ internal override void WriteFieldSetBody (Field field, string indent, GenBase ty
 
 		void WritePeerMembers (string indent, string rawJniType, string declaringType, bool isInterface)
 		{
-			var signature = "static readonly JniPeerMembers _members = ";
+			var signature = $"{(isInterface ? "private " : "")}static readonly JniPeerMembers _members = ";
 			var type = $"new {GetPeerMembersType ()} (\"{rawJniType}\", typeof ({declaringType}){(isInterface ? ", isInterface: true" : string.Empty)});";
 
 			writer.WriteLine ($"{indent}{signature}{type}");

Original file line number	Diff line number	Diff line change
`@@ -1,34 +1,34 @@`
`1`	`1`	`// Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='IMyInterface']"`
`2`	`2`	`[Register ("java/code/IMyInterface", "", "java.code.IMyInterfaceInvoker")]`
`3`	`3`	`public partial interface IMyInterface : IJavaObject, IJavaPeerable {`
`4`		`- static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);`
	`4`	`+ private static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/IMyInterface", typeof (IMyInterface), isInterface: true);`
`5`	`5`
`6`		`- static Delegate cb_get_Value;`
	`6`	`+ private static Delegate cb_get_Value;`
`7`	`7`	`#pragma warning disable 0169`
`8`		`- static Delegate Getget_ValueHandler ()`
	`8`	`+ private static Delegate Getget_ValueHandler ()`
`9`	`9`	`{`
`10`	`10`	`if (cb_get_Value == null)`
`11`	`11`	`cb_get_Value = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, int>) n_get_Value);`
`12`	`12`	`return cb_get_Value;`
`13`	`13`	`}`
`14`	`14`
`15`		`- static int n_get_Value (IntPtr jnienv, IntPtr native__this)`
	`15`	`+ private static int n_get_Value (IntPtr jnienv, IntPtr native__this)`
`16`	`16`	`{`
`17`	`17`	`java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);`
`18`	`18`	`return __this.Value;`
`19`	`19`	`}`
`20`	`20`	`#pragma warning restore 0169`
`21`	`21`
`22`		`- static Delegate cb_set_Value_I;`
	`22`	`+ private static Delegate cb_set_Value_I;`
`23`	`23`	`#pragma warning disable 0169`
`24`		`- static Delegate Getset_Value_IHandler ()`
	`24`	`+ private static Delegate Getset_Value_IHandler ()`
`25`	`25`	`{`
`26`	`26`	`if (cb_set_Value_I == null)`
`27`	`27`	`cb_set_Value_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, int>) n_set_Value_I);`
`28`	`28`	`return cb_set_Value_I;`
`29`	`29`	`}`
`30`	`30`
`31`		`- static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)`
	`31`	`+ private static void n_set_Value_I (IntPtr jnienv, IntPtr native__this, int value)`
`32`	`32`	`{`
`33`	`33`	`java.code.IMyInterface __this = global::Java.Lang.Object.GetObject<java.code.IMyInterface> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);`
`34`	`34`	`__this.Value = value;`