[Java.Interop.Tools.JavaTypeSystem] Prefer !synthetic base methods (#1110)

Context: dotnet/android-libraries#723

Imagine we have [the following Java][0]:

	public abstract class Key {
	  public abstract Parameters getParameters();
	}

	public abstract class StreamingAeadKey extends Key {
	  @OverRide
	  public abstract StreamingAeadParameters getParameters();
	}

	public final class AesGcmHkdfStreamingKey extends StreamingAeadKey {
	  @OverRide
	  public AesGcmHkdfStreamingParameters getParameters() { ... }
	}

The reabstraction in `StreamingAeadKey.getParameters()` causes an
additional synthetic `getParameters()` method to be generated:

	<class name="StreamingAeadKey" abstract="true" deprecated="not deprecated"
	    jni-extends="Lcom/google/crypto/tink/Key;" extends="com.google.crypto.tink.Key" extends-generic-aware="com.google.crypto.tink.Key" final="false"
	    jni-signature="Lcom/google/crypto/tink/streamingaead/StreamingAeadKey;"
	    source-file-name="StreamingAeadKey.java" static="false" visibility="public">
	  <method abstract="false" name="getParameters" deprecated="not deprecated"
	      jni-signature="()Lcom/google/crypto/tink/Parameters;"
	      final="false" native="false" bridge="true" synthetic="true"
	      return="com.google.crypto.tink.Parameters" jni-return="Lcom/google/crypto/tink/Parameters;"
	      static="false" synchronized="false" visibility="public"
	  />
	  <method abstract="true" name="getParameters" deprecated="not deprecated"
	      jni-signature="()Lcom/google/crypto/tink/streamingaead/StreamingAeadParameters;"
	      final="false" native="false" bridge="false" synthetic="false"
	      return="com.google.crypto.tink.streamingaead.StreamingAeadParameters" jni-return="Lcom/google/crypto/tink/streamingaead/StreamingAeadParameters;"
	      static="false" synchronized="false" visibility="public"
	  />
	</class>

Rephrasing in Java, we have:

	/* partial */ class StreamingAeadKey extends Key {
	    // abstract=false, bridge=true, synthetic=true
	    public Parameters getParameters() {…}

	    // abstract=true, bridge=false, synthetic=false
	    public abstract StreamingAeadParameters getParameters();
	}

When looking for the "base method" for
`AesGcmHkdfStreamingKey.getParameters()` we grab the "first" one we
find with a matching name and parameters.  In this case, it is the
synthetic method instead of the "regular" method.

Note that the synthetic method is not `abstract`.  When deciding if
we need to output `AesGcmHkdfStreamingKey.getParameters()`, we see
that both it and the "base method" are not `abstract`, so we don't
need to bother outputting the derived method.

In reality, the base method should be the `abstract` method.  Then it
is different from the derived non-abstract method, and we will output
the derived method to `api.xml.adjusted`.

The fix is to prefer non-`synthetic`, non-`bridge` methods when
choosing base methods.

[0]: https://github.com/google/tink/blob/9bc2667963e20eb42611b7581e570f0dddf65a2b/java_src/src/main/java/com/google/crypto/tink/Key.java

Author: jpobst

src/Java.Interop.Tools.JavaTypeSystem/JavaModels/JavaMethodModel.cs

@@ -77,7 +77,15 @@ public void FindBaseMethod (JavaClassModel? type)
 
 			var pt = (JavaClassModel)DeclaringType;
 
-			var candidate = type.Methods.FirstOrDefault (p => p.Name == Name && IsImplementing (this, p, pt.GenericInheritanceMapping ?? throw new InvalidOperationException ($"missing {nameof (pt.GenericInheritanceMapping)}!")));
+			var candidates = type.Methods.Where (p => p.Name == Name && IsImplementing (this, p, pt.GenericInheritanceMapping ?? throw new InvalidOperationException ($"missing {nameof (pt.GenericInheritanceMapping)}!")));
+
+			JavaMethodModel? candidate;
+
+			// Prefer non-synthetic, non-bridge methods
+			if (candidates.FirstOrDefault (c => !c.IsSynthetic && !c.IsBridge) is JavaMethodModel jm)
+				candidate = jm;
+			else
+				candidate = candidates.FirstOrDefault ();
 
 			if (candidate != null) {
 				BaseMethod = candidate;

tests/Java.Interop.Tools.JavaTypeSystem-Tests/BaseMethodTests.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Linq;
+using System.Text;
+using System.Xml;
 using Java.Interop.Tools.JavaTypeSystem.Models;
 using NUnit.Framework;
 
@@ -83,5 +85,57 @@ public void GenericConstructors ()
 			var m = t.Constructors.FirstOrDefault ();
 			Assert.IsNotNull (m.TypeParameters, "constructor not found");
 		}
+
+		[Test]
+		public void PreferRealApi ()
+		{
+			// Our base method is abstract, and our derived method is not.  However their is also a "matching" non-abstract
+			// base method that is "synthetic, bridge".  If this method is chosen as the base then we will not write the derived
+			// method because it is not "different" from the base.  (They are both not-abstract.)  In reality, we need to
+			// match to the "not-synthetic, not-bridge" method that *is* abstract, so that the derived method is different
+			// enough to get written to the output.
+			// See "JavaXmlApiExporter.SaveMethod ()" for what constitutes "different" enough to be written.
+			string xml = @"<api>
+  <package name='com.google.crypto.tink.streamingaead' jni-name='com/google/crypto/tink/streamingaead'>
+  
+    <class abstract='true' deprecated='not deprecated' jni-extends='Ljava/lang/Object;' extends='java.lang.Object' extends-generic-aware='java.lang.Object' final='false' name='StreamingAeadKey' jni-signature='Lcom/google/crypto/tink/streamingaead/StreamingAeadKey;' source-file-name='StreamingAeadKey.java' static='false' visibility='public'>
+      <method abstract='false' deprecated='not deprecated' final='false' name='getParameters' native='false' return='java.lang.Object' jni-return='Ljava/lang/Object;' static='false' synchronized='false' visibility='public' bridge='true' synthetic='true' jni-signature='()Ljava/lang/Object;' />
+      <method abstract='true' deprecated='not deprecated' final='false' name='getParameters' native='false' return='java.lang.Object' jni-return='Ljava/lang/Object;' static='false' synchronized='false' visibility='public' bridge='false' synthetic='false' jni-signature='()Ljava/lang/Object;' />
+    </class>
+    
+    <class abstract='false' deprecated='not deprecated' jni-extends='Lcom/google/crypto/tink/streamingaead/StreamingAeadKey;' extends='com.google.crypto.tink.streamingaead.StreamingAeadKey' extends-generic-aware='com.google.crypto.tink.streamingaead.StreamingAeadKey' final='true' name='AesGcmHkdfStreamingKey' jni-signature='Lcom/google/crypto/tink/streamingaead/AesGcmHkdfStreamingKey;' source-file-name='AesGcmHkdfStreamingKey.java' static='false' visibility='public'>
+      <method abstract='false' deprecated='not deprecated' final='false' name='getParameters' native='false' return='java.lang.Object' jni-return='Ljava/lang/Object;' static='false' synchronized='false' visibility='public' bridge='false' synthetic='false' jni-signature='()Ljava/lang/Object;' />
+    </class>
+  
+  </package>
+</api>";
+
+			var xapi = JavaApiTestHelper.GetLoadedApi ();
+			JavaXmlApiImporter.ParseString (xml, xapi);
+
+			var results = xapi.ResolveCollection ();
+
+			var t = xapi.Packages ["com.google.crypto.tink.streamingaead"].Types.First (_ => _.Name == "AesGcmHkdfStreamingKey") as JavaClassModel;
+			var m = t.Methods.FirstOrDefault ();
+
+			// The non-synthetic, non-bridge, abstract base method should be chosen
+			Assert.IsFalse (m.BaseMethod.IsSynthetic);
+			Assert.IsFalse (m.BaseMethod.IsBridge);
+			Assert.IsTrue (m.BaseMethod.IsAbstract);
+
+			var sb = new StringBuilder ();
+
+			// Write the results out to XML
+			using (var xw = XmlWriter.Create (sb))
+				JavaXmlApiExporter.Save (xapi, xw);
+
+			// Read results back in to ensure AesGcmHkdfStreamingKey.getParameters was output
+			var new_collection = JavaXmlApiImporter.ParseString (sb.ToString ());
+
+			var t2 = new_collection.Packages ["com.google.crypto.tink.streamingaead"].Types.First (_ => _.Name == "AesGcmHkdfStreamingKey") as JavaClassModel;
+			var m2 = t.Methods.FirstOrDefault ();
+
+			Assert.IsNotNull (m2);
+		}
 	}
 }