Skip to content

Commit

Permalink
[Java.Interop.Tools.JavaCallableWrappers] Error on bad IJavaObject (#179
Browse files Browse the repository at this point in the history 
)

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.
  • Loading branch information
@jonpryor @atsushieno
jonpryor authored and atsushieno committed Aug 30, 2017
1 parent bac28ab commit ab3c2b2
Show file tree
Hide file tree
Showing 7 changed files with 111 additions and 42 deletions.
21 changes: 15 additions & 6 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
//

using System;
using System.Diagnostics;
using System.Collections;
using System.Collections.Generic;
using System.IO;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand Down
17 changes: 15 additions & 2 deletions src/Java.Interop.Tools.Diagnostics/Java.Interop.Tools.Diagnostics/Diagnostic.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;

using Mono.Cecil.Cil;

Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
}
}

37 changes: 27 additions & 10 deletions ...rop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/JavaTypeScanner.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using Mono.Cecil;
Expand All @@ -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> ();

Expand All @@ -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)
Expand All @@ -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);
}
}
}
48 changes: 32 additions & 16 deletions ...ools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;
using System.IO;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -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 ();

Expand All @@ -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 ();
}

Expand All @@ -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)
Expand Down Expand Up @@ -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));
Expand Down
23 changes: 17 additions & 6 deletions ...allableWrappers/Test/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGeneratorTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/Java.Interop/Documentation/Java.Interop/IJavaPeerable.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down
5 changes: 4 additions & 1 deletion tools/jcw-gen/App.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand Down

0 comments on commit ab3c2b2

Please sign in to comment.