[generator] Ensure consistent member visibility. [#367]

tools/generator/CodeGenerator.cs

@@ -160,6 +160,9 @@ static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolve
 			foreach (GenBase gen in gens)
 				gen.FixupExplicitImplementation ();
 
+			foreach (GenBase gen in gens)
+				MethodVisibilityFixup.Fixup (gen);
+
 			GenerateAnnotationAttributes (gens, annotations_zips);
 
 			//SymbolTable.Dump ();

tools/generator/Java.Interop.Tools.Generator.ObjectModel/GenBase.cs

@@ -946,9 +946,9 @@ public static string GetSignature (MethodBase method, CodeGenerationOptions opt)
 			return sb.ToString ();
 		}
 
-		static IEnumerable<Method> GetAllMethods (GenBase t)
+		public IEnumerable<Method> GetAllMethods ()
 		{
-			return t.Methods.Concat (t.Properties.Select (p => p.Getter)).Concat (t.Properties.Select (p => p.Setter).Where (m => m != null));
+			return Methods.Concat (Properties.Select (p => p.Getter)).Concat (Properties.Select (p => p.Setter).Where (m => m != null));
 		}
 
 		static string [] AutoDetectEnumifiedOverrideParameters (MethodBase method, AncestorDescendantCache cache)
@@ -958,7 +958,7 @@ static string [] AutoDetectEnumifiedOverrideParameters (MethodBase method, Ances
 			var classes = cache.GetAncestorsAndDescendants (method.DeclaringType);
 			classes = classes.Concat (classes.SelectMany(x => x.GetAllImplementedInterfaces ()));
 			foreach (var t in classes) {
-				foreach (var candidate in GetAllMethods (t).Where (m => m.Name == method.Name
+				foreach (var candidate in t.GetAllMethods ().Where (m => m.Name == method.Name
 					&& m.Parameters.Count == method.Parameters.Count
 					&& m.Parameters.Any (p => p.IsEnumified))) {
 					var ret = new string [method.Parameters.Count];
@@ -989,7 +989,7 @@ static string AutoDetectEnumifiedOverrideReturn (Method method, AncestorDescenda
 			var classes = cache.GetAncestorsAndDescendants (method.DeclaringType);
 			classes = classes.Concat (classes.SelectMany(x => x.GetAllImplementedInterfaces ()));
 			foreach (var t in classes) {
-				foreach (var candidate in GetAllMethods (t).Where (m => m.Name == method.Name && m.Parameters.Count == method.Parameters.Count)) {
+				foreach (var candidate in t.GetAllMethods ().Where (m => m.Name == method.Name && m.Parameters.Count == method.Parameters.Count)) {
 					if (method.JniSignature != candidate.JniSignature)
 						continue;
 					if (candidate.IsReturnEnumified)

tools/generator/Java.Interop.Tools.Generator.ObjectModel/Method.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Linq;
 
 namespace MonoDroid.Generation
 {
@@ -245,6 +246,47 @@ internal string GetAdapterName (CodeGenerationOptions opt, string adapter)
 				return adapter + ", " + opt.AssemblyName;
 			return adapter + AssemblyName;
 		}
+
+		// This gets the Method's most direct ancestor
+		public Method GetBaseMethod ()
+		{
+			if (!is_override)
+				return null;
+
+			var base_class = DeclaringType?.BaseGen;
+
+			if (base_class == null)
+				return null;
+
+			while (base_class != null) {
+				var method = base_class.GetAllMethods ()
+					.FirstOrDefault (m => m.Name == Name
+						&& ParameterList.Equals (m.Parameters, Parameters)
+						&& m.ReturnType == ReturnType);
+
+				if (method != null)
+					return method;
+
+				base_class = base_class.BaseGen;
+			}
+
+			return null;
+		}
+
+		// This gets the instance where the Method is originally declared (ie: it may be several ancestors up)
+		public Method GetBaseMethodDeclaration ()
+		{
+			var candidate = GetBaseMethod ();
+			var last_candidate = (Method) null;
+
+			while (true) {
+				if (candidate is null)
+					return last_candidate;
+
+				last_candidate = candidate;
+				candidate = last_candidate.GetBaseMethod ();
+			}
+		}
 	}
 }
 

tools/generator/Java.Interop.Tools.Generator.Transformation/MethodVisibilityFixup.cs

@@ -0,0 +1,28 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace MonoDroid.Generation
+{
+	// Java allows a public member to override a protected member, but C# does not.
+	// This Fixup makes the override member protected as well.
+	static class MethodVisibilityFixup
+	{
+		public static void Fixup (GenBase gen)
+		{
+			foreach (var method in gen.GetAllMethods ()) {
+				var base_method = method.GetBaseMethodDeclaration ();
+
+				if (base_method == null)
+					continue;
+
+				if (method.Visibility == "public" && (base_method.Visibility == "protected" || base_method.Visibility == "protected internal")) {
+					Report.Warning (0, Report.WarningInconsistentAccessbility, $"Setting {gen.FullName}.{method.Name} visibility to {base_method.Visibility} to match base method");
+					method.Visibility = base_method.Visibility;
+				}
+			}
+		}
+	}
+}

tools/generator/Tests/Unit-Tests/MethodVisibilityFixupTests.cs

@@ -0,0 +1,176 @@
+using System;
+using MonoDroid.Generation;
+using NUnit.Framework;
+
+namespace generatortests
+{
+	[TestFixture]
+	public class MethodVisibilityFixupTests
+	{
+		CodeGenerationOptions opt;
+		GenericParameterDefinitionList list;
+		CodeGeneratorContext context;
+
+		[SetUp]
+		public void SetUp ()
+		{
+			opt = new CodeGenerationOptions ();
+			list = new GenericParameterDefinitionList ();
+			context = new CodeGeneratorContext ();
+		}
+
+		[Test]
+		public void FixupBaseOverride ()
+		{
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
+
+			base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
+			base_class.Methods [0].Visibility = "protected";
+			opt.SymbolTable.AddType (base_class);
+
+			this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
+			this_class.Methods [0].Visibility = "public";
+			this_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
+		}
+
+		[Test]
+		public void FixupRecursiveBaseOverride ()
+		{
+			// This throws a middle class into the inheritance tree that does
+			// not override the method to ensure we look recursively
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var middle_class = new TestClass ("com.mypackage.Foo", "com.mypackage.Middle");
+			var this_class = new TestClass ("com.mypackage.Middle", "com.mypackage.MyClass");
+
+			base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
+			base_class.Methods [0].Visibility = "protected";
+			opt.SymbolTable.AddType (base_class);
+
+			opt.SymbolTable.AddType (middle_class);
+
+			this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
+			this_class.Methods [0].Visibility = "public";
+			this_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
+		}
+
+		[Test]
+		public void FixupNestedBaseOverride ()
+		{
+			// This tests:
+			// - MyClass: public override void DoSomething ()
+			//   |- MiddleClass: public override void DoSomething ()
+			//      |- BaseClass: protected virtual void DoSomething ()
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var middle_class = new TestClass ("com.mypackage.Foo", "com.mypackage.Middle");
+			var this_class = new TestClass ("com.mypackage.Middle", "com.mypackage.MyClass");
+
+			base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
+			base_class.Methods [0].Visibility = "protected";
+			opt.SymbolTable.AddType (base_class);
+
+			middle_class.Methods.Add (SupportTypeBuilder.CreateMethod (middle_class, "DoSomething", opt));
+			middle_class.Methods [0].Visibility = "public";
+			middle_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (middle_class);
+
+			this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
+			this_class.Methods [0].Visibility = "public";
+			this_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			middle_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
+		}
+
+		[Test]
+		public void FixupBaseProtectedInternalOverride ()
+		{
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
+
+			base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
+			base_class.Methods [0].Visibility = "protected internal";
+			opt.SymbolTable.AddType (base_class);
+
+			this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
+			this_class.Methods [0].Visibility = "public";
+			this_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("protected internal", this_class.Methods [0].Visibility);
+		}
+
+		[Test]
+		public void IgnoreValidPublic ()
+		{
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
+
+			base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
+			base_class.Methods [0].Visibility = "public";
+			opt.SymbolTable.AddType (base_class);
+
+			this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
+			this_class.Methods [0].Visibility = "public";
+			this_class.Methods [0].IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("public", this_class.Methods [0].Visibility);
+		}
+
+		[Test]
+		public void FixupPropertyBaseOverride ()
+		{
+			var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
+			var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
+
+			base_class.Properties.Add (SupportTypeBuilder.CreateProperty (base_class, "Count", "int", opt));
+			base_class.Properties [0].Getter.Visibility = "protected";
+			opt.SymbolTable.AddType (base_class);
+
+			this_class.Properties.Add (SupportTypeBuilder.CreateProperty (this_class, "Count", "int", opt));
+			this_class.Properties [0].Getter.Visibility = "public";
+			this_class.Properties [0].Getter.IsOverride = true;
+			opt.SymbolTable.AddType (this_class);
+
+			base_class.Validate (opt, list, context);
+			this_class.Validate (opt, list, context);
+
+			MethodVisibilityFixup.Fixup (this_class);
+
+			Assert.AreEqual ("protected", this_class.Properties [0].Getter.Visibility);
+		}
+	}
+}

tools/generator/Tests/generator-Tests.csproj

@@ -74,6 +74,7 @@
     <Compile Include="Unit-Tests\AdjusterTests.cs" />
     <Compile Include="Unit-Tests\DefaultInterfaceMethodsTests.cs" />
     <Compile Include="Unit-Tests\ManagedTests.cs" />
+    <Compile Include="Unit-Tests\MethodVisibilityFixupTests.cs" />
     <Compile Include="Unit-Tests\SupportTypes.cs" />
     <Compile Include="Unit-Tests\TestExtensions.cs" />
     <Compile Include="Unit-Tests\XmlTests.cs" />

tools/generator/generator.csproj

@@ -105,6 +105,7 @@
     <Compile Include="Java.Interop.Tools.Generator.ObjectModel\NamespaceMapping.cs" />
     <Compile Include="Java.Interop.Tools.Generator.ObjectModel\Parameter.cs" />
     <Compile Include="Java.Interop.Tools.Generator.ObjectModel\ParameterList.cs" />
+    <Compile Include="Java.Interop.Tools.Generator.Transformation\MethodVisibilityFixup.cs" />
     <Compile Include="Java.Interop.Tools.Generator.Transformation\Parser.cs" />
     <Compile Include="Utilities\AncestorDescendantCache.cs" />
     <Compile Include="Utilities\ProcessRocks.cs" />