[Xamarin.Android.Build.Tasks] XA4214, XA4215 in GenerateJavaStubs (#2349

)

Fixes: #2266

Context: #1560

Java type names are not managed type names, and vice versa; there is
a *mapping* between them, e.g. the Java class`java.lang.Object` is
mapped (bound) as the managed type `Java.Lang.Object`.

Ideally, there would be a 1:1 mapping between these names, e.g.
`java.lang.Object` is *always* mapped to `Java.Lang.Object`, but
*this is not required*, and occasionally there are "duplicate
mappings"; for example, the Java class `java.util.HashSet` is
mapped to `Java.Util.HashSet`, `Android.Runtime.JavaSet`, *and*
`Android.Runtime.JavaSet<T>`:

	namespace Java.Util {
	  [Register ("java/util/HashSet", DoNotGenerateAcw=true, ApiSince = 1)]
	  public partial class HashSet {}
	}
	namespace Android.Runtime {
	  [Register ("java/util/HashSet", DoNotGenerateAcw=true)]
	  public partial class JavaSet {}
	  [Register ("java/util/HashSet", DoNotGenerateAcw=true)]
	  public partial class JavaSet<T> : JavaSet, ICollection<T> {}
	}

Sometimes such 1:N mapping is acceptable, and sometimes it isn't.

When is it acceptable?  When Java types *are not generated*, as
above.  The above results in an *ambiguity* within
`Java.Lang.Object.GetObject()` when given an instance of type
`java.util.HashSet` -- which managed type should be used? -- but the
ambiguity problems can be reduced by using `Object.GetObject<T>()`
and various other means.

When is it *not* acceptable?  When two managed types would result in
the same Java type name.  For example:

	namespace Example {
	  [Register ("type/Collision")]
	  public partial class Bad1 : Java.Lang.Object {}

	  [Register ("type/Collision")]
	  public partial class Bad2 : Java.Lang.Object {}
	}

Attempting to build an App project containing such a set of types
would result in build errors:

	error : Duplicate Java type found! Mappings between managed types and Java types must be unique.
	 First Type: 'Example.Bad1, Assembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null';
	Second Type: 'Example.Bad2, Assembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'

(This doesn't happen with `Java.Util.HashSet` and
`Android.Runtime.JavaSet` because they use
`[Register(DoNotGenerateAcw=true)]`.)

There are three issues that need to be addressed:

 1. The error and warning messages do not contain codes or
    associated documentation.
 2. The error and warning messages are not *actionable*.
 3. The error checking is occasionally missed.

(3) is a problem: Consider if we have two assemblies with the same
Java name and same managed name:

	// This file present in two seprate assemblies
	namespace Example {
	  [Register ("example.EmptyClass")]
	  public class EmptyClass : Java.Lang.Object {}
	}

Building such an app will result in an ambiguity warning:

	warning : Duplicate managed type found! Mappings between managed types and Java types must be unique.
	 First Type: 'Example.EmptyClass, Assembly1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null';
	Second Type: 'Example.EmptyClass, Assembly2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.

However, this should *also* elicit an *error* because of the Java-
side type name collision, but the error isn't emitted!

Fix these three issues:

 1. Emit an XA4214 warning when identical managed types in different
    assemblies are found, such as `Example.EmptyClass`.
    It's only a warning because certain constructs such as
    `<fragment type="Example.EmptyClass, Assembly1">` support
    assembly-qualified names.

 2. Emit an XA4215 error when identical Java types are encountered.

 3. Improve the messages to be actionable.

 4. Add documentation for XA4214 and XA4215. 

 5. Fix the error checking so that if an XA4214 warning is generated,
    the XA4215 error condition is *also* checked.

The `Example.EmptyClass` scenario will result in warning XA4214:

	warning XA4214: The managed type `Example.EmptyClass` exists in multiple assemblies: Assembly1, Assembly2.
	  Please refactor the managed type names in these assemblies so that they are not identical.
	warning XA4214: References to the type `Example.EmptyClass` will refer to `Example.EmptyClass, Assembly1`.

The `Example.EmptyClass` scenario will also result in error XA4215:

	error XA4215: The Jvaa type `example.EmptyClass` is generated by more than one managed type.
	  Please change the [Register] attribute so that the same Java type is not emitted.
	error XA4215:   `example.EmptyClass` generated by: Example.EmptyClass, Assembly1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null
	error XA4215:   `example.EmptyClass` generated by: Example.EmptyClass, Assembly2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null

A possible future enhancement could be to move the warning into the
`<ConvertCustomView/>` task so that it only appears when a managed
type name from an Android resource file matches more than one line in
the `acw-map`.  That way, if a user wants to solve the issue by using
`[Register]` attributes or assembly-qualified type names rather than
renaming the managed types, the build process will automatically stop
showing the warning.

Documentation/guides/messages/README.md

@@ -60,6 +60,8 @@
 
 ### XA4xxx Code Generation
 
++ [XA4214](xa4214.md): The managed type \`Library1.Class1\` exists in multiple assemblies: Library1, Library2. Please refactor the managed type names in these assemblies so that they are not identical.
++ [XA4215](xa4215.md): The Java type \`com.contoso.library1.Class1\` is generated by more than one managed type. Please change the \[Register\] attribute so that the same Java type is not emitted.
 + [XA4301](xa4301.md): : Apk already contains the item `xxx`.
 
 ### XA5xxx GCC and toolchain

Documentation/guides/messages/xa4214.md

@@ -0,0 +1,33 @@
+# Compiler Warning XA4214
+
+If two or more C# types from different assemblies inherit from
+`Java.Lang.Object` and share the same fully qualified name, that name will
+always refer to just *one* of the types when used in Android resource files.
+
+The typical way to resolve this warning is to rename the types so that each
+fully qualified name only exists in one assembly.
+
+Another option is to add `[Register]` attributes on the conflicting managed
+types so that each one has a unique Java type name.
+
+A third option is to qualify the type names with the assembly name in the
+Android resource files.  For example, use the assembly-qualified name
+`Library1.Class1, Library` rather than just `Library1.Class1`.  This only works
+in places where the XML schema allows a type name within an XML attribute.  One
+example is the `class` attribute on `fragment` elements.
+
+If you choose to use `[Register]` attributes or assembly-qualified names rather
+than renaming the managed types, then you can hide the warnings either by adding
+the `/nowarn:XA4214` switch to the MSBuild command line or by adding `XA4214` to
+the `$(NoWarn)` property in your .csproj file:
+
+```xml
+<PropertyGroup>
+    <NoWarn>$(NoWarn);XA4214</NoWarn>
+</PropertyGroup>
+```
+
+Example message:
+
+  * <code>warning XA4214: The managed type \`Library1.Class1\` exists in multiple assemblies: Library1, Library2. Please refactor the managed type names in these assemblies so that they are not identical.</code>  
+    <code>warning XA4214: References to the type \`Library1.Class1\` will refer to \`Library1.Class1, Library1\`.</code>

Documentation/guides/messages/xa4215.md

@@ -0,0 +1,13 @@
+# Compiler Error XA4215
+
+This error indicates that two or more C# types are emitting the same fully
+qualified Java type name.
+
+To resolve this error, change the `[Register]` attribute on one of the C# types
+to a different Java type name.
+
+Example message:
+
+  * <code>error XA4215: The Java type \`com.contoso.library1.Class1\` is generated by more than one managed type. Please change the \[Register\] attribute so that the same Java type is not emitted.</code>  
+    <code>error XA4215:   \`com.contoso.library1.Class1\` generated by: Library1.Class1, Library1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null</code>  
+    <code>error XA4215:   \`com.contoso.library1.Class1\` generated by: Library1.Class1, Library2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null</code>

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

@@ -152,6 +152,10 @@ void Run (DirectoryAssemblyResolver res)
 			// We need to save a map of .NET type -> ACW type for resource file fixups
 			var managed = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
 			var java    = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
+
+			var managedConflicts = new Dictionary<string, List<string>> (0, StringComparer.Ordinal);
+			var javaConflicts    = new Dictionary<string, List<string>> (0, StringComparer.Ordinal);
+
 			// Allocate a MemoryStream with a reasonable guess at its capacity
 			using (var stream = new MemoryStream (java_types.Length * 32))
 			using (var acw_map = new StreamWriter (stream)) {
@@ -165,45 +169,60 @@ void Run (DirectoryAssemblyResolver res)
 					acw_map.WriteLine ();
 
 					TypeDefinition conflict;
+					bool hasConflict = false;
 					if (managed.TryGetValue (managedKey, out conflict)) {
-						Log.LogWarning (
-								"Duplicate managed type found! Mappings between managed types and Java types must be unique. " +
-								"First Type: '{0}'; Second Type: '{1}'.",
-								conflict.GetAssemblyQualifiedName (),
-								type.GetAssemblyQualifiedName ());
-						Log.LogWarning (
-								"References to the type '{0}' will refer to '{1}'.",
-								managedKey, conflict.GetAssemblyQualifiedName ());
-						continue;
+						if (!managedConflicts.TryGetValue (managedKey, out var list))
+							managedConflicts.Add (managedKey, list = new List<string> { conflict.GetPartialAssemblyName () });
+						list.Add (type.GetPartialAssemblyName ());
+						hasConflict = true;
 					}
 					if (java.TryGetValue (javaKey, out conflict)) {
-						Log.LogError (
-								"Duplicate Java type found! Mappings between managed types and Java types must be unique. " +
-								"First Type: '{0}'; Second Type: '{1}'",
-								conflict.GetAssemblyQualifiedName (),
-								type.GetAssemblyQualifiedName ());
+						if (!javaConflicts.TryGetValue (javaKey, out var list))
+							javaConflicts.Add (javaKey, list = new List<string> { conflict.GetAssemblyQualifiedName () });
+						list.Add (type.GetAssemblyQualifiedName ());
 						success = false;
-						continue;
+						hasConflict = true;
+					}
+					if (!hasConflict) {
+						managed.Add (managedKey, type);
+						java.Add (javaKey, type);
+
+						acw_map.Write (managedKey);
+						acw_map.Write (';');
+						acw_map.Write (javaKey);
+						acw_map.WriteLine ();
+
+						acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'));
+						acw_map.Write (';');
+						acw_map.Write (javaKey);
+						acw_map.WriteLine ();
 					}
-
-					managed.Add (managedKey, type);
-					java.Add (javaKey, type);
-
-					acw_map.Write (managedKey);
-					acw_map.Write (';');
-					acw_map.Write (javaKey);
-					acw_map.WriteLine ();
-
-					acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'));
-					acw_map.Write (';');
-					acw_map.Write (javaKey);
-					acw_map.WriteLine ();
 				}
 
 				acw_map.Flush ();
 				MonoAndroidHelper.CopyIfStreamChanged (stream, AcwMapFile);
 			}
 
+			foreach (var kvp in managedConflicts) {
+				Log.LogCodedWarning (
+					"XA4214",
+					"The managed type `{0}` exists in multiple assemblies: {1}. " +
+					"Please refactor the managed type names in these assemblies so that they are not identical.",
+					kvp.Key,
+					string.Join (", ", kvp.Value));
+				Log.LogCodedWarning ("XA4214", "References to the type `{0}` will refer to `{0}, {1}`.", kvp.Key, kvp.Value [0]);
+			}
+
+			foreach (var kvp in javaConflicts) {
+				Log.LogCodedError (
+					"XA4215",
+					"The Java type `{0}` is generated by more than one managed type. " +
+					"Please change the [Register] attribute so that the same Java type is not emitted.",
+					kvp.Key);
+				foreach (var typeName in kvp.Value)
+					Log.LogCodedError ("XA4215", "  `{0}` generated by: {1}", kvp.Key, typeName);
+			}
+
 			// Step 3 - Merge [Activity] and friends into AndroidManifest.xml
 			var manifest = new ManifestDocument (ManifestTemplate, this.Log);
 

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs

@@ -3364,6 +3364,94 @@ public void FastDeploymentDoesNotAddContentProvider ()
 				Assert.IsFalse (StringAssertEx.ContainsText (content, type), $"`{type}` should not exist in `AndroidManifest.xml`!");
 			}
 		}
+
+		[Test]
+		public void DuplicateJCWNames ()
+		{
+			var source = @"[Android.Runtime.Register (""examplelib.EmptyClass"")] public class EmptyClass : Java.Lang.Object { }";
+			var library1 = new XamarinAndroidLibraryProject () {
+				ProjectName = "Library1",
+				Sources = {
+					new BuildItem.Source ("EmptyClass.cs") {
+						TextContent = () => source
+					}
+				}
+			};
+			var library2 = new XamarinAndroidLibraryProject () {
+				ProjectName = "Library2",
+				Sources = {
+					new BuildItem.Source ("EmptyClass.cs") {
+						TextContent = () => source
+					}
+				}
+			};
+			var app = new XamarinAndroidApplicationProject {
+				ProjectName = "App1",
+				References = {
+					new BuildItem ("ProjectReference", "..\\Library1\\Library1.csproj"),
+					new BuildItem ("ProjectReference", "..\\Library2\\Library2.csproj")
+				},
+			};
+			var projectPath = Path.Combine ("temp", TestName);
+			using (var lib1b = CreateDllBuilder (Path.Combine (projectPath, library1.ProjectName), cleanupAfterSuccessfulBuild: false))
+			using (var lib2b = CreateDllBuilder (Path.Combine (projectPath, library2.ProjectName), cleanupAfterSuccessfulBuild: false)) {
+				Assert.IsTrue (lib1b.Build (library1), "Build of Library1 should have succeeded");
+				Assert.IsTrue (lib2b.Build (library2), "Build of Library2 should have succeeded");
+				using (var appb = CreateApkBuilder (Path.Combine (projectPath, app.ProjectName))) {
+					appb.ThrowOnBuildFailure = false;
+					Assert.IsFalse (appb.Build (app), "Build of App1 should have failed");
+					IEnumerable<string> errors = appb.LastBuildOutput.Where (x => x.Contains ("error XA4215"));
+					Assert.NotNull (errors, "Error should be XA4215");
+					StringAssertEx.Contains ("EmptyClass", errors, "Error should mention the conflicting type name");
+					StringAssertEx.Contains ("Library1", errors, "Error should mention all of the assemblies with conflicts");
+					StringAssertEx.Contains ("Library2", errors, "Error should mention all of the assemblies with conflicts");
+				}
+			}
+		}
+
+		[Test]
+		public void DuplicateManagedNames ()
+		{
+			var source = @"public class EmptyClass : Java.Lang.Object { }";
+			var library1 = new XamarinAndroidLibraryProject () {
+				ProjectName = "Library1",
+				Sources = {
+					new BuildItem.Source ("EmptyClass.cs") {
+						TextContent = () => source
+					}
+				}
+			};
+			var library2 = new XamarinAndroidLibraryProject () {
+				ProjectName = "Library2",
+				Sources = {
+					new BuildItem.Source ("EmptyClass.cs") {
+						TextContent = () => source
+					}
+				}
+			};
+			var app = new XamarinAndroidApplicationProject {
+				ProjectName = "App1",
+				References = {
+					new BuildItem ("ProjectReference", "..\\Library1\\Library1.csproj"),
+					new BuildItem ("ProjectReference", "..\\Library2\\Library2.csproj")
+				},
+			};
+			var projectPath = Path.Combine ("temp", TestName);
+			using (var lib1b = CreateDllBuilder (Path.Combine (projectPath, library1.ProjectName), cleanupAfterSuccessfulBuild: false))
+			using (var lib2b = CreateDllBuilder (Path.Combine (projectPath, library2.ProjectName), cleanupAfterSuccessfulBuild: false)) {
+				Assert.IsTrue (lib1b.Build (library1), "Build of Library1 should have succeeded");
+				Assert.IsTrue (lib2b.Build (library2), "Build of Library2 should have succeeded");
+				using (var appb = CreateApkBuilder (Path.Combine (projectPath, app.ProjectName))) {
+					appb.ThrowOnBuildFailure = false;
+					Assert.IsTrue (appb.Build (app), "Build of App1 should have succeeded");
+					IEnumerable<string> warnings = appb.LastBuildOutput.Where (x => x.Contains ("warning XA4214"));
+					Assert.NotNull (warnings, "Warning should be XA4214");
+					StringAssertEx.Contains ("EmptyClass", warnings, "Warning should mention the conflicting type name");
+					StringAssertEx.Contains ("Library1", warnings, "Warning should mention all of the assemblies with conflicts");
+					StringAssertEx.Contains ("Library2", warnings, "Warning should mention all of the assemblies with conflicts");
+				}
+			}
+		}
 	}
 }