[generator] Be smarter about when members with same names need "new" (#567)

Today when a generated member has the same name as a method on
`System.Object` we generate the method with a `new` in order to hide
the base member.  However, we only compare via the method name and
not the method signature, meaning we add more `new` than is necessary.

For example this creates a warning because `new` is not needed:

	// Org.Json.JSONArray.cs
	public new virtual unsafe string ToString (int indentSpaces) { … }

	warning CS0109: The member 'JSONArray.ToString(int)' does not hide an accessible member. The new keyword is not required.

These are the methods from `System.Object` that we currently check against:

  * `bool Equals (object)`
  * `static bool Equals (object, object)`
  * `static bool ReferenceEquals (object, object)`
  * `int GetHashCode ()`
  * `Type GetType ()`
  * `string ToString ()`

Additionally, all properties named `Handle` must get `new`.

With this change we now are more explicit about what methods we are
checking against so we can ensure that parameters and `static`-ness
match as well.

This eliminates 87 warnings from the API-29 `Mono.Android.dll` build.

Author: jpobst

tests/generator-Tests/Unit-Tests/GenBaseTests.cs

@@ -0,0 +1,130 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using MonoDroid.Generation;
+using NUnit.Framework;
+
+namespace generatortests
+{
+	[TestFixture]
+	public class GenBaseTests
+	{
+		CodeGenerationOptions options = new CodeGenerationOptions ();
+
+		[Test]
+		public void PropertyRequiresNew ()
+		{
+			var c = SupportTypeBuilder.CreateClassWithProperty ("MyClass", "java.myClass", "Handle", "int", options);
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "GetHashCode";
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "GetType";
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "ToString";
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "Equals";
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "ReferenceEquals";
+			Assert.True (c.RequiresNew (c.Properties.First ()));
+
+			c.Properties.First ().Name = "Handle2";
+			Assert.False (c.RequiresNew (c.Properties.First ()));
+		}
+
+		[Test]
+		public void ToStringRequiresNew () => TestParameterlessMethods ("ToString");
+
+		[Test]
+		public void GetTypeRequiresNew () => TestParameterlessMethods ("GetType");
+
+		[Test]
+		public void GetHashCodeRequiresNew () => TestParameterlessMethods ("GetHashCode");
+
+		[Test]
+		public void StaticEqualsCodeRequiresNew () => TestStaticMethodsWithTwoParameters ("Equals");
+
+		[Test]
+		public void ReferenceEqualsEqualsCodeRequiresNew () => TestStaticMethodsWithTwoParameters ("ReferenceEquals");
+
+		[Test]
+		public void HandleAlwaysRequiresNew ()
+		{
+			// The same name as a property always requires new, no matter the parameters
+			var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
+			var m = SupportTypeBuilder.CreateMethod (c, "Handle", options);
+
+			// Yes
+			Assert.True (c.RequiresNew (m.Name, m));
+
+			// Yes, even with parameters
+			m.Parameters.Add (new Parameter ("value", "int", "int", false));
+
+			Assert.True (c.RequiresNew (m.Name, m));
+		}
+
+		[Test]
+		public void TestEqualsMethodsWithOneParameter ()
+		{
+			var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
+			var m = SupportTypeBuilder.CreateMethod (c, "Equals", options, "void", false, false);
+
+			// No because 0 parameters
+			Assert.False (c.RequiresNew (m.Name, m));
+
+			var p0 = new Parameter ("p0", "object", "object", false);
+			m.Parameters.Add (p0);
+
+			// Yes
+			Assert.True (c.RequiresNew (m.Name, m));
+
+			// No because parameter is wrong type
+			var p1 = new Parameter ("p1", "string", "string", false);
+			m = SupportTypeBuilder.CreateMethod (c, "Equals", options, "void", true, false, p1);
+
+			Assert.False (c.RequiresNew (m.Name, m));
+		}
+
+		void TestParameterlessMethods (string name)
+		{
+			var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
+			var m = SupportTypeBuilder.CreateMethod (c, name, options);
+
+			// Yes
+			Assert.True (c.RequiresNew (m.Name, m));
+
+			// No because > 0 parameters
+			m.Parameters.Add (new Parameter ("value", "int", "int", false));
+
+			Assert.False (c.RequiresNew (m.Name, m));
+		}
+
+		void TestStaticMethodsWithTwoParameters (string name)
+		{
+			var c = SupportTypeBuilder.CreateClass ("java.myClass", options);
+
+			var p0 = new Parameter ("p0", "object", "object", false);
+			var p1 = new Parameter ("p1", "object", "object", false);
+			var m = SupportTypeBuilder.CreateMethod (c, name, options, "void", true, false, p0, p1);
+
+			// Yes
+			Assert.True (c.RequiresNew (m.Name, m));
+
+			// No because != 2 parameters
+			m.Parameters.Add (new Parameter ("value", "int", "int", false));
+			Assert.False (c.RequiresNew (m.Name, m));
+
+			// No because parameter is wrong type
+			var p2 = new Parameter ("p1", "string", "string", false);
+			m = SupportTypeBuilder.CreateMethod (c, name, options, "void", true, false, p0, p2);
+
+			Assert.False (c.RequiresNew (m.Name, m));
+		}
+	}
+}

tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs

@@ -1119,7 +1119,7 @@ public void WriteMethodAbstractDeclaration (Method method, string indent, Interf
 				WriteMethodCustomAttributes (method, indent);
 				writer.WriteLine ("{0}{1}{2} abstract {3} {4} ({5});",
 					indent,
-					impl.RequiresNew (method.Name) ? "new " : "",
+					impl.RequiresNew (method.Name, method) ? "new " : "",
 					method.Visibility,
 					opt.GetOutputName (method.RetVal.FullName),
 					name,
@@ -1367,7 +1367,7 @@ public void WriteMethod (Method method, string indent, GenBase type, bool genera
 				seal = string.Empty;
 			}
 
-			if ((string.IsNullOrEmpty (virt_ov) || virt_ov == " virtual") && type.RequiresNew (method.AdjustedName)) {
+			if ((string.IsNullOrEmpty (virt_ov) || virt_ov == " virtual") && type.RequiresNew (method.AdjustedName, method)) {
 				virt_ov = " new" + virt_ov;
 			}
 			string ret = opt.GetOutputName (method.RetVal.FullName);
@@ -1460,7 +1460,7 @@ public void WriteProperty (Property property, GenBase gen, string indent, bool w
 				force_override = true;
 
 			string decl_name = property.AdjustedName;
-			string needNew = gen.RequiresNew (decl_name) ? " new" : "";
+			string needNew = gen.RequiresNew (property) ? " new" : "";
 			string virtual_override = String.Empty;
 			bool is_virtual = property.Getter.IsVirtual && (property.Setter == null || property.Setter.IsVirtual);
 			if (with_callbacks && is_virtual) {
@@ -1537,7 +1537,7 @@ public void WritePropertyAbstractDeclaration (Property property, string indent,
 			string abstract_name = property.AdjustedName;
 			string visibility = property.Getter.RetVal.IsGeneric ? "protected" : property.Getter.Visibility;
 			if (!overrides) {
-				requiresNew = gen.RequiresNew (abstract_name);
+				requiresNew = gen.RequiresNew (property);
 				WritePropertyCallbacks (property, indent, gen, abstract_name);
 			}
 			writer.WriteLine ("{0}{1}{2} abstract{3} {4} {5} {{",

tools/generator/Java.Interop.Tools.Generator.ObjectModel/GenBase.cs

@@ -727,10 +727,44 @@ public string [] PreCallback (CodeGenerationOptions opt, string var_name, bool o
 
 		public string RawVisibility => support.Visibility;
 
-		public bool RequiresNew (string memberName)
+		public bool RequiresNew (Property property)
+		{
+			switch (property.AdjustedName.ToLowerInvariant ()) {
+				case "handle":
+				case "gethashcode":
+				case "gettype":
+				case "tostring":
+				case "equals":
+				case "referenceequals":
+					return true;
+			}
+
+			return IsThrowable () && ThrowableRequiresNew.Contains (property.AdjustedName);
+		}
+
+		public bool RequiresNew (string memberName, Method method)
 		{
-			if (ObjectRequiresNew.Contains (memberName))
-				return true;
+			switch (memberName.ToLowerInvariant ()) {
+				case "handle":
+					// The same name as a property always requires new, no matter the parameters
+					return true;
+				case "gethashcode":
+				case "gettype":
+				case "tostring":
+					return method.Parameters.Count == 0;
+				case "equals":
+					if (method.Parameters.Count == 1 && method.Parameters.All (p => p.Type == "object"))
+						return true;
+					if (method.Parameters.Count == 2 && method.Parameters.All (p => p.Type == "object"))
+						return true;
+
+					break;
+				case "referenceequals":
+					if (method.Parameters.Count == 2 && method.Parameters.All (p => p.Type == "object"))
+						return true;
+
+					break;
+			}
 
 			return IsThrowable () && ThrowableRequiresNew.Contains (memberName);
 		}