[Java.Interop.Tools.JavaCallableWrappers] Error on bad IJavaObject (#179)

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=56819

Nothing *prevents* "anybody" from custom implementing `IJavaObject`:

	class MyBadClass : Android.Runtime.IJavaObject {
		public IntPtr Handle {
			get {return IntPtr.Zero;}
		}

		public void Dispose () {}
	}

The problem is that the above doesn't actually work: the
`IJavaObject.Handle` value contains the JNI Object Reference to pass
into JNI. The above code will thus result in always passing `null`
into Java code, which could result in a `NullPointerException`, but
will always result in *not* doing what was intended.

While it is *theoretically* possible to manually implement
`IJavaObject`, it's not something *I* would want to contemplate, nor
is it for the faint of heart, so long ago we decided to emit a warning
if we encounter a type which:

 1. Implements `Android.Runtime.IJavaObject`, and
 2. Does *not* also inherit `Java.Lang.Object` or
    `Java.Lang.Throwable`.

As such, the above `MyBadClass` would elicit the warning:

	Type 'MyBadClass' implements Android.Runtime.IJavaObject but does not inherit from Java.Lang.Object. It is not supported.

This is all well and good, but (effectively) *nobody* reads warnings,
*especially* since our toolchain emits so many warnings that enabling
`/warnaserror` is for the truly foolhardy, so *lots* of warnings is,
unfortunately, normal.

Which brings us to Bug #56819: Could we make this an error?

Answer: Of course we can. That said, I have no idea how much existing
code this will *break* if we turned it into an error. That might be
good and useful, but if there's no way to *disable* the error,
existing apps which (appear to) work will no longer build.

That's not desirable.

Turn `JavaTypeScanner` from a `static` class into a normal class, and
add a new `JavaTypeScanner.ErrorOnCustomJavaObject` property which,
when true, will cause `JavaTypeScanner` to emit an error instead of
a warning when it encounters types such as `MyBadClass`. (I'm turning
it into a normal class instead of adding a new
`JavaTypeScanner.GetJavaTypes()` overload in case we need to add
additional such "control" parameters in the future. Method overloads
will otherwise get unwieldy.)

`JavaTypeScanner.ErrorOnCustomJavaObject` is all well and good, but
how do we *actually* emit a warning vs. an error? The previous
`Action<string, object[]> log` delegate doesn't allow distinguishing
between warnings and errors (and possible diagnostic messages), so how
would we eventually hook this up into xamarin-android's
`<GenerateJavaStubs/>` task?

Attempt to better handle that question by replacing
`Action<string, object[]>` delegates with `Action<TraceLevel, string>`
delegates. This allows messages to explicitly specify
TraceLevel.Error, TraceLevel.Warning, or TraceLevel.Info (or others!)
so that messages can be sanely processed by downstream users.
Additionally, add a new `Diagnostic.CreateConsoleLogger()` method
which creates an `Action<TraceLevel, string>` which forwards messages
to `Console.Error` or `Console.Out`, as appropriate.

Author: jonpryor

src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs

@@ -28,6 +28,7 @@
 //
 
 using System;
+using System.Diagnostics;
 using System.Collections;
 using System.Collections.Generic;
 using System.IO;
@@ -62,20 +63,28 @@ public class DirectoryAssemblyResolver : IAssemblyResolver {
 
 		Dictionary<string, AssemblyDefinition> cache;
 		bool loadDebugSymbols;
-		Action<string, object[]> logWarnings;
+		Action<TraceLevel, string>              logger;
 
 		ReaderParameters                        loadReaderParameters;
 
 		static  readonly    ReaderParameters    DefaultLoadReaderParameters = new ReaderParameters {
 		};
 
+		[Obsolete ("Use DirectoryAssemblyResolver(Action<TraceLevel, string>, bool, ReaderParameters)")]
 		public DirectoryAssemblyResolver (Action<string, object[]> logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null)
+			: this ((TraceLevel level, string value) => logWarnings?.Invoke ("{0}", new[]{value}), loadDebugSymbols, loadReaderParameters)
 		{
 			if (logWarnings == null)
 				throw new ArgumentNullException (nameof (logWarnings));
+		}
+
+		public DirectoryAssemblyResolver (Action<TraceLevel, string> logger, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null)
+		{
+			if (logger == null)
+				throw new ArgumentNullException (nameof (logger));
 			cache = new Dictionary<string, AssemblyDefinition> ();
 			this.loadDebugSymbols = loadDebugSymbols;
-			this.logWarnings = logWarnings;
+			this.logger       = logger;
 			SearchDirectories = new List<string> ();
 			this.loadReaderParameters = loadReaderParameters ?? DefaultLoadReaderParameters;
 		}
@@ -141,10 +150,10 @@ protected virtual AssemblyDefinition ReadAssembly (string file)
 			try {
 				return AssemblyDefinition.ReadAssembly (file, reader_parameters);
 			} catch (Exception ex) {
-				logWarnings (
-						"Failed to read '{0}' with debugging symbols. Retrying to load it without it. Error details are logged below.",
-						new [] { file });
-				logWarnings ("{0}", new [] { ex.ToString () });
+				logger (
+						TraceLevel.Warning,
+						$"Failed to read '{file}' with debugging symbols. Retrying to load it without it. Error details are logged below.");
+				logger (TraceLevel.Warning, $"{ex.ToString ()}");
 				reader_parameters.ReadSymbols = false;
 				return AssemblyDefinition.ReadAssembly (file, reader_parameters);
 			}

src/Java.Interop.Tools.Diagnostics/Java.Interop.Tools.Diagnostics/Diagnostic.cs

@@ -2,6 +2,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 
 using Mono.Cecil.Cil;
 
@@ -27,14 +28,14 @@ namespace Java.Interop.Tools.Diagnostics {
 	//					XA0013	Profiling support requires sgen to be enabled too
 	//					XA0014	iOS {0} does not support building applications targeting ARMv6
 	//					XA0020	Could not launch mandroid daemon.
-	
+
 	//					XA0100 EmbeddedNativeLibrary '{0}' is invalid in Android Application project. Please use AndroidNativeLibrary instead.
 	//					XA0101 @(Content) build action is not supported
 	//					XA0102 Lint Warning
 	//					XA0103 Lint Error
 	//					XA0104 Invalid Sequence Point mode
 	//					XA0105 The TargetFrameworkVersion for {0} (v{1}) is greater than the TargetFrameworkVersion for your project (v{2}). You need to increase the TargetFrameworkVersion for your project.
-	
+
 	// XA1xxx	file copy / symlinks (project related)
 	//			XA10xx	installer.cs / mtouch.cs
 	//					XA1001	Could not find an application at the specified directory
@@ -101,6 +102,7 @@ namespace Java.Interop.Tools.Diagnostics {
 	//					XA4209 Failed to create JavaTypeInfo for class: {0} due to {1}
 	//					XA4210 "You need to add a reference to Mono.Android.Export.dll when you use ExportAttribute or ExportFieldAttribute."
 	//					XA4211  AndroidManifest.xml //uses-sdk/@android:targetSdkVersion '{0}' is less than $(TargetFrameworkVersion) '{1}'. Using API-{1} for ACW compilation.
+	//					XA4212  Type `{0}` implements `Android.Runtime.IJavaObject` but does not inherit `Java.Lang.Object` or `Java.Lang.Throwable`. This is not supported.
 	// XA5xxx	GCC and toolchain
 	//			XA32xx	.apk generation
 	//					XA4300  Unsupported $(AndroidSupportedAbis) value '{0}'; ignoring.
@@ -176,6 +178,17 @@ public static void WriteTo (System.IO.TextWriter destination, Exception message,
 			destination.WriteLine ("monodroid: error XA0000: Unexpected error - Please file a bug report at http://bugzilla.xamarin.com. Reason: {0}",
 					verbose ? message.Message : message.ToString ());
 		}
+
+		public static Action<TraceLevel, string> CreateConsoleLogger ()
+		{
+			Action<TraceLevel, string> logger = (level, value) => {
+				if (level == TraceLevel.Error)
+					Console.Error.WriteLine (value);
+				else
+					Console.WriteLine (value);
+			};
+			return logger;
+		}
 	}
 }
 

src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/JavaTypeScanner.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Text;
 using Mono.Cecil;
@@ -10,10 +11,19 @@
 
 namespace Java.Interop.Tools.JavaCallableWrappers
 {
-	public static class JavaTypeScanner
+	public class JavaTypeScanner
 	{
-		// Returns all types for which we need to generate Java delegate types.
-		public static List<TypeDefinition> GetJavaTypes (IEnumerable<string> assemblies, IAssemblyResolver resolver, Action<string, object[]> log)
+		public  Action<TraceLevel, string>      Logger                      { get; private set; }
+		public  bool                            ErrorOnCustomJavaObject     { get; set; }
+
+		public JavaTypeScanner (Action<TraceLevel, string> logger)
+		{
+			if (logger == null)
+				throw new ArgumentNullException (nameof (logger));
+			Logger      = logger;
+		}
+
+		public List<TypeDefinition> GetJavaTypes (IEnumerable<string> assemblies, IAssemblyResolver resolver)
 		{
 			var javaTypes = new List<TypeDefinition> ();
 
@@ -22,32 +32,34 @@ public static List<TypeDefinition> GetJavaTypes (IEnumerable<string> assemblies,
 
 				foreach (ModuleDefinition md in assm.Modules) {
 					foreach (TypeDefinition td in md.Types) {
-						AddJavaTypes (javaTypes, td, log);
+						AddJavaTypes (javaTypes, td);
 					}
 				}
 			}
 
 			return javaTypes;
 		}
 
-		static void AddJavaTypes (List<TypeDefinition> javaTypes, TypeDefinition type, Action<string, object[]> log)
+		void AddJavaTypes (List<TypeDefinition> javaTypes, TypeDefinition type)
 		{
 			if (type.IsSubclassOf ("Java.Lang.Object") || type.IsSubclassOf ("Java.Lang.Throwable")) {
 
 				// For subclasses of e.g. Android.App.Activity.
 				javaTypes.Add (type);
 			} else if (type.IsClass && !type.IsSubclassOf ("System.Exception") && type.ImplementsInterface ("Android.Runtime.IJavaObject")) {
-				log (
-						"Type '{0}' implements Android.Runtime.IJavaObject but does not inherit from Java.Lang.Object. It is not supported.",
-						new [] { type.FullName });
+				var level   = ErrorOnCustomJavaObject ? TraceLevel.Error : TraceLevel.Warning;
+				var prefix  = ErrorOnCustomJavaObject ? "error" : "warning";
+				Logger (
+						level,
+						$"{prefix} XA412: Type `{type.FullName}` implements `Android.Runtime.IJavaObject` but does not inherit `Java.Lang.Object` or `Java.Lang.Throwable`. This is not supported.");
 				return;
 			}
 
 			if (!type.HasNestedTypes)
 				return;
 
 			foreach (TypeDefinition nested in type.NestedTypes)
-				AddJavaTypes (javaTypes, nested, log);
+				AddJavaTypes (javaTypes, nested);
 		}
 
 		public static bool ShouldSkipJavaCallableWrapperGeneration (TypeDefinition type)
@@ -64,6 +76,11 @@ public static bool ShouldSkipJavaCallableWrapperGeneration (TypeDefinition type)
 
 			return false;
 		}
-
+		// Returns all types for which we need to generate Java delegate types.
+		public static List<TypeDefinition> GetJavaTypes (IEnumerable<string> assemblies, IAssemblyResolver resolver, Action<string, object []> log)
+		{
+			Action<TraceLevel, string> l = (level, value) => log ("{0}", new string [] { value });
+			return new JavaTypeScanner (l).GetJavaTypes (assemblies, resolver);
+		}
 	}
 }

src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.IO;
 using System.Collections.Generic;
 using System.Linq;
@@ -56,18 +57,27 @@ namespace Java.Interop.Tools.JavaCallableWrappers {
 	 */
 	public class TypeNameMapGenerator : IDisposable {
 
-		Action<string, object[]>        Log;
+		Action<TraceLevel, string>      Log;
 		List<TypeDefinition>            Types;
 		DirectoryAssemblyResolver       Resolver;
+		JavaTypeScanner                 Scanner;
 
+		[Obsolete ("Use TypeNameMapGenerator(IEnumerable<string>, Action<TraceLevel, string>)")]
 		public TypeNameMapGenerator (IEnumerable<string> assemblies, Action<string, object []> logMessage)
+			: this (assemblies, (TraceLevel level, string value) => logMessage?.Invoke ("{0}", new[]{value}))
 		{
-			if (assemblies == null)
-				throw new ArgumentNullException ("assemblies");
 			if (logMessage == null)
 				throw new ArgumentNullException (nameof (logMessage));
+		}
 
-			Log             = logMessage;
+		public TypeNameMapGenerator (IEnumerable<string> assemblies, Action<TraceLevel, string> logger)
+		{
+			if (assemblies == null)
+				throw new ArgumentNullException ("assemblies");
+			if (logger == null)
+				throw new ArgumentNullException (nameof (logger));
+
+			Log             = logger;
 			var Assemblies  = assemblies.ToList ();
 			var rp          = new ReaderParameters ();
 
@@ -83,17 +93,28 @@ public TypeNameMapGenerator (IEnumerable<string> assemblies, Action<string, obje
 				Resolver.Load (Path.GetFullPath (assembly));
 			}
 
-			Types       = JavaTypeScanner.GetJavaTypes (Assemblies, Resolver, logMessage);
+			Scanner     = new JavaTypeScanner (Log) {
+				ErrorOnCustomJavaObject     = false,
+			};
+			Types       = Scanner.GetJavaTypes (Assemblies, Resolver);
 		}
 
+		[Obsolete ("Use TypeNameMapGenerator(IEnumerable<TypeDefinition>, Action<TraceLevel, string>)")]
 		public TypeNameMapGenerator (IEnumerable<TypeDefinition> types, Action<string, object[]> logMessage)
+			: this (types, (TraceLevel level, string value) => logMessage?.Invoke ("{0}", new [] { value }))
 		{
-			if (types == null)
-				throw new ArgumentNullException ("types");
 			if (logMessage == null)
 				throw new ArgumentNullException (nameof (logMessage));
+		}
+
+		public TypeNameMapGenerator (IEnumerable<TypeDefinition> types, Action<TraceLevel, string> logger)
+		{
+			if (types == null)
+				throw new ArgumentNullException ("types");
+			if (logger == null)
+				throw new ArgumentNullException (nameof (logger));
 
-			Log         = logMessage;
+			Log         = logger;
 			Types       = types.ToList ();
 		}
 
@@ -112,11 +133,6 @@ protected virtual void Dispose (bool disposing)
 			Resolver = null;
 		}
 
-		void LogWarning (string format, params object[] args)
-		{
-			Log (format, args);
-		}
-
 		public void WriteJavaToManaged (Stream output)
 		{
 			if (output == null)
@@ -160,10 +176,10 @@ Dictionary<string, string> GetTypeMapping (Func<TypeDefinition, bool> skipType,
 				}
 			}
 			foreach (var e in aliases.OrderBy (e => e.Key)) {
-				LogWarning ("Mapping for type '{0}' is ambiguous between {1} types.", e.Key, e.Value.Count);
-				LogWarning ("     Using: {0}", e.Value.First ());
+				Log (TraceLevel.Warning, $"Mapping for type '{e.Key}' is ambiguous between {e.Value.Count} types.");
+				Log (TraceLevel.Warning, $"     Using: {e.Value.First ()}");
 				foreach (var o in e.Value.Skip (1)) {
-					LogWarning ("  Ignoring: {0}", o);
+					Log (TraceLevel.Info, $"  Ignoring: {o}");
 				}
 			}
 			return typeMap.ToDictionary (e => e.Key, e => value (e.Value));

src/Java.Interop.Tools.JavaCallableWrappers/Test/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGeneratorTests.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.IO;
 using System.Linq;
 using System.Text;
@@ -9,6 +10,7 @@
 using NUnit.Framework;
 
 using Java.Interop.Tools.Cecil;
+using Java.Interop.Tools.Diagnostics;
 using Java.Interop.Tools.JavaCallableWrappers;
 
 using Android.App;
@@ -24,16 +26,25 @@ public class TypeNameMapGeneratorTests
 		public void ConstructorExceptions ()
 		{
 			Action<string, object []> logger = (f, o) => { };
-			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((string [])null, logger));
-			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((TypeDefinition [])null, logger));
-			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new string [0], null));
-			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new TypeDefinition [0], null));
+			Action<string, object []> nullLogger = null;
+			Action<TraceLevel, string> levelLogger = (l, v) => { };
+			Action<TraceLevel, string> nullLevelLogger = null;
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((string []) null, levelLogger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((TypeDefinition []) null, levelLogger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new string [0], nullLevelLogger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new TypeDefinition [0], nullLevelLogger));
+#pragma warning disable 0618
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((string []) null, logger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator ((TypeDefinition []) null, logger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new string [0], nullLogger));
+			Assert.Throws<ArgumentNullException> (() => new TypeNameMapGenerator (new TypeDefinition [0], nullLogger));
+#pragma warning restore 0618
 		}
 
 		[Test]
 		public void WriteJavaToManaged ()
 		{
-			var v = new TypeNameMapGenerator (SupportDeclarations.GetTestTypeDefinitions (), logMessage: Console.WriteLine);
+			var v = new TypeNameMapGenerator (SupportDeclarations.GetTestTypeDefinitions (), logger: Diagnostic.CreateConsoleLogger ());
 			var o = new MemoryStream ();
 			v.WriteJavaToManaged (o);
 			var a = ToArray (o);
@@ -97,7 +108,7 @@ static string GetEntryPart (string value, int length)
 		[Test]
 		public void WriteManagedToJava ()
 		{
-			var v = new TypeNameMapGenerator (SupportDeclarations.GetTestTypeDefinitions (), logMessage: Console.WriteLine);
+			var v = new TypeNameMapGenerator (SupportDeclarations.GetTestTypeDefinitions (), logger: Diagnostic.CreateConsoleLogger ());
 			var o = new MemoryStream ();
 			v.WriteManagedToJava (o);
 			var a = ToArray (o);

src/Java.Interop/Documentation/Java.Interop/IJavaPeerable.xml

@@ -314,7 +314,7 @@ partial class ExampleBinding {
   </member>
   <member name="M:SetPeerReference">
     <summary>Set the value returned by <c>PeerReference</c>.</summary>
-    <param name="value">
+    <param name="reference">
       A <see cref="T:Java.Interop.JniObjectReference" /> which contains the
       value that future invocations of the
       <see cref="P:Java.Interop.IJavaPeerable.PeerReference" />

tools/jcw-gen/App.cs

@@ -1,9 +1,11 @@
 using System;
+using System.Diagnostics;
 using System.Linq;
 using System.IO;
 using System.Threading.Tasks;
 
 using Java.Interop.Tools.Cecil;
+using Java.Interop.Tools.Diagnostics;
 using Java.Interop.Tools.JavaCallableWrappers;
 using Mono.Cecil;
 using Mono.Options;
@@ -41,6 +43,7 @@ public static int Main (string [] args)
 				  "Show this message and exit",
 				  v => help = v != null },
 			};
+			var scanner = new JavaTypeScanner (Diagnostic.CreateConsoleLogger ());
 			try {
 				var assemblies = options.Parse (args);
 				if (assemblies.Count == 0 || outputPath == null || help) {
@@ -60,7 +63,7 @@ public static int Main (string [] args)
 					resolver.SearchDirectories.Add (Path.GetDirectoryName (assembly));
 					resolver.Load (assembly);
 				}
-				var types = JavaTypeScanner.GetJavaTypes (assemblies, resolver, log: Console.WriteLine)
+				var types = scanner.GetJavaTypes (assemblies, resolver)
 					.Where (td => !JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (td));
 				foreach (var type in types) {
 					GenerateJavaCallableWrapper (type, outputPath);