[generator] Only apply ConstSugar to Mono.Android.dll (#780)

Fixes: #752

Context: xamarin/GooglePlayServicesComponents#404

The Android API has an "interesting" pattern where they have an
interface with some static constants, and then they inherit that
interface in other types to inherit those constants.

For example, [`android.provider.BaseColumns`][0]:

	// Java
	public interface BaseColumns {
	    public static final String _COUNT = "_count";
	    public static final String _ID = "_id";
	}

This interface is inherited by ~115 other types, including other
interfaces that only have static constant fields, such as
[CalendarContract.CalendarSyncColumns][1].  This made for a very messy
API when we bound it due the constants being copied into our
alternative `IBaseColumnsConsts` and `ICalendarSyncColumnsConsts`
classes.

Therefore we decided to call this pattern "ConstSugar" and we had
`generator` remove these interfaces, as we believed that they did not
provide value to the managed API.

However, this logic is applied to all binding libraries, and there can
exist marker interfaces that meet the same criteria as ConstSugar, and
they get silently removed from binding libraries.

	// Java
	public interface CanSerialize {
	    public static final int VERSION = 1;
	}

	public interface CanJsonSerialize implements CanSerialize {
	}

In this example we would not bind the `CanJsonSerialize` interface,
because the presence of the `CanSerialize.VERSION` field led us to
treat the `CanSerialize` interface as a ConstSugar type.

Worse, an interface can go from not being considered as ConstSugar to
triggering ConstSugar simply by *additions* to an API:
if `CanSerialize` originally had no fields, both interfaces would be
generated.  Once `VERSION` is added, `CanJsonSerialize` is considered
ConstSugar and will get removed.

Since this logic is very specific to `Mono.Android`, we're going to
use `CodeGenerationOptions.BuildingCoreAssembly` to determine if we're
building `Mono.Android.dll`, and only run ConstSugar logic when true.
This will prevent the logic from running on user binding projects and
causing unexplained missing API for users.

[0]: https://developer.android.com/reference/android/provider/BaseColumns
[1]: https://developer.android.com/reference/android/provider/CalendarContract.CalendarSyncColumns?hl=en

Author: jpobst

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

@@ -48,6 +48,13 @@ protected override CodeGenerationOptions CreateOptions ()
 		[Test]
 		public void WriteConstSugarInterfaceFields ()
 		{
+			// Need a JLO class "FromXml" to trigger ConstSugar logic. (ie: this is "building" Mono.Android.dll)
+			var klass = new TestClass ("java.lang.Object", "java.lang.Object") {
+				FromXml = true,
+			};
+
+			options.SymbolTable.AddType (klass);
+
 			// This is an interface that only has fields (IsConstSugar)
 			// We treat   a little differenly because they don't need to interop with Java
 			var iface = SupportTypeBuilder.CreateEmptyInterface ("java.code.IMyInterface");

tools/generator/CodeGenerationOptions.cs

@@ -59,8 +59,10 @@ internal CodeGenerator CreateCodeGenerator (TextWriter writer)
 		public bool SupportNestedInterfaceTypes { get; set; }
 		public bool SupportNullableReferenceTypes { get; set; }
 		public bool UseShallowReferencedTypes { get; set; }
+		public bool RemoveConstSugar => BuildingCoreAssembly;
 
 		bool? buildingCoreAssembly;
+		// Basically this means "Are we building Mono.Android.dll?"
 		public bool BuildingCoreAssembly {
 			get {
 				return buildingCoreAssembly ?? (buildingCoreAssembly = (SymbolTable.Lookup ("java.lang.Object") is ClassGen gen && gen.FromXml)).Value;

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

@@ -55,7 +55,7 @@ public void WriteClass (ClassGen @class, string indent, GenerationInfo gen_info)
 			foreach (ISymbol isym in @class.Interfaces) {
 				GenericSymbol gs = isym as GenericSymbol;
 				InterfaceGen gen = (gs == null ? isym : gs.Gen) as InterfaceGen;
-				if (gen != null && (gen.IsConstSugar || gen.RawVisibility != "public"))
+				if (gen != null && (gen.IsConstSugar (opt) || gen.RawVisibility != "public"))
 					continue;
 				if (sb.Length > 0)
 					sb.Append (", ");
@@ -465,13 +465,13 @@ public void WriteInterface (InterfaceGen @interface, string indent, GenerationIn
 
 			// If this interface is just fields and we can't generate any of them
 			// then we don't need to write the interface
-			if (@interface.IsConstSugar && @interface.GetGeneratableFields (opt).Count () == 0)
+			if (@interface.IsConstSugar (opt) && @interface.GetGeneratableFields (opt).Count () == 0)
 				return;
 
 			WriteInterfaceDeclaration (@interface, indent, gen_info);
 
 			// If this interface is just constant fields we don't need to write all the invoker bits
-			if (@interface.IsConstSugar)
+			if (@interface.IsConstSugar (opt))
 				return;
 
 			if (!@interface.AssemblyQualifiedName.Contains ('/'))
@@ -514,7 +514,7 @@ public void WriteInterfaceDeclaration (InterfaceGen @interface, string indent, G
 			StringBuilder sb = new StringBuilder ();
 			foreach (ISymbol isym in @interface.Interfaces) {
 				InterfaceGen igen = (isym is GenericSymbol ? (isym as GenericSymbol).Gen : isym) as InterfaceGen;
-				if (igen.IsConstSugar || igen.RawVisibility != "public")
+				if (igen.IsConstSugar (opt) || igen.RawVisibility != "public")
 					continue;
 				if (sb.Length > 0)
 					sb.Append (", ");
@@ -526,7 +526,7 @@ public void WriteInterfaceDeclaration (InterfaceGen @interface, string indent, G
 			if (@interface.IsDeprecated)
 				writer.WriteLine ("{0}[ObsoleteAttribute (@\"{1}\")]", indent, @interface.DeprecatedComment);
 
-			if (!@interface.IsConstSugar) {
+			if (!@interface.IsConstSugar (opt)) {
 				var signature = string.IsNullOrWhiteSpace (@interface.Namespace)
 					? @interface.FullName.Replace ('.', '/')
 					: @interface.Namespace + "." + @interface.FullName.Substring (@interface.Namespace.Length + 1).Replace ('.', '/');
@@ -537,7 +537,7 @@ public void WriteInterfaceDeclaration (InterfaceGen @interface, string indent, G
 			if (@interface.TypeParameters != null && @interface.TypeParameters.Any ())
 				writer.WriteLine ("{0}{1}", indent, @interface.TypeParameters.ToGeneratedAttributeString ());
 			writer.WriteLine ("{0}{1} partial interface {2}{3} {{", indent, @interface.Visibility, @interface.Name,
-				@interface.IsConstSugar ? string.Empty : @interface.Interfaces.Count == 0 || sb.Length == 0 ? " : " + GetAllInterfaceImplements () : " : " + sb.ToString ());
+				@interface.IsConstSugar (opt) ? string.Empty : @interface.Interfaces.Count == 0 || sb.Length == 0 ? " : " + GetAllInterfaceImplements () : " : " + sb.ToString ());
 
 			if (opt.SupportDefaultInterfaceMethods && (@interface.HasDefaultMethods || @interface.HasStaticMethods))
 				WriteClassHandle (@interface, indent + "\t", @interface.Name);

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

@@ -158,22 +158,24 @@ public IEnumerable<Field> GetGeneratableFields (CodeGenerationOptions options)
 
 		public bool HasStaticMethods => GetAllMethods ().Any (m => m.IsStatic);
 
-		public bool IsConstSugar {
-			get {
-				if (Methods.Count > 0 || Properties.Count > 0)
-					return false;
+		public bool IsConstSugar (CodeGenerationOptions options)
+		{
+			if (!options.RemoveConstSugar)
+				return false;
 
-				foreach (InterfaceGen impl in GetAllDerivedInterfaces ())
-					if (!impl.IsConstSugar)
-						return false;
+			if (Methods.Count > 0 || Properties.Count > 0)
+				return false;
 
-				// Need to keep Java.IO.ISerializable as a "marker interface"; want to
-				// hide android.provider.ContactsContract.DataColumnsWithJoins
-				if (Fields.Count == 0 && Interfaces.Count == 0)
+			foreach (InterfaceGen impl in GetAllDerivedInterfaces ())
+				if (!impl.IsConstSugar (options))
 					return false;
 
-				return true;
-			}
+			// Need to keep Java.IO.ISerializable as a "marker interface"; want to
+			// hide android.provider.ContactsContract.DataColumnsWithJoins
+			if (Fields.Count == 0 && Interfaces.Count == 0)
+				return false;
+
+			return true;
 		}
 
 		// If there is a property it cannot generate valid implementor, so reject this at least so far.

tools/generator/SourceWriters/BoundClass.cs

@@ -145,7 +145,7 @@ void AddCharSequenceEnumerator (ClassGen klass)
 		void AddImplementedInterfaces (ClassGen klass)
 		{
 			foreach (var isym in klass.Interfaces) {
-				if ((!(isym is GenericSymbol gs) ? isym : gs.Gen) is InterfaceGen gen && (gen.IsConstSugar || gen.RawVisibility != "public"))
+				if ((!(isym is GenericSymbol gs) ? isym : gs.Gen) is InterfaceGen gen && (gen.IsConstSugar (opt) || gen.RawVisibility != "public"))
 					continue;
 
 				Implements.Add (opt.GetOutputName (isym.FullName));

tools/generator/SourceWriters/BoundInterface.cs

@@ -26,7 +26,7 @@ public BoundInterface (InterfaceGen iface, CodeGenerationOptions opt, CodeGenera
 			// If this interface is just fields and we can't generate any of them
 			// then we don't need to write the interface.  We still keep this type
 			// because it may have nested types or need an InterfaceMemberAlternativeClass.
-			if (iface.IsConstSugar && iface.GetGeneratableFields (opt).Count () == 0) {
+			if (iface.IsConstSugar (opt) && iface.GetGeneratableFields (opt).Count () == 0) {
 				dont_generate = true;
 				return;
 			}
@@ -43,7 +43,7 @@ public BoundInterface (InterfaceGen iface, CodeGenerationOptions opt, CodeGenera
 			if (iface.IsDeprecated)
 				Attributes.Add (new ObsoleteAttr (iface.DeprecatedComment) { WriteAttributeSuffix = true, WriteEmptyString = true });
 
-			if (!iface.IsConstSugar) {
+			if (!iface.IsConstSugar (opt)) {
 				var signature = string.IsNullOrWhiteSpace (iface.Namespace)
 					? iface.FullName.Replace ('.', '/')
 					: iface.Namespace + "." + iface.FullName.Substring (iface.Namespace.Length + 1).Replace ('.', '/');
@@ -63,7 +63,7 @@ public BoundInterface (InterfaceGen iface, CodeGenerationOptions opt, CodeGenera
 			AddNestedTypes (iface, opt, context, genInfo);
 
 			// If this interface is just constant fields we don't need to add all the invoker bits
-			if (iface.IsConstSugar)
+			if (iface.IsConstSugar (opt))
 				return;
 
 			if (!iface.AssemblyQualifiedName.Contains ('/')) {
@@ -137,13 +137,13 @@ void AddInheritedInterfaces (InterfaceGen iface, CodeGenerationOptions opt)
 			foreach (var isym in iface.Interfaces) {
 				var igen = (isym is GenericSymbol ? (isym as GenericSymbol).Gen : isym) as InterfaceGen;
 
-				if (igen.IsConstSugar || igen.RawVisibility != "public")
+				if (igen.IsConstSugar (opt) || igen.RawVisibility != "public")
 					continue;
 
 				Implements.Add (opt.GetOutputName (isym.FullName));
 			}
 
-			if (Implements.Count == 0 && !iface.IsConstSugar)
+			if (Implements.Count == 0 && !iface.IsConstSugar (opt))
 				Implements.AddRange (new [] { "IJavaObject", "IJavaPeerable" });
 		}