Skip to content

Commit ef82a07

Browse files
jonathanpeppersjonathanpeppers
authored
[generator] FixupMethodOverrides() checks properties (#1366)
Context: #1313 Fixes: dotnet/android#10509 In .NET 10 RC 1, `View.Foreground` is defined in C# as: [SupportedOSPlatform("android23.0")] public virtual Drawable? Foreground { get; set; } But `FrameLayout.Foreground` is defined as: [UnsupportedOSPlatform("android23.0")] public override Drawable? Foreground { get; set; } I think this is the exact same bug that was addressed in #1313 for methods, but we still have the issue for properties. I was able to duplicate the test and make a `getThing()` and `setThing()` methods in Java to see the issue. I updated the `FixupMethodOverrides()` to iterate over properties and do the same fixup for property getters and setters. This also needed to be done for: * Interfaces * Fields (think of `static final int` values)
1 parent 19be370 commit ef82a07

File tree

2 files changed

+221
-17
lines changed

2 files changed

+221
-17
lines changed

‎tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs‎

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1441,13 +1441,23 @@ public void UnsupportedOSPlatformConstFields ()
14411441
public void UnsupportedOSPlatformIgnoresMethodOverrides ()
14421442
{
14431443
// Given:
1444+
// Class inheritance scenario:
14441445
// public class TextView {
14451446
// public Object doThing () { ... }
14461447
// }
14471448
// public class TextView2 : TextView {
14481449
// public Object doThing () { ... } // removed-since = 30
14491450
// }
1450-
// We should not write [UnsupportedOSPlatform] on TextView2.doThing (), because the base method isn't "removed".
1451+
// Interface inheritance scenario:
1452+
// public interface IFoo {
1453+
// public Object doSomething () { ... }
1454+
// public static final String DATE_TAKEN = "datetaken";
1455+
// }
1456+
// public interface IBar : IFoo {
1457+
// public Object doSomething () { ... } // removed-since = 30
1458+
// public static final String DATE_TAKEN = "datetaken"; // removed-since = 30
1459+
// }
1460+
// We should not write [UnsupportedOSPlatform] on overriding methods or fields, because the base methods/fields aren't "removed".
14511461
var xml = @$"<api>
14521462
<package name='java.lang' jni-name='java/lang'>
14531463
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
@@ -1460,13 +1470,97 @@ public void UnsupportedOSPlatformIgnoresMethodOverrides ()
14601470
<method abstract='false' deprecated='not deprecated' final='false' name='doThing' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' removed-since='30' />
14611471
</class>
14621472
</package>
1473+
<package name='com.example' jni-name='com/example'>
1474+
<interface abstract='true' deprecated='not deprecated' final='false' name='Foo' static='false' visibility='public' jni-signature='Lcom/example/Foo;'>
1475+
<method abstract='true' deprecated='not deprecated' final='false' name='doSomething' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' />
1476+
<field deprecated='not deprecated' final='true' name='DATE_TAKEN' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;datetaken&quot;' visibility='public' volatile='false'></field>
1477+
</interface>
1478+
<interface abstract='true' deprecated='not deprecated' final='false' name='Bar' static='false' visibility='public' jni-signature='Lcom/example/Bar;'>
1479+
<implements name='com.example.Foo' name-generic-aware='com.example.Foo' jni-type='Lcom/example/Foo;'></implements>
1480+
<method abstract='true' deprecated='not deprecated' final='false' name='doSomething' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' removed-since='30' />
1481+
<field deprecated='not deprecated' final='true' name='DATE_TAKEN' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;datetaken&quot;' visibility='public' volatile='false' removed-since='30'></field>
1482+
</interface>
1483+
</package>
1484+
</api>";
1485+
1486+
var gens = ParseApiDefinition (xml);
1487+
1488+
// Test class inheritance scenario
1489+
var klass = gens.Single (g => g.Name == "TextView2");
1490+
var actual = GetGeneratedTypeOutput (klass);
1491+
StringAssert.DoesNotContain ("[global::System.Runtime.Versioning.UnsupportedOSPlatformAttribute (\"android30.0\")]", actual, "Should not contain UnsupportedOSPlatform on class override!");
1492+
1493+
// Test interface inheritance scenario
1494+
var iface = gens.OfType<InterfaceGen> ().Single (g => g.Name == "IBar");
1495+
var ifaceActual = GetGeneratedTypeOutput (iface);
1496+
StringAssert.DoesNotContain ("[global::System.Runtime.Versioning.UnsupportedOSPlatformAttribute (\"android30.0\")]", ifaceActual, "Should not contain UnsupportedOSPlatform on interface override!");
1497+
}
1498+
1499+
[Test]
1500+
public void UnsupportedOSPlatformIgnoresPropertyOverrides ()
1501+
{
1502+
// Given:
1503+
// Class inheritance scenario:
1504+
// public class TextView {
1505+
// public Object getThing () { ... }
1506+
// public void setThing (Object value) { ... }
1507+
// }
1508+
// public class TextView2 : TextView {
1509+
// public Object getThing () { ... } // removed-since = 30
1510+
// public void setThing (Object value) { ... } // removed-since = 30
1511+
// }
1512+
// Interface inheritance scenario:
1513+
// public interface IPropertyProvider {
1514+
// public Object getSomething () { ... }
1515+
// public static final String DATE_TAKEN = "datetaken";
1516+
// }
1517+
// public interface IExtendedProvider : IPropertyProvider {
1518+
// public Object getSomething () { ... } // removed-since = 30
1519+
// public static final String DATE_TAKEN = "datetaken"; // removed-since = 30
1520+
// }
1521+
// We should not write [UnsupportedOSPlatform] on overriding properties or fields, because the base methods/fields aren't "removed".
1522+
var xml = @$"<api>
1523+
<package name='java.lang' jni-name='java/lang'>
1524+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
1525+
</package>
1526+
<package name='android.widget' jni-name='android/widget'>
1527+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' final='false' name='TextView' static='false' visibility='public'>
1528+
<method abstract='false' deprecated='not deprecated' final='false' name='getThing' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' />
1529+
<method abstract='false' deprecated='not deprecated' final='false' name='setThing' bridge='false' native='false' return='void' static='false' synchronized='false' synthetic='false' visibility='public'>
1530+
<parameter name='value' type='java.lang.Object' />
1531+
</method>
1532+
</class>
1533+
<class abstract='false' deprecated='not deprecated' extends='android.widget.TextView' extends-generic-aware='java.lang.Object' final='false' name='TextView2' static='false' visibility='public'>
1534+
<method abstract='false' deprecated='not deprecated' final='false' name='getThing' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' removed-since='30' />
1535+
<method abstract='false' deprecated='not deprecated' final='false' name='setThing' bridge='false' native='false' return='void' static='false' synchronized='false' synthetic='false' visibility='public' removed-since='30'>
1536+
<parameter name='value' type='java.lang.Object' />
1537+
</method>
1538+
</class>
1539+
</package>
1540+
<package name='com.example' jni-name='com/example'>
1541+
<interface abstract='true' deprecated='not deprecated' final='false' name='PropertyProvider' static='false' visibility='public' jni-signature='Lcom/example/PropertyProvider;'>
1542+
<method abstract='true' deprecated='not deprecated' final='false' name='getSomething' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' />
1543+
<field deprecated='not deprecated' final='true' name='DATE_TAKEN' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;datetaken&quot;' visibility='public' volatile='false'></field>
1544+
</interface>
1545+
<interface abstract='true' deprecated='not deprecated' final='false' name='ExtendedProvider' static='false' visibility='public' jni-signature='Lcom/example/ExtendedProvider;'>
1546+
<implements name='com.example.PropertyProvider' name-generic-aware='com.example.PropertyProvider' jni-type='Lcom/example/PropertyProvider;'></implements>
1547+
<method abstract='true' deprecated='not deprecated' final='false' name='getSomething' bridge='false' native='false' return='java.lang.Object' static='false' synchronized='false' synthetic='false' visibility='public' removed-since='30' />
1548+
<field deprecated='not deprecated' final='true' name='DATE_TAKEN' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;datetaken&quot;' visibility='public' volatile='false' removed-since='30'></field>
1549+
</interface>
1550+
</package>
14631551
</api>";
14641552

14651553
var gens = ParseApiDefinition (xml);
1554+
1555+
// Test class inheritance scenario
14661556
var klass = gens.Single (g => g.Name == "TextView2");
14671557
var actual = GetGeneratedTypeOutput (klass);
1558+
StringAssert.DoesNotContain ("[global::System.Runtime.Versioning.UnsupportedOSPlatformAttribute (\"android30.0\")]", actual, "Should not contain UnsupportedOSPlatform on class property override!");
14681559

1469-
StringAssert.DoesNotContain ("[global::System.Runtime.Versioning.UnsupportedOSPlatformAttribute (\"android30.0\")]", actual, "Should contain UnsupportedOSPlatform!");
1560+
// Test interface inheritance scenario
1561+
var iface = gens.OfType<InterfaceGen> ().Single (g => g.Name == "IExtendedProvider");
1562+
var ifaceActual = GetGeneratedTypeOutput (iface);
1563+
StringAssert.DoesNotContain ("[global::System.Runtime.Versioning.UnsupportedOSPlatformAttribute (\"android30.0\")]", ifaceActual, "Should not contain UnsupportedOSPlatform on interface property override!");
14701564
}
14711565

14721566
[Test]

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

Lines changed: 125 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -307,27 +307,137 @@ public virtual void FixupExplicitImplementation ()
307307

308308
public void FixupMethodOverrides (CodeGenerationOptions opt)
309309
{
310+
// Process regular methods (non-static, non-interface default methods)
310311
foreach (var m in Methods.Where (m => !m.IsStatic && !m.IsInterfaceDefaultMethod)) {
311312
for (var bt = GetBaseGen (opt); bt != null; bt = bt.GetBaseGen (opt)) {
312-
var bm = bt.Methods.FirstOrDefault (mm => mm.Name == m.Name && mm.Visibility == m.Visibility && ParameterList.Equals (mm.Parameters, m.Parameters));
313-
if (bm != null && bm.RetVal.FullName == m.RetVal.FullName) { // if return type is different, it could be still "new", not "override".
314-
m.IsOverride = true;
315-
316-
if (opt.FixObsoleteOverrides) {
317-
// If method overrides a deprecated method, it also needs to be marked as deprecated
318-
if (bm.Deprecated.HasValue () && !m.Deprecated.HasValue ())
319-
m.Deprecated = bm.Deprecated;
320-
321-
// Fix issue when base method was deprecated before the overriding method, set both both to base method value
322-
if (bm.DeprecatedSince.GetValueOrDefault (default) < m.DeprecatedSince.GetValueOrDefault (default))
323-
m.DeprecatedSince = bm.DeprecatedSince;
313+
var bm = bt.Methods.FirstOrDefault (mm =>
314+
mm.Name == m.Name &&
315+
mm.Visibility == m.Visibility &&
316+
mm.RetVal.FullName == m.RetVal.FullName && // if return type is different, it could be still "new", not "override".
317+
ParameterList.Equals (mm.Parameters, m.Parameters));
318+
if (bm == null) {
319+
continue;
320+
}
321+
322+
m.IsOverride = true;
323+
if (opt.FixObsoleteOverrides) {
324+
// If method overrides a deprecated method, it also needs to be marked as deprecated
325+
if (bm.Deprecated.HasValue () && !m.Deprecated.HasValue ())
326+
m.Deprecated = bm.Deprecated;
327+
328+
// Fix issue when base method was deprecated before the overriding method, set both both to base method value
329+
if (bm.DeprecatedSince.GetValueOrDefault (default) < m.DeprecatedSince.GetValueOrDefault (default))
330+
m.DeprecatedSince = bm.DeprecatedSince;
331+
}
332+
333+
// If a "removed" method overrides a "not removed" method, the method was
334+
// likely moved to a base class, so don't mark it as removed.
335+
if (m.ApiRemovedSince > 0 && bm.ApiRemovedSince == 0) {
336+
m.ApiRemovedSince = default;
337+
}
338+
break;
339+
}
340+
}
341+
342+
// Process property getter/setter methods for ApiRemovedSince fixup
343+
foreach (var prop in Properties) {
344+
for (var bt = GetBaseGen (opt); bt != null; bt = bt.GetBaseGen (opt)) {
345+
var baseProp = bt.Properties.FirstOrDefault (p => p.Name == prop.Name && p.Type == prop.Type);
346+
if (baseProp == null) {
347+
continue;
348+
}
349+
350+
bool shouldBreak = false;
351+
if (prop.Getter != null && prop.Getter.ApiRemovedSince > 0 && baseProp.Getter != null && baseProp.Getter.ApiRemovedSince == 0) {
352+
if (baseProp.Getter.Visibility == prop.Getter.Visibility &&
353+
ParameterList.Equals (baseProp.Getter.Parameters, prop.Getter.Parameters) &&
354+
baseProp.Getter.RetVal.FullName == prop.Getter.RetVal.FullName) {
355+
// If a "removed" property getter overrides a "not removed" getter, the method was
356+
// likely moved to a base class, so don't mark it as removed.
357+
prop.Getter.ApiRemovedSince = default;
358+
shouldBreak = true;
324359
}
360+
}
361+
if (prop.Setter != null && prop.Setter.ApiRemovedSince > 0 && baseProp.Setter != null && baseProp.Setter.ApiRemovedSince == 0) {
362+
if (baseProp.Setter.Visibility == prop.Setter.Visibility &&
363+
ParameterList.Equals (baseProp.Setter.Parameters, prop.Setter.Parameters)) {
364+
// If a "removed" property setter overrides a "not removed" setter, the method was
365+
// likely moved to a base class, so don't mark it as removed.
366+
prop.Setter.ApiRemovedSince = default;
367+
shouldBreak = true;
368+
}
369+
}
370+
if (shouldBreak) {
371+
break;
372+
}
373+
}
374+
}
325375

326-
// If a "removed" method overrides a "not removed" method, the method was
327-
// likely moved to a base class, so don't mark it as removed.
328-
if (m.ApiRemovedSince > 0 && bm.ApiRemovedSince == 0)
376+
// Process interface inheritance for both regular and default interface methods
377+
if (this is InterfaceGen currentInterface) {
378+
// For interfaces, check all base interfaces (interfaces that this interface implements/extends)
379+
var baseInterfaces = currentInterface.GetAllDerivedInterfaces ();
380+
381+
foreach (var m in Methods.Where (m => !m.IsStatic)) {
382+
foreach (var baseIface in baseInterfaces) {
383+
var bm = baseIface.Methods.FirstOrDefault (mm =>
384+
mm.Name == m.Name &&
385+
mm.Visibility == m.Visibility &&
386+
mm.RetVal.FullName == m.RetVal.FullName &&
387+
ParameterList.Equals (mm.Parameters, m.Parameters));
388+
if (bm == null) {
389+
continue;
390+
}
391+
// If a "removed" interface method overrides a "not removed" method, the method was
392+
// likely moved to a base interface, so don't mark it as removed.
393+
if (m.ApiRemovedSince > 0 && bm.ApiRemovedSince == 0) {
329394
m.ApiRemovedSince = default;
395+
}
396+
break;
397+
}
398+
}
399+
400+
// Process interface property getter/setter methods for ApiRemovedSince fixup
401+
foreach (var prop in Properties) {
402+
foreach (var baseIface in baseInterfaces) {
403+
var baseProp = baseIface.Properties.FirstOrDefault (p => p.Name == prop.Name && p.Type == prop.Type);
404+
if (baseProp == null)
405+
continue;
406+
407+
bool shouldBreak = false;
408+
if (prop.Getter != null && prop.Getter.ApiRemovedSince > 0 && baseProp.Getter != null && baseProp.Getter.ApiRemovedSince == 0) {
409+
if (baseProp.Getter.Visibility == prop.Getter.Visibility &&
410+
ParameterList.Equals (baseProp.Getter.Parameters, prop.Getter.Parameters) &&
411+
baseProp.Getter.RetVal.FullName == prop.Getter.RetVal.FullName) {
412+
prop.Getter.ApiRemovedSince = default;
413+
shouldBreak = true;
414+
}
415+
}
416+
if (prop.Setter != null && prop.Setter.ApiRemovedSince > 0 && baseProp.Setter != null && baseProp.Setter.ApiRemovedSince == 0) {
417+
if (baseProp.Setter.Visibility == prop.Setter.Visibility &&
418+
ParameterList.Equals (baseProp.Setter.Parameters, prop.Setter.Parameters)) {
419+
prop.Setter.ApiRemovedSince = default;
420+
shouldBreak = true;
421+
}
422+
}
423+
if (shouldBreak) {
424+
break;
425+
}
426+
}
427+
}
330428

429+
// Process interface field inheritance for ApiRemovedSince fixup
430+
foreach (var field in Fields) {
431+
foreach (var baseIface in baseInterfaces) {
432+
var baseField = baseIface.Fields.FirstOrDefault (f => f.Name == field.Name && f.TypeName == field.TypeName && f.Visibility == field.Visibility);
433+
if (baseField == null) {
434+
continue;
435+
}
436+
// If a "removed" interface field overrides a "not removed" field, the field was
437+
// likely moved to a base interface, so don't mark it as removed.
438+
if (field.ApiRemovedSince > 0 && baseField.ApiRemovedSince == 0) {
439+
field.ApiRemovedSince = default;
440+
}
331441
break;
332442
}
333443
}

0 commit comments

Comments
 (0)