[generator] BG8403 when type name matches enclosing namespace name (#1010)

jpobst · web-flow · commit 45fe39286932 · 2022-07-14T15:42:54.000-04:00
Fixes: #982 As elaborated upon in [Eric Lippert's blog][0] series "Do not name a class the same as its namespace" ([Part 1][1], [Part 2][2], [Part 3][3], and [Part 4][4]), and as explained in the [Framework Design Guidelines][5]: > ❌ DO NOT use the same name for a namespace and a type in that namespace. > > For example, do not use `Debug` as a namespace name and then also > provide a class named `Debug` in the same namespace. > Several compilers require such types to be fully qualified. emit a BG8403 warning when a type is generated which matches the enclosing namespace "part": namespace Android.Graphics.Drawable { partial class Drawable { } } Such constructs will now emit the warning: warning BG8403: Type `Android.Graphics.Drawable.Drawable` has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information. This construct not a warning in Roslyn, and bindings code is autogenerated, so it can be hard for bindings authors to notice that they are making this mistake. <https://aka.ms/BG8403> explains the issue and possible solutions, which generally includes using [`@(AndroidNamespaceReplacement)`][6] to rename the namespace (e.g. to `Android.Graphics.Drawables`) or using `Metadata.xml` to rename the type. ~~ Testing Note ~~ It is very hard to unit test `generator` warning/error reporting, since the `Report` type is a global static class. Add a static property `Report.OutputDelegate` that allows the tester to temporarily set a different output method that tests can use to verify `Report` output. These tests should be marked `[NonParallelizable]` since they affect the global state shared with any other running tests. [0]: https://docs.microsoft.com/en-us/archive/blogs/ericlippert [1]: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/do-not-name-a-class-the-same-as-its-namespace-part-one [2]: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/do-not-name-a-class-the-same-as-its-namespace-part-two [3]: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/do-not-name-a-class-the-same-as-its-namespace-part-three [4]: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/do-not-name-a-class-the-same-as-its-namespace-part-four [5]: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-namespaces [6]: dotnet/android@398afd4
diff --git a/src/Java.Interop.Localization/Resources.Designer.cs b/src/Java.Interop.Localization/Resources.Designer.cs
diff --git a/src/Java.Interop.Localization/Resources.resx b/src/Java.Interop.Localization/Resources.resx
@@ -201,6 +201,10 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</comment>
   </data>
+  <data name="Generator_BG8403" xml:space="preserve">
+    <value>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</value>
+    <comment>{0} - Java type.</comment>
+  </data>
   <data name="Generator_BG8500" xml:space="preserve">
     <value>Unexpected child element of '&lt;interface&gt;': '{0}'.</value>
     <comment>{0} - XML element name.
diff --git a/src/Java.Interop.Localization/xlf/Resources.cs.xlf b/src/Java.Interop.Localization/xlf/Resources.cs.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.de.xlf b/src/Java.Interop.Localization/xlf/Resources.de.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.es.xlf b/src/Java.Interop.Localization/xlf/Resources.es.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.fr.xlf b/src/Java.Interop.Localization/xlf/Resources.fr.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.it.xlf b/src/Java.Interop.Localization/xlf/Resources.it.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.ja.xlf b/src/Java.Interop.Localization/xlf/Resources.ja.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.ko.xlf b/src/Java.Interop.Localization/xlf/Resources.ko.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.pl.xlf b/src/Java.Interop.Localization/xlf/Resources.pl.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.pt-BR.xlf b/src/Java.Interop.Localization/xlf/Resources.pt-BR.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.ru.xlf b/src/Java.Interop.Localization/xlf/Resources.ru.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.tr.xlf b/src/Java.Interop.Localization/xlf/Resources.tr.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.zh-Hans.xlf b/src/Java.Interop.Localization/xlf/Resources.zh-Hans.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Localization/xlf/Resources.zh-Hant.xlf b/src/Java.Interop.Localization/xlf/Resources.zh-Hant.xlf
@@ -104,6 +104,11 @@ In this message, the term "constants" refers to class or interface members that
 {1} - .NET field name.
 {2} - Java type.</note>
       </trans-unit>
+      <trans-unit id="Generator_BG8403">
+        <source>Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</source>
+        <target state="new">Type '{0}' has a type name which matches the enclosing namespace name. See https://aka.ms/BG8403 for more information.</target>
+        <note>{0} - Java type.</note>
+      </trans-unit>
       <trans-unit id="Generator_BG8500">
         <source>Unexpected child element of '&lt;interface&gt;': '{0}'.</source>
         <target state="new">Unexpected child element of '&lt;interface&gt;': '{0}'.</target>
diff --git a/src/Java.Interop.Tools.Generator/Utilities/Report.cs b/src/Java.Interop.Tools.Generator/Utilities/Report.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.Xml;
 using System.Xml.Linq;
 
@@ -7,6 +8,7 @@ namespace Java.Interop.Tools.Generator
 	public class Report
 	{
 		public static int? Verbosity { get; set; }
+		public static Action<TraceLevel, string>? OutputDelegate { get; set; }
 
 		public class LocalizedMessage
 		{
@@ -43,6 +45,7 @@ public LocalizedMessage (int code, string value)
 		public static LocalizedMessage WarningFieldNameCollision_Method => new LocalizedMessage (0x8401, Localization.Resources.Generator_BG8401_Method);
 		public static LocalizedMessage WarningFieldNameCollision_NestedType => new LocalizedMessage (0x8401, Localization.Resources.Generator_BG8401_NestedType);
 		public static LocalizedMessage WarningDuplicateField => new LocalizedMessage (0x8402, Localization.Resources.Generator_BG8402);
+		public static LocalizedMessage WarningTypeNameMatchesEnclosingNamespace => new LocalizedMessage (0x8403, Localization.Resources.Generator_BG8403);
 		public static LocalizedMessage WarningUnexpectedInterfaceChild => new LocalizedMessage (0x8500, Localization.Resources.Generator_BG8500);
 		public static LocalizedMessage WarningEmptyEventName => new LocalizedMessage (0x8501, Localization.Resources.Generator_BG8501);
 		public static LocalizedMessage WarningInvalidDueToInterfaces => new LocalizedMessage (0x8502, Localization.Resources.Generator_BG8502);
@@ -95,7 +98,7 @@ public static void LogCodedError (LocalizedMessage message, XNode? node, params
 
 		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));
+			WriteOutput (TraceLevel.Error, Format (true, message.Code, sourceFile, line, column, message.Value, args));
 		}
 
 		public static void LogCodedWarning (int verbosity, LocalizedMessage message, params string? [] args)
@@ -120,17 +123,17 @@ public static void LogCodedWarning (int verbosity, LocalizedMessage message, Exc
 				return;
 
 			var supp = innerException != null ? "  For details, see verbose output." : null;
-			Console.Error.WriteLine (Format (false, message.Code, sourceFile, line, column, message.Value, args) + supp);
+			WriteOutput (TraceLevel.Warning, Format (false, message.Code, sourceFile, line, column, message.Value, args) + supp);
 
 			if (innerException != null)
-				Console.Error.WriteLine (innerException);
+				WriteOutput (TraceLevel.Warning, innerException.ToString ());
 		}
 		
 		public static void Verbose (int verbosity, string format, params object?[] args)
 		{
 			if (verbosity > (Verbosity ?? 0))
 				return;
-			Console.Error.WriteLine (format, args);
+			WriteOutput (TraceLevel.Verbose, format, args);
 		}
 
 		public static string FormatCodedMessage (bool error, LocalizedMessage message, params object? [] args)
@@ -169,6 +172,18 @@ public static string Format (bool error, int errorCode, string? sourceFile, int
 
 			return (file, pos?.LineNumber ?? -1, pos?.LinePosition ?? -1);
 		}
+
+		static void WriteOutput (TraceLevel traceLevel, string format, params object?[] args)
+		{
+			// Write to overridden output if requested
+			if (OutputDelegate != null) {
+				OutputDelegate (traceLevel, string.Format (format, args));
+				return;
+			}
+
+			// Write to Console.Error
+			Console.Error.WriteLine (format, args);
+		}
 	}
 
 	/// <summary>
diff --git a/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs b/tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
@@ -1,7 +1,10 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
+using System.Text;
 using generator.SourceWriters;
+using Java.Interop.Tools.Generator;
 using MonoDroid.Generation;
 using NUnit.Framework;
 using Xamarin.Android.Binder;
@@ -527,6 +530,47 @@ public void ObsoleteBoundMethodAbstractDeclaration ()
 			// Ensure [Obsolete] was written
 			Assert.True (writer.ToString ().Contains ("[Obsolete (@\"This is so old!\")]"), writer.ToString ());
 		}
+
+		[Test]
+		[NonParallelizable]	// We are setting a static property on Report
+		public void WarnIfTypeNameMatchesNamespace ()
+		{
+			var @class = new TestClass ("Object", "java.myclass.MyClass");
+			var sb = new StringBuilder ();
+
+			var write_output = new Action<TraceLevel, string> ((t, s) => { sb.AppendLine (s); });
+			Report.OutputDelegate = write_output;
+
+			generator.Context.ContextTypes.Push (@class);
+			generator.WriteType (@class, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
+			generator.Context.ContextTypes.Pop ();
+
+			Report.OutputDelegate = null;
+
+			// Ensure the warning was raised
+			Assert.True (sb.ToString ().Contains ("warning BG8403"));
+		}
+
+		[Test]
+		[NonParallelizable]     // We are setting a static property on Report
+		public void DontWarnIfNestedTypeNameMatchesNamespace ()
+		{
+			var @class = new TestClass ("Object", "java.myclass.MyParentClass");
+			@class.NestedTypes.Add (new TestClass ("Object", "java.myclass.MyParentClass.MyClass"));
+			var sb = new StringBuilder ();
+
+			var write_output = new Action<TraceLevel, string> ((t, s) => { sb.AppendLine (s); });
+			Report.OutputDelegate = write_output;
+
+			generator.Context.ContextTypes.Push (@class);
+			generator.WriteType (@class, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
+			generator.Context.ContextTypes.Pop ();
+
+			Report.OutputDelegate = null;
+
+			// The warning should not be raised if the nested type matches enclosing namespace
+			Assert.False (sb.ToString ().Contains ("warning BG8403"));
+		}
 	}
 
 	[TestFixture]
diff --git a/tools/generator/Extensions/StringExtensions.cs b/tools/generator/Extensions/StringExtensions.cs
@@ -0,0 +1,45 @@
+using System;
+using System.Diagnostics.CodeAnalysis;
+
+namespace generator
+{
+	static class StringExtensions
+	{
+		/// <summary>
+		/// Shortcut for !string.IsNullOrWhiteSpace (s)
+		/// </summary>
+		public static bool HasValue (this string s) => !string.IsNullOrWhiteSpace (s);
+
+		/// <summary>
+		/// Removes the final subset of a delimited string. ("127.0.0.1" -> "127.0.0")
+		/// </summary>
+		public static string ChompLast (this string s, char separator)
+		{
+			if (!s.HasValue ())
+				return s;
+
+			var index = s.LastIndexOf (separator);
+
+			if (index < 0)
+				return string.Empty;
+
+			return s.Substring (0, index);
+		}
+
+		/// <summary>
+		/// Returns the final subset of a delimited string. ("127.0.0.1" -> "1")
+		/// </summary>
+		public static string LastSubset (this string s, char separator)
+		{
+			if (!s.HasValue ())
+				return s;
+
+			var index = s.LastIndexOf (separator);
+
+			if (index < 0)
+				return s;
+
+			return s.Substring (index + 1);
+		}
+	}
+}
diff --git a/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/JavaInteropCodeGenerator.cs b/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/JavaInteropCodeGenerator.cs
@@ -44,6 +44,10 @@ public override void WriteType (GenBase gen, string indent, GenerationInfo gen_i
 			else
 				throw new InvalidOperationException ("Unknown GenBase type");
 
+			// We do this here because we only want to check for top-level types,
+			// we should not check types nested in other types.
+			SourceWriterExtensions.WarnIfTypeNameMatchesNamespace (type_writer, gen);
+
 			var cw = new CodeWriter (writer, indent);
 			type_writer.Write (cw);
 		}
diff --git a/tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs b/tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs
@@ -397,5 +397,18 @@ public static TypeWriter BuildManagedTypeModel (GenBase gen, CodeGenerationOptio
 
 			throw new InvalidOperationException ("Unknown GenBase type");
 		}
+
+		public static void WarnIfTypeNameMatchesNamespace (TypeWriter type, GenBase gen)
+		{
+			// We only care about the last part of a namespace
+			// eg: android.graphics.drawable -> drawable
+			var ns = gen.Namespace?.LastSubset ('.');
+
+			if (!ns.HasValue ())
+				return;
+
+			if (string.Equals (ns, type.Name, StringComparison.OrdinalIgnoreCase))
+				Report.LogCodedWarning (0, Report.WarningTypeNameMatchesEnclosingNamespace, $"{gen.Namespace}.{type.Name}");
+		}
 	}
 }
