[generator] Gracefully handle BindingGeneratorException. (#845)

@moljac pointed out that if you have invalid XPath in your metadata,
we simply error with the unhelpful:

	"generator.exe" exited with code -XXXXXX

![before](https://user-images.githubusercontent.com/179295/119032575-f4891680-b971-11eb-9580-96cc2e96e4aa.png)

If you turn on `Diagnostic` logs, you can see the uncaught exception
hiding in a lot of ceremony:

	message BG0000: Unhandled Exception: Java.Interop.Tools.Generator.BindingGeneratorException:
	  C:\code\temp\xamarin.exoplayer\Xamarin.ExoPlayer.Core\Transforms\Metadata.xml(11, 4):
	  error BG4304: Invalid XPath specification: /api/package[@name='com.google.android.exoplayer2']interface[@name='ExoPlayer']/method[@name='addMediaItems' and count(parameter)=2 and parameter[1][@type='int'] and parameter[2][@type='java.util.List<com.google.android.exoplayer2.source.MediaSource>']]/parameter[2].
	  ---> System.Xml.XPath.XPathException: '/api/package[@name='com.google.android.exoplayer2']interface[@name='ExoPlayer']/method[@name='addMediaItems' and count(parameter)=2 and parameter[1][@type='int'] and parameter[2][@type='java.util.List<com.google.android.exoplayer2.source.MediaSource>']]/parameter[2]' has an invalid token.
	message BG0000:    at MS.Internal.Xml.XPath.XPathParser.ParseXPathExpresion(String xpathExpresion)
	message BG0000:    at System.Xml.XPath.XPathExpression.Compile(String xpath, IXmlNamespaceResolver nsResolver)
	message BG0000:    at System.Xml.XPath.XPathNavigator.Evaluate(String xpath, IXmlNamespaceResolver resolver)
	message BG0000:    at System.Xml.XPath.XPathEvaluator.Evaluate[T](XNode node, String expression, IXmlNamespaceResolver resolver)
	message BG0000:    at System.Xml.XPath.Extensions.XPathSelectElements(XNode node, String expression, IXmlNamespaceResolver resolver)
	message BG0000:    at Java.Interop.Tools.Generator.FixupXmlDocument.Apply(ApiXmlDocument apiDocument, String apiLevelString, Int32 productVersion)
	message BG0000:    --- End of inner exception stack trace --- (TaskId:46)
	message BG0000:    at Java.Interop.Tools.Generator.Report.LogCodedError(LocalizedMessage message, Exception innerException, String sourceFile, Int32 line, Int32 column, String[] args)
	message BG0000:    at Java.Interop.Tools.Generator.Report.LogCodedError(LocalizedMessage message, Exception innerException, XNode node, String[] args)
	message BG0000:    at Java.Interop.Tools.Generator.FixupXmlDocument.Apply(ApiXmlDocument apiDocument, String apiLevelString, Int32 productVersion)
	message BG0000:    at Java.Interop.Tools.Generator.ApiXmlDocument.ApplyFixupFile(FixupXmlDocument fixup)
	message BG0000:    at Xamarin.Android.Binder.CodeGenerator.Run(CodeGeneratorOptions options, DirectoryAssemblyResolver resolver)
	message BG0000:    at Xamarin.Android.Binder.CodeGenerator.Run(CodeGeneratorOptions options)
	message BG0000:    at Xamarin.Android.Binder.CodeGenerator.Main(String[] args)
	error MSB6006: "generator.exe" exited with code -532462766.


It turns out this happens anywhere we throw a
`BindingGeneratorException`, as there is no `catch` handler that
attempts to gracefully handles this exception.

This led to making several improvements:

  - Gracefully handle `BindingGeneratorException` such that we print
    out the error as a standard MSBuild error that can be processed
    by Visual Studio.

  - Exit `generator` with `-1` if `BindingGeneratorException` is thrown.

  - Add a separate `Report.LogCodedError(…)` method called
    `Report.LogCodeErrorAndExit(…)`, which is used for terminal errors
    ("don't do any further processing and exit").

  - Audit existing calls to `Report.LogCodedError()` to allow errors
    such as `Invalid XPath` to be non-terminal.
    (Today `generator` will exit after finding the first one.)

  - Remove spaces in the error file location: `metadata.xml(8, 4): `
    becomes `metadata.xml(8,4):`.
    
    These were not being processed correctly and could not be double
    clicked in VS previously.

The result is a more useful error message:

	Metadata.xml(11,4): error BG4304: Invalid XPath specification …

We also now handle any unhandled exception a little better.

Additionally, `ApiFixup.cs` was deleted as it was no longer used after
the refactor in commit f4e68b5.

src/Java.Interop.Tools.Generator/Metadata/FixupXmlDocument.cs

@@ -58,9 +58,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 						else
 							// BG8A00
 							Report.LogCodedWarning (0, Report.WarningRemoveNodeMatchedNoNodes, null, metaitem, $"<remove-node path=\"{path}\" />");
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4301
-						Report.LogCodedError (Report.ErrorRemoveNodeInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorRemoveNodeInvalidXPath, metaitem, path);
 					}
 					break;
 				case "add-node":
@@ -74,9 +74,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 							foreach (var node in nodes)
 								node.Add (metaitem.Nodes ());
 						}
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4302
-						Report.LogCodedError (Report.ErrorAddNodeInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorAddNodeInvalidXPath, metaitem, path);
 					}
 					break;
 				case "change-node":
@@ -95,9 +95,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 						if (!matched)
 							// BG8A03
 							Report.LogCodedWarning (0, Report.WarningChangeNodeTypeMatchedNoNodes, null, metaitem, $"<change-node-type path=\"{path}\" />");
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4303
-						Report.LogCodedError (Report.ErrorChangeNodeInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorChangeNodeInvalidXPath, metaitem, path);
 					}
 					break;
 				case "attr":
@@ -106,7 +106,7 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 
 						if (string.IsNullOrEmpty (attr_name))
 							// BG4307
-							Report.LogCodedError (Report.ErrorMissingAttrName, null, metaitem, path);
+							Report.LogCodedError (Report.ErrorMissingAttrName, metaitem, path);
 						var nodes = attr_last_cache != null ? new XElement [] { attr_last_cache } : apiDocument.ApiDocument.XPathSelectElements (path);
 						var attr_matched = 0;
 
@@ -119,9 +119,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 							Report.LogCodedWarning (0, Report.WarningAttrMatchedNoNodes, null, metaitem, $"<attr path=\"{path}\" />");
 						if (attr_matched != 1)
 							attr_last_cache = null;
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4304
-						Report.LogCodedError (Report.ErrorAttrInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorAttrInvalidXPath, metaitem, path);
 					}
 					break;
 				case "move-node":
@@ -140,9 +140,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 						if (!matched)
 							// BG8A05
 							Report.LogCodedWarning (0, Report.WarningMoveNodeMatchedNoNodes, null, metaitem, $"<move-node path=\"{path}\" />");
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4305
-						Report.LogCodedError (Report.ErrorMoveNodeInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorMoveNodeInvalidXPath, metaitem, path);
 					}
 					break;
 				case "remove-attr":
@@ -159,9 +159,9 @@ public void Apply (ApiXmlDocument apiDocument, string apiLevelString, int produc
 						if (!matched)
 							// BG8A06
 							Report.LogCodedWarning (0, Report.WarningRemoveAttrMatchedNoNodes, null, metaitem, $"<remove-attr path=\"{path}\" />");
-					} catch (XPathException e) {
+					} catch (XPathException) {
 						// BG4306
-						Report.LogCodedError (Report.ErrorRemoveAttrInvalidXPath, e, metaitem, path);
+						Report.LogCodedError (Report.ErrorRemoveAttrInvalidXPath, metaitem, path);
 					}
 					break;
 				}

src/Java.Interop.Tools.Generator/Utilities/ApiXmlDocument.cs

@@ -40,7 +40,7 @@ public void ApplyFixupFile (FixupXmlDocument fixup)
 				fixup.Apply (this, ApiLevel, ProductVersion);
 			} catch (XmlException ex) {
 				// BG4200
-				Report.LogCodedError (Report.ErrorFailedToProcessMetadata, ex.Message);
+				Report.LogCodedErrorAndExit (Report.ErrorFailedToProcessMetadata, null, fixup.FixupDocument, ex.Message);
 			}
 		}
 	}

src/Java.Interop.Tools.Generator/Utilities/Report.cs

@@ -69,23 +69,30 @@ public LocalizedMessage (int code, string value)
 		public static LocalizedMessage WarningBaseInterfaceNotFound => new LocalizedMessage (0x8C00, Localization.Resources.Generator_BG8C00);
 		public static LocalizedMessage WarningBaseInterfaceInvalid => new LocalizedMessage (0x8C01, Localization.Resources.Generator_BG8C01);
 
-		public static void LogCodedError (LocalizedMessage message, params string [] args)
-			=> LogCodedError (message, null, null, -1, -1, args);
+		public static void LogCodedErrorAndExit (LocalizedMessage message, params string [] args)
+			=> LogCodedErrorAndExit (message, null, null, args);
 
-		public static void LogCodedError (LocalizedMessage message, Exception? innerException, params string [] args)
-			=> LogCodedError (message, innerException, null, -1, -1, args);
+		public static void LogCodedErrorAndExit (LocalizedMessage message, Exception? innerException, params string [] args)
+			=> LogCodedErrorAndExit (message, innerException, null, args);
 
-		public static void LogCodedError (LocalizedMessage message, Exception? innerException, XNode node, params string? [] args)
+		public static void LogCodedErrorAndExit (LocalizedMessage message, Exception? innerException, XNode? node, params string? [] args)
 		{
-			var file = Uri.TryCreate (node.BaseUri, UriKind.Absolute, out var uri) ? uri.LocalPath : null;
-			var line_info = (node as IXmlLineInfo)?.HasLineInfo () == true ? node as IXmlLineInfo : null;
+			LogCodedError (message, node, args);
 
-			LogCodedError (message, innerException, file, line_info?.LineNumber ?? -1, line_info?.LinePosition ?? -1, args);
+			// Throwing a BindingGeneratorException will cause generator to terminate
+			throw new BindingGeneratorException (message.Code, string.Format (message.Value, args), innerException);
 		}
 
-		public static void LogCodedError (LocalizedMessage message, Exception? innerException, string? sourceFile, int line, int column, params string? [] args)
+		public static void LogCodedError (LocalizedMessage message, XNode? node, params string? [] args)
 		{
-			throw new BindingGeneratorException (message.Code, sourceFile, line, column, string.Format (message.Value, args), innerException);
+			var (file, line, col) = GetLineInfo (node);
+
+			LogCodedError (message, file, line, col, args);
+		}
+
+		public static void LogCodedError (LocalizedMessage message, string? sourceFile, int line, int column, params string? [] args)
+		{
+			Console.Error.WriteLine (Format (true, message.Code, sourceFile, line, column, message.Value, args));
 		}
 
 		public static void LogCodedWarning (int verbosity, LocalizedMessage message, params string? [] args)
@@ -99,10 +106,9 @@ public static void LogCodedWarning (int verbosity, LocalizedMessage message, Exc
 
 		public static void LogCodedWarning (int verbosity, LocalizedMessage message, Exception? innerException, XNode node, params string? [] args)
 		{
-			var file = Uri.TryCreate (node.BaseUri, UriKind.Absolute, out var uri) ? uri.LocalPath : null;
-			var line_info = (node as IXmlLineInfo)?.HasLineInfo () == true ? node as IXmlLineInfo : null;
+			var (file, line, col) = GetLineInfo (node);
 
-			LogCodedWarning (verbosity, message, innerException, file, line_info?.LineNumber ?? -1, line_info?.LinePosition ?? -1, args);
+			LogCodedWarning (verbosity, message, innerException, file, line, col, args);
 		}
 
 		public static void LogCodedWarning (int verbosity, LocalizedMessage message, Exception? innerException, string? sourceFile, int line, int column, params string? [] args)
@@ -145,12 +151,26 @@ public static string Format (bool error, int errorCode, string? sourceFile, int
 				return ret + ": ";
 
 			if (column > 0)
-				return ret + $"({line}, {column}): ";
+				return ret + $"({line},{column}): ";
 
 			return ret + $"({line}): ";
 		}
+
+		static (string? file, int line, int col) GetLineInfo (XNode? node)
+		{
+			if (node is null)
+				return (null, -1, -1);
+
+			var file = Uri.TryCreate (node.BaseUri, UriKind.Absolute, out var uri) ? uri.LocalPath : null;
+			var pos = (node as IXmlLineInfo)?.HasLineInfo () == true ? node as IXmlLineInfo : null;
+
+			return (file, pos?.LineNumber ?? -1, pos?.LinePosition ?? -1);
+		}
 	}
-
+
+	/// <summary>
+	/// Throwing this exception will cause generator to exit gracefully.
+	/// </summary>
 	public class BindingGeneratorException : Exception
 	{
 		public BindingGeneratorException (int errorCode, string message)

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

@@ -24,8 +24,8 @@ public void FormatTests ()
 			Assert.AreEqual ("error BG0037: There was a bad error", Report.Format (true, code, null, 0, 0, msg, args));
 			Assert.AreEqual (@"C:\code\test.cs: error BG0037: There was a bad error", Report.Format (true, code, sourcefile, 0, 0, msg, args));
 			Assert.AreEqual (@"C:\code\test.cs(32): error BG0037: There was a bad error", Report.Format (true, code, sourcefile, line, 0, msg, args));
-			Assert.AreEqual (@"C:\code\test.cs(32, 12): error BG0037: There was a bad error", Report.Format (true, code, sourcefile, line, col, msg, args));
-			Assert.AreEqual (@"C:\code\test.cs(32, 12): warning BG0037: There was a bad error", Report.Format (false, code, sourcefile, line, col, msg, args));
+			Assert.AreEqual (@"C:\code\test.cs(32,12): error BG0037: There was a bad error", Report.Format (true, code, sourcefile, line, col, msg, args));
+			Assert.AreEqual (@"C:\code\test.cs(32,12): warning BG0037: There was a bad error", Report.Format (false, code, sourcefile, line, col, msg, args));
 		}
 	}
 }

tools/generator/CodeGenerationOptions.cs

@@ -222,7 +222,7 @@ public string GetOutputName (string type)
 			if (type.StartsWith ("params "))
 				return "params " + GetOutputName (type.Substring ("params ".Length));
 			if (type.StartsWith ("global::"))
-				Report.LogCodedError (Report.ErrorUnexpectedGlobal);
+				Report.LogCodedErrorAndExit (Report.ErrorUnexpectedGlobal);
 			if (!UseGlobal)
 				return type;
 

tools/generator/CodeGenerator.cs

@@ -22,11 +22,19 @@ public class CodeGenerator
 	{
 		public static int Main (string[] args)
 		{
-			var options = CodeGeneratorOptions.Parse (args);
-			if (options == null)
+			try {
+				var options = CodeGeneratorOptions.Parse (args);
+				if (options == null)
+					return 1;
+
+				Run (options);
+			} catch (BindingGeneratorException) {
 				return 1;
+			} catch (Exception ex) {
+				Console.Error.WriteLine (Report.Format (true, 0, null, -1, -1, ex.ToString ()));
+				return 1;
+			}
 
-			Run (options);
 			return 0;
 		}