[api-xml-adjuster] method overrides cannot expose non-public types. (dotnet#94)

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=44531

When a Java method is overriden, its parameter type or return type can be
variant, which possibly result in exposure of non-public types (returning
private class object which is derived from public type is totally valid).

It is because we blindly emit the parsed type from class-parse output
as the API XML adjuster output. It needs to be examined and adjusted to
be public (or protected) type.

jar2xml didn't have this problem because Java reflection API is clever
enough to cover that issue.

The fixes are... not cosmetic.

Basically now we use TypeReference.ToString() which used to not be
precise and ready for API XML output. So there are couple of fixes
for that. (We also want to track from which method base a parameter
or return type is bound, so we pass that to the .ctor now).

We hide extraneous public TypeParameter.ctor() now to reduce chances
for bugs. Tests need to be modified to reflect the changes.

Lastly, Mono.Android API XML will be updated to reflect this change.
There will be some removal of method overrides from api-*.xml.in but
there was no regression reported from api-diff.

Author: atsushieno

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApi.XmlModel.cs

@@ -253,6 +253,12 @@ public override string ToString ()
 
 	public partial class JavaParameter
 	{
+		public JavaParameter (JavaMethodBase parent)
+		{
+			Parent = parent;
+		}
+
+		public JavaMethodBase Parent { get; private set; }
 		public string Name { get; set; }
 		public string Type { get; set; }
 
@@ -309,7 +315,7 @@ public JavaTypeParameter (JavaTypeParameters parent)
 
 		public override string ToString ()
 		{
-			return Name + (GenericConstraints == null ? null : GenericConstraints.ToString ());
+			return Name;
 		}
 	}
 
@@ -326,7 +332,10 @@ public JavaGenericConstraints ()
 
 		public override string ToString ()
 		{
-			return " " + (BoundsType ?? "extends") + " " + string.Join (" & ", GenericConstraints);
+			string csts = string.Join (" & ", GenericConstraints);
+			if (csts == "java.lang.Object")
+				return string.Empty;
+			return " " + (BoundsType ?? "extends") + " " + csts;
 		}
 	}
 

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiFixVisibilityExtensions.cs

@@ -0,0 +1,52 @@
+using System;
+using System.Linq;
+
+namespace Xamarin.Android.Tools.ApiXmlAdjuster
+{
+	public static class JavaApiFixVisibilityExtensions
+	{
+		public static string GetVisibleTypeName (this JavaParameter parameter)
+		{
+			var r = GetVisibleNonSpecialType (parameter);
+			return r != null ? r.ToString () : parameter.Type;
+		}
+
+		public static string GetVisibleReturnTypeName (this JavaMethod method)
+		{
+			var r = GetVisibleNonSpecialReturnType (method);
+			return r != null ? r.ToString () : method.Return;
+		}
+
+		public static JavaTypeReference GetVisibleNonSpecialType (this JavaParameter parameter)
+		{
+			return GetVisibleNonSpecialType (parameter.Parent, parameter.ResolvedType);
+		}
+
+		public static JavaTypeReference GetVisibleNonSpecialReturnType (this JavaMethod method)
+		{
+			return GetVisibleNonSpecialType (method, method.ResolvedReturnType);
+		}
+
+		static JavaTypeReference GetVisibleNonSpecialType (this JavaMethodBase method, JavaTypeReference r)
+		{
+			if (r == null || r.SpecialName != null || r.ReferencedTypeParameter != null || r.ArrayPart != null)
+				return null;
+			var requiredVisibility = method.Visibility == "public" && method.Parent.Visibility == "public" ? "public" : method.Visibility;
+			for (var t = r; t != null; t = (t.ReferencedType as JavaClass)?.ResolvedExtends) {
+				if (t.ReferencedType == null)
+					break;
+				if (IsAcceptableVisibility (required: requiredVisibility, actual: t.ReferencedType.Visibility))
+					return t;
+			}
+			return null;
+		}
+
+		static bool IsAcceptableVisibility (string required, string actual)
+		{
+			if (required == "public")
+				return actual == "public";
+			else
+				return true;
+		}
+	}
+}

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiTypeResolverExtensions.cs

@@ -24,9 +24,11 @@ static JavaTypeReference JavaTypeNameToReference (this JavaApi api, JavaTypeName
 			var tp = contextTypeParameters.Where (tps => tps != null).SelectMany (tps => tps.TypeParameters).FirstOrDefault (_ => _.Name == tn.FullNameNonGeneric);
 			if (tp != null)
 				return new JavaTypeReference (tp, tn.ArrayPart);
+			if (tn.FullNameNonGeneric == JavaTypeReference.GenericWildcard.SpecialName)
+				return new JavaTypeReference (tn.BoundsType, tn.GenericConstraints?.Select (gc => JavaTypeNameToReference (api, gc, contextTypeParameters)), tn.ArrayPart);
 			var primitive = JavaTypeReference.GetSpecialType (tn.FullNameNonGeneric);
 			if (primitive != null)
-				return tn.ArrayPart == null ? primitive : new JavaTypeReference (primitive, tn.ArrayPart);
+				return tn.ArrayPart == null && tn.GenericConstraints == null ? primitive : new JavaTypeReference (primitive, tn.ArrayPart, tn.BoundsType, tn.GenericConstraints?.Select (gc => JavaTypeNameToReference (api, gc, contextTypeParameters)));
 			var type = api.FindNonGenericType (tn.FullNameNonGeneric);
 			return new JavaTypeReference (type,
 				tn.GenericArguments != null ? tn.GenericArguments.Select (_ => api.JavaTypeNameToReference (_, contextTypeParameters)).ToArray () : null,
@@ -111,7 +113,7 @@ static void ResolveMethodBase (this JavaMethodBase m, JavaTypeParameters methodT
 		public static void Resolve (this JavaMethod m)
 		{
 			if (m.TypeParameters != null)
-				m.TypeParameters.Resolve (m.GetApi (), m.Parent.TypeParameters);
+				m.TypeParameters.Resolve (m.GetApi (), m.TypeParameters);
 			m.ResolveMethodBase (m.TypeParameters);
 			m.ResolvedReturnType = m.GetApi ().Parse (m.Return, m.Parent.TypeParameters, m.TypeParameters);
 		}

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiXmlGeneratorExtensions.cs

@@ -176,7 +176,7 @@ static void Save (this JavaMethod method, XmlWriter writer)
 			SaveCommon (method, writer, "method",
 				XmlConvert.ToString (method.Abstract),
 				XmlConvert.ToString (method.Native),
-				method.Return,
+				method.GetVisibleReturnTypeName (),
 				XmlConvert.ToString (method.Synchronized),
 				null,
 				null,
@@ -232,8 +232,8 @@ static void SaveCommon (this JavaMember m, XmlWriter writer, string elementName,
 				foreach (var p in parameters) {
 					writer.WriteStartElement ("parameter");
 					writer.WriteAttributeString ("name", p.Name);
-					writer.WriteAttributeString ("type", p.Type);
-					writer.WriteString ("\n        ");			
+					writer.WriteAttributeString ("type", p.GetVisibleTypeName ());
+					writer.WriteString ("\n        ");
 					writer.WriteFullEndElement ();
 				}
 			}

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaApiXmlLoaderExtensions.cs

@@ -242,7 +242,7 @@ static void LoadMethodBase (this JavaMethodBase methodBase, string elementName,
 						tp.Load (reader);
 						method.TypeParameters = tp;
 					} else if (reader.LocalName == "parameter") {
-						var p = new JavaParameter ();
+						var p = new JavaParameter (methodBase);
 						p.Load (reader);
 						methodBase.Parameters.Add (p);
 					} else if (reader.LocalName == "exception") {

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaTypeName.cs

@@ -19,14 +19,14 @@ namespace Xamarin.Android.Tools.ApiXmlAdjuster
 	//
 	// A generic type parameter, or a primitive type, is also represented by this too.
 	//
-	internal class JavaTypeName
+	public class JavaTypeName
 	{
 		const string extendsLabel = " extends ";
 		const string superLabel = " super ";
 
 		static readonly string [] genericConstraintsLabels = { extendsLabel, superLabel };
 
-		private JavaTypeName ()
+		JavaTypeName ()
 		{
 		}
 
@@ -40,6 +40,7 @@ public static JavaTypeName Parse (string dottedFullName)
 				int gccidx = gcidx < 0 ? -1 : dottedFullName.IndexOf (',', 0, gcidx);
 				if (gcidx > 0 && gcgidx < 0 && gccidx < 0) {
 					string args = dottedFullName.Substring (gcidx + label.Length).Trim ();
+					ret.BoundsType = label;
 					ret.GenericConstraints = ParseCommaSeparatedTypeNames (args).Select (s => Parse (s)).ToArray ();
 					dottedFullName = dottedFullName.Substring (0, gcidx).Trim ();
 				}
@@ -69,7 +70,7 @@ public static JavaTypeName Parse (string dottedFullName)
 
 			return ret;
 		}
-		
+
 		static IEnumerable<string> ParseCommaSeparatedTypeNames (string args)
 		{
 			int comma = args.IndexOf (',');
@@ -79,7 +80,7 @@ static IEnumerable<string> ParseCommaSeparatedTypeNames (string args)
 				int open = args.IndexOf ('<', 0, comma);
 				if (open > 0) {
 					int openCount = 1;
-					int i = open;
+					int i = open + 1;
 					while (i < args.Length) {
 						if (args [i] == '<')
 							openCount++;
@@ -90,10 +91,11 @@ static IEnumerable<string> ParseCommaSeparatedTypeNames (string args)
 							break;
 					}
 					yield return args.Substring (0, i);
-					if (i > args.Length) {
+					if (i < args.Length) {
 						comma = args.IndexOf (',', i);
-						foreach (var s in ParseCommaSeparatedTypeNames (args.Substring (comma + 1)))
-							yield return s;
+						if (comma > 0)
+							foreach (var s in ParseCommaSeparatedTypeNames (args.Substring (comma + 1)))
+								yield return s;
 					}
 				} else {
 					yield return args.Substring (0, comma);
@@ -104,6 +106,7 @@ static IEnumerable<string> ParseCommaSeparatedTypeNames (string args)
 		}
 
 		public string FullNameNonGeneric { get; set; }
+		public string BoundsType { get; set; } // " extends " / " super "
 		public IList<JavaTypeName> GenericConstraints { get; private set; }
 		public IList<JavaTypeName> GenericArguments { get; private set; }
 		public string ArrayPart { get; set; }

src/Xamarin.Android.Tools.ApiXmlAdjuster/JavaTypeReference.cs

@@ -48,17 +48,28 @@ static JavaTypeReference ()
 			GenericWildcard = new JavaTypeReference ("?");
 		}
 
-		public JavaTypeReference (string specialName)
+		JavaTypeReference (string specialName)
 		{
 			SpecialName = specialName;
 		}
 
-		public JavaTypeReference (JavaTypeReference referencedType, string arrayPart)
+		public JavaTypeReference (string constraintLabel, IEnumerable<JavaTypeReference> wildcardConstraints, string arrayPart)
+		{
+			SpecialName = GenericWildcard.SpecialName;
+			ArrayPart = arrayPart;
+			WildcardBoundsType = constraintLabel;
+			WildcardConstraints = wildcardConstraints != null && wildcardConstraints.Any () ? wildcardConstraints.ToList () : null;
+		}
+
+		public JavaTypeReference (JavaTypeReference referencedType, string arrayPart, string wildcardBoundsType, IEnumerable<JavaTypeReference> wildcardConstraints)
 		{
 			if (referencedType == null)
 				throw new ArgumentNullException ("referencedType");
 			SpecialName = referencedType.SpecialName;
+			WildcardBoundsType = wildcardBoundsType;
+			WildcardConstraints = wildcardConstraints?.ToList ();
 			ReferencedType = referencedType.ReferencedType;
+			ReferencedTypeParameter = referencedType.ReferencedTypeParameter;
 			TypeParameters = referencedType.TypeParameters;
 			ArrayPart = arrayPart;
 		}
@@ -81,17 +92,23 @@ public JavaTypeReference (JavaType referencedType, IList<JavaTypeReference> type
 		}
 
 		public string SpecialName { get; private set; }
+
+		public string WildcardBoundsType { get; private set; }
+		public IList<JavaTypeReference> WildcardConstraints { get; private set; }
+
 		public JavaType ReferencedType { get; private set; }
 		public JavaTypeParameter ReferencedTypeParameter { get; private set; }
 		public IList<JavaTypeReference> TypeParameters { get; private set; }
 		public string ArrayPart { get; private set; }
 
 		public override string ToString ()
 		{
-			if (SpecialName != null)
-				return SpecialName;
+			if (SpecialName == GenericWildcard.SpecialName && WildcardConstraints != null)
+				return SpecialName + WildcardBoundsType + string.Join (" & ", WildcardConstraints);
+			else if (SpecialName != null)
+				return SpecialName + ArrayPart;
 			else if (ReferencedTypeParameter != null)
-				return ReferencedTypeParameter.Name;
+				return ReferencedTypeParameter.ToString () + ArrayPart;
 			else
 				return string.Format ("{0}{1}{2}{3}{4}",
 					ReferencedType.Parent.Name,

src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/JavaApiTest.cs

@@ -24,6 +24,24 @@ public void TestToString ()
 			var kls = pkg.Types.First (t => t.FullName == "android.database.ContentObservable");
 			Assert.AreEqual ("[Class] android.database.ContentObservable", kls.ToString ());
 		}
+
+		[Test]
+		public void ParseName ()
+		{
+			var tn = JavaTypeName.Parse ("java.util.Function<java.util.Map.Entry<K, V>, ? extends U>");
+			Assert.AreEqual ("java.util.Function", tn.FullNameNonGeneric, "top failed to parse name");
+			Assert.AreEqual (2, tn.GenericArguments.Count, "top incorrect number of parsed generic arguments");
+			var ga1 = tn.GenericArguments [0];
+			Assert.AreEqual ("java.util.Map.Entry", ga1.FullNameNonGeneric, "genarg#0 name mismatch");
+			Assert.AreEqual (2, ga1.GenericArguments.Count, "genarg#0 incorrect number of parsed generic arguments");
+			Assert.AreEqual ("K", ga1.GenericArguments [0].FullNameNonGeneric, "genarg#0.1 name mismatch");
+			Assert.AreEqual ("V", ga1.GenericArguments [1].FullNameNonGeneric, "genarg#0.2 name mismatch");
+			var ga2 = tn.GenericArguments [1];
+			Assert.AreEqual ("?", ga2.FullNameNonGeneric, "genarg#1 name mismatch");
+			Assert.AreEqual (" extends ", ga2.BoundsType, "genarg#1 incorrect bounds type");
+			Assert.AreEqual (1, ga2.GenericConstraints.Count, "genarg#1 incorrect number of parsed generic constraints");
+			Assert.AreEqual ("U", ga2.GenericConstraints [0].FullNameNonGeneric, "genarg#1.1 constraint name mismatch");
+		}
 	}
 }
 

src/Xamarin.Android.Tools.ApiXmlAdjuster/Tests/JavaApiTestHelper.cs

@@ -16,7 +16,7 @@ public class JavaApiTestHelper
 				"src",
 				"Xamarin.Android.Tools.ApiXmlAdjuster",
 				"Tests",
-				"api-10.xml.in");
+				"api-24.xml.in");
 
 		public static JavaApi GetLoadedApi ()
 		{