[generator] Prevent generating duplicate EventArgs classes (#726)

jpobst · web-flow · commit ee7afeedcf1e · 2020-09-24T20:26:38.000-04:00
Fixes: #461 Given this Java listener interface: // Java interface AnimatorListener { void onAnimationEnd (int param1); void onAnimationEnd (int param1, int param2); } Previously we would generate *two* `partial class AnimationEndEventArgs` declarations: // C# binding event args for java.code.AnimatorListener.onAnimationEnd public partial class AnimationEndEventArgs : global::System.EventArgs { public AnimationEndEventArgs (int param1) { this.param1 = param1; } int param1; public int Param1 { get { return param1; } } } // C# binding event args for java.code.AnimatorListener.onAnimationEnd public partial class AnimationEndEventArgs : global::System.EventArgs { public AnimationEndEventArgs (int param1, int param2) { this.param1 = param1; this.param2 = param2; } int param1; public int Param1 { get { return param1; } } int param2; public int Param2 { get { return param2; } } } This causes a compilation error: error CS0102: The type 'AnimationEndEventArgs' already contains a definition for 'param1' We need to detect these situations and instead combine multiple `EventArgs` classes into one, with a constructor for each method: // event args for java.code.AnimatorListener.OnAnimationEnd public partial class AnimationEndEventArgs : global::System.EventArgs { public AnimationEndEventArgs (int param1) { this.Param1 = param1; } public AnimationEndEventArgs (int param1, int param2) { this.Param1 = param1; this.Param2 = param2; } public int Param1 { get; private set; } public int Param2 { get; private set; } } Additionally, clean up the generated code a little bit by using auto properties instead of fields.
diff --git a/src/Xamarin.SourceWriter/Models/PropertyWriter.cs b/src/Xamarin.SourceWriter/Models/PropertyWriter.cs
@@ -35,6 +35,7 @@ public class PropertyWriter : ISourceWriter
 		public List<AttributeWriter> SetterAttributes { get; } = new List<AttributeWriter> ();
 		public int Priority { get; set; }
 		public string ExplicitInterfaceImplementation { get; set; }
+		public Visibility AutoSetterVisibility { get; set; }
 
 		public void SetVisibility (string visibility)
 		{
@@ -165,6 +166,8 @@ protected virtual void WriteAutomaticPropertyBody (CodeWriter writer)
 					writer.WriteLine ();
 					writer.Indent ();
 					need_unindent = true;
+				} else {
+					writer.Write (" ");
 				}
 
 				WriteGetterComments (writer);
@@ -187,6 +190,14 @@ protected virtual void WriteAutomaticPropertyBody (CodeWriter writer)
 
 				WriteSetterComments (writer);
 				WriteSetterAttributes (writer);
+
+				if (AutoSetterVisibility == Visibility.Private && !IsPrivate)
+					writer.Write ("private ");
+				else if (AutoSetterVisibility == Visibility.Protected && !IsProtected)
+					writer.Write ("protected ");
+				if (AutoSetterVisibility == Visibility.Internal && !IsInternal)
+					writer.Write ("internal ");
+
 				writer.Write ("set; ");
 
 				if (need_unindent) {
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/Common/WriteDuplicateInterfaceEventArgs.txt b/tests/generator-Tests/Unit-Tests/CodeGeneratorExpectedResults/Common/WriteDuplicateInterfaceEventArgs.txt
@@ -0,0 +1,191 @@
+// Metadata.xml XPath interface reference: path="/api/package[@name='java.code']/interface[@name='AnimatorListener']"
+[Register ("java/code/AnimatorListener", "", "java.code.AnimatorListenerInvoker")]
+public partial interface AnimatorListener : IJavaObject, IJavaPeerable {
+	// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='AnimatorListener']/method[@name='OnAnimationEnd' and count(parameter)=1 and parameter[1][@type='int']]"
+	[Register ("OnAnimationEnd", "(I)Z", "GetOnAnimationEnd_IHandler:java.code.AnimatorListenerInvoker, ")]
+	bool OnAnimationEnd (int param1);
+
+	// Metadata.xml XPath method reference: path="/api/package[@name='java.code']/interface[@name='AnimatorListener']/method[@name='OnAnimationEnd' and count(parameter)=2 and parameter[1][@type='int'] and parameter[2][@type='int']]"
+	[Register ("OnAnimationEnd", "(II)Z", "GetOnAnimationEnd_IIHandler:java.code.AnimatorListenerInvoker, ")]
+	bool OnAnimationEnd (int param1, int param2);
+
+}
+
+[global::Android.Runtime.Register ("java/code/AnimatorListener", DoNotGenerateAcw=true)]
+internal partial class AnimatorListenerInvoker : global::Java.Lang.Object, AnimatorListener {
+	static readonly JniPeerMembers _members = new JniPeerMembers ("java/code/AnimatorListener", typeof (AnimatorListenerInvoker));
+
+	static IntPtr java_class_ref {
+		get { return _members.JniPeerType.PeerReference.Handle; }
+	}
+
+	[global::System.Diagnostics.DebuggerBrowsable (global::System.Diagnostics.DebuggerBrowsableState.Never)]
+	[global::System.ComponentModel.EditorBrowsable (global::System.ComponentModel.EditorBrowsableState.Never)]
+	public override global::Java.Interop.JniPeerMembers JniPeerMembers {
+		get { return _members; }
+	}
+
+	[global::System.Diagnostics.DebuggerBrowsable (global::System.Diagnostics.DebuggerBrowsableState.Never)]
+	[global::System.ComponentModel.EditorBrowsable (global::System.ComponentModel.EditorBrowsableState.Never)]
+	protected override IntPtr ThresholdClass {
+		get { return class_ref; }
+	}
+
+	[global::System.Diagnostics.DebuggerBrowsable (global::System.Diagnostics.DebuggerBrowsableState.Never)]
+	[global::System.ComponentModel.EditorBrowsable (global::System.ComponentModel.EditorBrowsableState.Never)]
+	protected override global::System.Type ThresholdType {
+		get { return _members.ManagedPeerType; }
+	}
+
+	IntPtr class_ref;
+
+	public static AnimatorListener GetObject (IntPtr handle, JniHandleOwnership transfer)
+	{
+		return global::Java.Lang.Object.GetObject<AnimatorListener> (handle, transfer);
+	}
+
+	static IntPtr Validate (IntPtr handle)
+	{
+		if (!JNIEnv.IsInstanceOf (handle, java_class_ref))
+			throw new InvalidCastException (string.Format ("Unable to convert instance of type '{0}' to type '{1}'.", JNIEnv.GetClassNameFromInstance (handle), "java.code.AnimatorListener"));
+		return handle;
+	}
+
+	protected override void Dispose (bool disposing)
+	{
+		if (this.class_ref != IntPtr.Zero)
+			JNIEnv.DeleteGlobalRef (this.class_ref);
+		this.class_ref = IntPtr.Zero;
+		base.Dispose (disposing);
+	}
+
+	public AnimatorListenerInvoker (IntPtr handle, JniHandleOwnership transfer) : base (Validate (handle), transfer)
+	{
+		IntPtr local_ref = JNIEnv.GetObjectClass (((global::Java.Lang.Object) this).Handle);
+		this.class_ref = JNIEnv.NewGlobalRef (local_ref);
+		JNIEnv.DeleteLocalRef (local_ref);
+	}
+
+	static Delegate cb_OnAnimationEnd_I;
+#pragma warning disable 0169
+	static Delegate GetOnAnimationEnd_IHandler ()
+	{
+		if (cb_OnAnimationEnd_I == null)
+			cb_OnAnimationEnd_I = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPI_Z) n_OnAnimationEnd_I);
+		return cb_OnAnimationEnd_I;
+	}
+
+	static bool n_OnAnimationEnd_I (IntPtr jnienv, IntPtr native__this, int param1)
+	{
+		var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
+		return __this.OnAnimationEnd (param1);
+	}
+#pragma warning restore 0169
+
+	IntPtr id_OnAnimationEnd_I;
+	public unsafe bool OnAnimationEnd (int param1)
+	{
+		if (id_OnAnimationEnd_I == IntPtr.Zero)
+			id_OnAnimationEnd_I = JNIEnv.GetMethodID (class_ref, "OnAnimationEnd", "(I)Z");
+		JValue* __args = stackalloc JValue [1];
+		__args [0] = new JValue (param1);
+		return JNIEnv.CallBooleanMethod (((global::Java.Lang.Object) this).Handle, id_OnAnimationEnd_I, __args);
+	}
+
+	static Delegate cb_OnAnimationEnd_II;
+#pragma warning disable 0169
+	static Delegate GetOnAnimationEnd_IIHandler ()
+	{
+		if (cb_OnAnimationEnd_II == null)
+			cb_OnAnimationEnd_II = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPII_Z) n_OnAnimationEnd_II);
+		return cb_OnAnimationEnd_II;
+	}
+
+	static bool n_OnAnimationEnd_II (IntPtr jnienv, IntPtr native__this, int param1, int param2)
+	{
+		var __this = global::Java.Lang.Object.GetObject<java.code.AnimatorListener> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
+		return __this.OnAnimationEnd (param1, param2);
+	}
+#pragma warning restore 0169
+
+	IntPtr id_OnAnimationEnd_II;
+	public unsafe bool OnAnimationEnd (int param1, int param2)
+	{
+		if (id_OnAnimationEnd_II == IntPtr.Zero)
+			id_OnAnimationEnd_II = JNIEnv.GetMethodID (class_ref, "OnAnimationEnd", "(II)Z");
+		JValue* __args = stackalloc JValue [2];
+		__args [0] = new JValue (param1);
+		__args [1] = new JValue (param2);
+		return JNIEnv.CallBooleanMethod (((global::Java.Lang.Object) this).Handle, id_OnAnimationEnd_II, __args);
+	}
+
+}
+
+// event args for java.code.AnimatorListener.OnAnimationEnd
+public partial class AnimationEndEventArgs : global::System.EventArgs {
+	public AnimationEndEventArgs (bool handled, int param1)
+	{
+		this.Handled = handled;
+		this.Param1 = param1;
+	}
+
+	public AnimationEndEventArgs (bool handled, int param1, int param2)
+	{
+		this.Handled = handled;
+		this.Param1 = param1;
+		this.Param2 = param2;
+	}
+
+	public bool Handled { get; set; }
+
+	public int Param1 { get; private set; }
+
+	public int Param2 { get; private set; }
+
+}
+
+[global::Android.Runtime.Register ("mono/java/code/AnimatorListenerImplementor")]
+internal sealed partial class AnimatorListenerImplementor : global::Java.Lang.Object, AnimatorListener {
+
+	object sender;
+
+	public AnimatorListenerImplementor (object sender) : base (global::Android.Runtime.JNIEnv.StartCreateInstance ("mono/java/code/AnimatorListenerImplementor", "()V"), JniHandleOwnership.TransferLocalRef)
+	{
+		global::Android.Runtime.JNIEnv.FinishCreateInstance (((global::Java.Lang.Object) this).Handle, "()V");
+		this.sender = sender;
+	}
+
+	#pragma warning disable 0649
+	public EventHandler<AnimationEndEventArgs> OnAnimationEndHandler;
+	#pragma warning restore 0649
+
+	public bool OnAnimationEnd (int param1)
+	{
+		var __h = OnAnimationEndHandler;
+		if (__h == null)
+			return false;
+		var __e = new AnimationEndEventArgs (true, param1);
+		__h (sender, __e);
+		return __e.Handled;
+	}
+
+	#pragma warning disable 0649
+	public EventHandler<AnimationEndEventArgs> OnAnimationEndHandler;
+	#pragma warning restore 0649
+
+	public bool OnAnimationEnd (int param1, int param2)
+	{
+		var __h = OnAnimationEndHandler;
+		if (__h == null)
+			return false;
+		var __e = new AnimationEndEventArgs (true, param1, param2);
+		__h (sender, __e);
+		return __e.Handled;
+	}
+
+	internal static bool __IsEmpty (AnimatorListenerImplementor value)
+	{
+		return value.OnAnimationEndHandler == null && value.OnAnimationEndHandler == null;
+	}
+
+}
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs b/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
@@ -195,6 +195,26 @@ public void ManagedOverrideProperty_Override ()
 
 			Assert.True (writer.ToString ().Contains ("public override unsafe int Name {"));
 		}
+
+		[Test]
+		public void WriteDuplicateInterfaceEventArgs ()
+		{
+			// If we have 2 methods that would each create the same EventArgs class,
+			// make sure we combine them into 1 class with both members instead.
+			var iface = SupportTypeBuilder.CreateEmptyInterface ("java.code.AnimatorListener");
+
+			var method1 = SupportTypeBuilder.CreateMethod (iface, "OnAnimationEnd", options, "boolean", false, true, new Parameter ("param1", "int", "int", false));
+			var method2 = SupportTypeBuilder.CreateMethod (iface, "OnAnimationEnd", options, "boolean", false, true, new Parameter ("param1", "int", "int", false), new Parameter ("param2", "int", "int", false));
+
+			iface.Methods.Add (method1);
+			iface.Methods.Add (method2);
+
+			generator.Context.ContextTypes.Push (iface);
+			generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
+			generator.Context.ContextTypes.Pop ();
+
+			Assert.AreEqual (GetExpected (nameof (WriteDuplicateInterfaceEventArgs)), writer.ToString ().NormalizeLineEndings ());
+		}
 	}
 
 	[TestFixture]
diff --git a/tests/generator-Tests/expected.ji/GenericArguments/Com.Google.Android.Exoplayer.Drm.IExoMediaDrm.cs b/tests/generator-Tests/expected.ji/GenericArguments/Com.Google.Android.Exoplayer.Drm.IExoMediaDrm.cs
@@ -123,42 +123,22 @@ public unsafe void OnEvent (global::Com.Google.Android.Exoplayer.Drm.IExoMediaDr
 	public partial class ExoMediaDrmOnEventEventArgs : global::System.EventArgs {
 		public ExoMediaDrmOnEventEventArgs (global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrm p0, byte[] p1, int p2, int p3, byte[] p4)
 		{
-			this.p0 = p0;
-			this.p1 = p1;
-			this.p2 = p2;
-			this.p3 = p3;
-			this.p4 = p4;
+			this.P0 = p0;
+			this.P1 = p1;
+			this.P2 = p2;
+			this.P3 = p3;
+			this.P4 = p4;
 		}
 
-		global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrm p0;
+		public global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrm P0 { get; private set; }
 
-		public global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrm P0 {
-			get { return p0; }
-		}
-
-		byte[] p1;
-
-		public byte[] P1 {
-			get { return p1; }
-		}
-
-		int p2;
+		public byte[] P1 { get; private set; }
 
-		public int P2 {
-			get { return p2; }
-		}
-
-		int p3;
+		public int P2 { get; private set; }
 
-		public int P3 {
-			get { return p3; }
-		}
+		public int P3 { get; private set; }
 
-		byte[] p4;
-
-		public byte[] P4 {
-			get { return p4; }
-		}
+		public byte[] P4 { get; private set; }
 
 	}
 
diff --git a/tools/generator/SourceWriters/BoundInterface.cs b/tools/generator/SourceWriters/BoundInterface.cs
@@ -104,8 +104,17 @@ void AddInterfaceEventHandler (InterfaceGen iface, CodeGenerationOptions opt, Co
 
 			foreach (var method in iface.Methods.Where (m => m.EventName != string.Empty)) {
 				if (method.RetVal.IsVoid || method.IsEventHandlerWithHandledProperty) {
-					if (!method.IsSimpleEventHandler || method.IsEventHandlerWithHandledProperty)
-						post_sibling_types.Add (new InterfaceEventArgsClass (iface, method, opt, context));
+					if (!method.IsSimpleEventHandler || method.IsEventHandlerWithHandledProperty) {
+						var event_args_class = post_sibling_types.OfType<InterfaceEventArgsClass> ().SingleOrDefault (c => c.Name == iface.GetArgsName (method));
+
+						// Check if there's an existing EventArgs class to add to
+						if (event_args_class is null) {
+							event_args_class = new InterfaceEventArgsClass (iface, method);
+							post_sibling_types.Add (event_args_class);
+						}
+
+						event_args_class.AddMembersFromMethod (iface, method, opt);
+					}
 				} else {
 					var del = new DelegateWriter {
 						Name = iface.GetEventDelegateName (method),
diff --git a/tools/generator/SourceWriters/BoundMethod.cs b/tools/generator/SourceWriters/BoundMethod.cs
@@ -12,8 +12,12 @@ public class BoundMethod : MethodWriter
 	{
 		readonly MethodCallback callback;
 
+		public Method JavaMethod { get; }
+
 		public BoundMethod (GenBase type, Method method, CodeGenerationOptions opt, bool generateCallbacks)
 		{
+			JavaMethod = method;
+
 			if (generateCallbacks && method.IsVirtual)
 				callback = new MethodCallback (type, method, opt, null, method.IsReturnCharSequence);
 
diff --git a/tools/generator/SourceWriters/InterfaceEventArgsClass.cs b/tools/generator/SourceWriters/InterfaceEventArgsClass.cs

Original file line number	Diff line number	Diff line change
`@@ -12,8 +12,12 @@ public class BoundMethod : MethodWriter`
`12`	`12`	`{`
`13`	`13`	`readonly MethodCallback callback;`
`14`	`14`
	`15`	`+ public Method JavaMethod { get; }`
	`16`	`+`
`15`	`17`	`public BoundMethod (GenBase type, Method method, CodeGenerationOptions opt, bool generateCallbacks)`
`16`	`18`	`{`
	`19`	`+ JavaMethod = method;`
	`20`	`+`
`17`	`21`	`if (generateCallbacks && method.IsVirtual)`
`18`	`22`	`callback = new MethodCallback (type, method, opt, null, method.IsReturnCharSequence);`
`19`	`23`