Skip to content

Commit 5622deb

Browse files
jpobstjpobst
committed
[generator] Ensure consistent member visibility. [#367]
1 parent f786463 commit 5622deb

File tree

8 files changed

+256
-4
lines changed

8 files changed

+256
-4
lines changed

‎tools/generator/CodeGenerator.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,9 @@ static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolve
160160
foreach (GenBase gen in gens)
161161
gen.FixupExplicitImplementation ();
162162

163+
foreach (GenBase gen in gens)
164+
MethodVisibilityFixup.Fixup (gen);
165+
163166
GenerateAnnotationAttributes (gens, annotations_zips);
164167

165168
//SymbolTable.Dump ();

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -946,9 +946,9 @@ public static string GetSignature (MethodBase method, CodeGenerationOptions opt)
946946
return sb.ToString ();
947947
}
948948

949-
static IEnumerable<Method> GetAllMethods (GenBase t)
949+
public IEnumerable<Method> GetAllMethods ()
950950
{
951-
return t.Methods.Concat (t.Properties.Select (p => p.Getter)).Concat (t.Properties.Select (p => p.Setter).Where (m => m != null));
951+
return Methods.Concat (Properties.Select (p => p.Getter)).Concat (Properties.Select (p => p.Setter).Where (m => m != null));
952952
}
953953

954954
static string [] AutoDetectEnumifiedOverrideParameters (MethodBase method, AncestorDescendantCache cache)
@@ -958,7 +958,7 @@ static string [] AutoDetectEnumifiedOverrideParameters (MethodBase method, Ances
958958
var classes = cache.GetAncestorsAndDescendants (method.DeclaringType);
959959
classes = classes.Concat (classes.SelectMany(x => x.GetAllImplementedInterfaces ()));
960960
foreach (var t in classes) {
961-
foreach (var candidate in GetAllMethods (t).Where (m => m.Name == method.Name
961+
foreach (var candidate in t.GetAllMethods ().Where (m => m.Name == method.Name
962962
&& m.Parameters.Count == method.Parameters.Count
963963
&& m.Parameters.Any (p => p.IsEnumified))) {
964964
var ret = new string [method.Parameters.Count];
@@ -989,7 +989,7 @@ static string AutoDetectEnumifiedOverrideReturn (Method method, AncestorDescenda
989989
var classes = cache.GetAncestorsAndDescendants (method.DeclaringType);
990990
classes = classes.Concat (classes.SelectMany(x => x.GetAllImplementedInterfaces ()));
991991
foreach (var t in classes) {
992-
foreach (var candidate in GetAllMethods (t).Where (m => m.Name == method.Name && m.Parameters.Count == method.Parameters.Count)) {
992+
foreach (var candidate in t.GetAllMethods ().Where (m => m.Name == method.Name && m.Parameters.Count == method.Parameters.Count)) {
993993
if (method.JniSignature != candidate.JniSignature)
994994
continue;
995995
if (candidate.IsReturnEnumified)

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Linq;
23

34
namespace MonoDroid.Generation
45
{
@@ -245,6 +246,47 @@ internal string GetAdapterName (CodeGenerationOptions opt, string adapter)
245246
return adapter + ", " + opt.AssemblyName;
246247
return adapter + AssemblyName;
247248
}
249+
250+
// This gets the Method's most direct ancestor
251+
public Method GetBaseMethod ()
252+
{
253+
if (!is_override)
254+
return null;
255+
256+
var base_class = DeclaringType?.BaseGen;
257+
258+
if (base_class == null)
259+
return null;
260+
261+
while (base_class != null) {
262+
var method = base_class.GetAllMethods ()
263+
.FirstOrDefault (m => m.Name == Name
264+
&& ParameterList.Equals (m.Parameters, Parameters)
265+
&& m.ReturnType == ReturnType);
266+
267+
if (method != null)
268+
return method;
269+
270+
base_class = base_class.BaseGen;
271+
}
272+
273+
return null;
274+
}
275+
276+
// This gets the instance where the Method is originally declared (ie: it may be several ancestors up)
277+
public Method GetBaseMethodDeclaration ()
278+
{
279+
var candidate = GetBaseMethod ();
280+
var last_candidate = (Method) null;
281+
282+
while (true) {
283+
if (candidate is null)
284+
return last_candidate;
285+
286+
last_candidate = candidate;
287+
candidate = last_candidate.GetBaseMethod ();
288+
}
289+
}
248290
}
249291
}
250292

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
using System.Text;
5+
using System.Threading.Tasks;
6+
7+
namespace MonoDroid.Generation
8+
{
9+
// Java allows a public member to override a protected member, but C# does not.
10+
// This Fixup makes the override member protected as well.
11+
static class MethodVisibilityFixup
12+
{
13+
public static void Fixup (GenBase gen)
14+
{
15+
foreach (var method in gen.GetAllMethods ()) {
16+
var base_method = method.GetBaseMethodDeclaration ();
17+
18+
if (base_method == null)
19+
continue;
20+
21+
if (method.Visibility == "public" && (base_method.Visibility == "protected" || base_method.Visibility == "protected internal")) {
22+
Report.Warning (0, Report.WarningInconsistentAccessbility, $"Setting {gen.FullName}.{method.Name} visibility to {base_method.Visibility} to match base method");
23+
method.Visibility = base_method.Visibility;
24+
}
25+
}
26+
}
27+
}
28+
}

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

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
using System;
2+
using MonoDroid.Generation;
3+
using NUnit.Framework;
4+
5+
namespace generatortests
6+
{
7+
[TestFixture]
8+
public class MethodVisibilityFixupTests
9+
{
10+
CodeGenerationOptions opt;
11+
GenericParameterDefinitionList list;
12+
CodeGeneratorContext context;
13+
14+
[SetUp]
15+
public void SetUp ()
16+
{
17+
opt = new CodeGenerationOptions ();
18+
list = new GenericParameterDefinitionList ();
19+
context = new CodeGeneratorContext ();
20+
}
21+
22+
[Test]
23+
public void FixupBaseOverride ()
24+
{
25+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
26+
var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
27+
28+
base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
29+
base_class.Methods [0].Visibility = "protected";
30+
opt.SymbolTable.AddType (base_class);
31+
32+
this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
33+
this_class.Methods [0].Visibility = "public";
34+
this_class.Methods [0].IsOverride = true;
35+
opt.SymbolTable.AddType (this_class);
36+
37+
base_class.Validate (opt, list, context);
38+
this_class.Validate (opt, list, context);
39+
40+
MethodVisibilityFixup.Fixup (this_class);
41+
42+
Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
43+
}
44+
45+
[Test]
46+
public void FixupRecursiveBaseOverride ()
47+
{
48+
// This throws a middle class into the inheritance tree that does
49+
// not override the method to ensure we look recursively
50+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
51+
var middle_class = new TestClass ("com.mypackage.Foo", "com.mypackage.Middle");
52+
var this_class = new TestClass ("com.mypackage.Middle", "com.mypackage.MyClass");
53+
54+
base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
55+
base_class.Methods [0].Visibility = "protected";
56+
opt.SymbolTable.AddType (base_class);
57+
58+
opt.SymbolTable.AddType (middle_class);
59+
60+
this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
61+
this_class.Methods [0].Visibility = "public";
62+
this_class.Methods [0].IsOverride = true;
63+
opt.SymbolTable.AddType (this_class);
64+
65+
base_class.Validate (opt, list, context);
66+
this_class.Validate (opt, list, context);
67+
68+
MethodVisibilityFixup.Fixup (this_class);
69+
70+
Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
71+
}
72+
73+
[Test]
74+
public void FixupNestedBaseOverride ()
75+
{
76+
// This tests:
77+
// - MyClass: public override void DoSomething ()
78+
// |- MiddleClass: public override void DoSomething ()
79+
// |- BaseClass: protected virtual void DoSomething ()
80+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
81+
var middle_class = new TestClass ("com.mypackage.Foo", "com.mypackage.Middle");
82+
var this_class = new TestClass ("com.mypackage.Middle", "com.mypackage.MyClass");
83+
84+
base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
85+
base_class.Methods [0].Visibility = "protected";
86+
opt.SymbolTable.AddType (base_class);
87+
88+
middle_class.Methods.Add (SupportTypeBuilder.CreateMethod (middle_class, "DoSomething", opt));
89+
middle_class.Methods [0].Visibility = "public";
90+
middle_class.Methods [0].IsOverride = true;
91+
opt.SymbolTable.AddType (middle_class);
92+
93+
this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
94+
this_class.Methods [0].Visibility = "public";
95+
this_class.Methods [0].IsOverride = true;
96+
opt.SymbolTable.AddType (this_class);
97+
98+
base_class.Validate (opt, list, context);
99+
middle_class.Validate (opt, list, context);
100+
this_class.Validate (opt, list, context);
101+
102+
MethodVisibilityFixup.Fixup (this_class);
103+
104+
Assert.AreEqual ("protected", this_class.Methods [0].Visibility);
105+
}
106+
107+
[Test]
108+
public void FixupBaseProtectedInternalOverride ()
109+
{
110+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
111+
var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
112+
113+
base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
114+
base_class.Methods [0].Visibility = "protected internal";
115+
opt.SymbolTable.AddType (base_class);
116+
117+
this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
118+
this_class.Methods [0].Visibility = "public";
119+
this_class.Methods [0].IsOverride = true;
120+
opt.SymbolTable.AddType (this_class);
121+
122+
base_class.Validate (opt, list, context);
123+
this_class.Validate (opt, list, context);
124+
125+
MethodVisibilityFixup.Fixup (this_class);
126+
127+
Assert.AreEqual ("protected internal", this_class.Methods [0].Visibility);
128+
}
129+
130+
[Test]
131+
public void IgnoreValidPublic ()
132+
{
133+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
134+
var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
135+
136+
base_class.Methods.Add (SupportTypeBuilder.CreateMethod (base_class, "DoSomething", opt));
137+
base_class.Methods [0].Visibility = "public";
138+
opt.SymbolTable.AddType (base_class);
139+
140+
this_class.Methods.Add (SupportTypeBuilder.CreateMethod (this_class, "DoSomething", opt));
141+
this_class.Methods [0].Visibility = "public";
142+
this_class.Methods [0].IsOverride = true;
143+
opt.SymbolTable.AddType (this_class);
144+
145+
base_class.Validate (opt, list, context);
146+
this_class.Validate (opt, list, context);
147+
148+
MethodVisibilityFixup.Fixup (this_class);
149+
150+
Assert.AreEqual ("public", this_class.Methods [0].Visibility);
151+
}
152+
153+
[Test]
154+
public void FixupPropertyBaseOverride ()
155+
{
156+
var base_class = new TestClass ("java.lang.Object", "com.mypackage.Foo");
157+
var this_class = new TestClass ("com.mypackage.Foo", "com.mypackage.MyClass");
158+
159+
base_class.Properties.Add (SupportTypeBuilder.CreateProperty (base_class, "Count", "int", opt));
160+
base_class.Properties [0].Getter.Visibility = "protected";
161+
opt.SymbolTable.AddType (base_class);
162+
163+
this_class.Properties.Add (SupportTypeBuilder.CreateProperty (this_class, "Count", "int", opt));
164+
this_class.Properties [0].Getter.Visibility = "public";
165+
this_class.Properties [0].Getter.IsOverride = true;
166+
opt.SymbolTable.AddType (this_class);
167+
168+
base_class.Validate (opt, list, context);
169+
this_class.Validate (opt, list, context);
170+
171+
MethodVisibilityFixup.Fixup (this_class);
172+
173+
Assert.AreEqual ("protected", this_class.Properties [0].Getter.Visibility);
174+
}
175+
}
176+
}

‎tools/generator/Tests/generator-Tests.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@
7474
<Compile Include="Unit-Tests\AdjusterTests.cs" />
7575
<Compile Include="Unit-Tests\DefaultInterfaceMethodsTests.cs" />
7676
<Compile Include="Unit-Tests\ManagedTests.cs" />
77+
<Compile Include="Unit-Tests\MethodVisibilityFixupTests.cs" />
7778
<Compile Include="Unit-Tests\SupportTypes.cs" />
7879
<Compile Include="Unit-Tests\TestExtensions.cs" />
7980
<Compile Include="Unit-Tests\XmlTests.cs" />

‎tools/generator/Utilities/Report.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ public class Report
2929
public const int WarningGenBase = 0x8C00;
3030
public const int WarningMethodBase = 0x8D00;
3131
public const int WarningAnnotationsProvider = 0x8E00;
32+
public const int WarningInconsistentAccessbility = 0x8F00;
3233

3334
public static int? Verbosity { get; set; }
3435

‎tools/generator/generator.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@
105105
<Compile Include="Java.Interop.Tools.Generator.ObjectModel\NamespaceMapping.cs" />
106106
<Compile Include="Java.Interop.Tools.Generator.ObjectModel\Parameter.cs" />
107107
<Compile Include="Java.Interop.Tools.Generator.ObjectModel\ParameterList.cs" />
108+
<Compile Include="Java.Interop.Tools.Generator.Transformation\MethodVisibilityFixup.cs" />
108109
<Compile Include="Java.Interop.Tools.Generator.Transformation\Parser.cs" />
109110
<Compile Include="Utilities\AncestorDescendantCache.cs" />
110111
<Compile Include="Utilities\ProcessRocks.cs" />

0 commit comments

Comments
 (0)