Skip to content

Commit f8d77fa

Browse files
jpobstjpobst
authored
[generator] Better support deprecated property getter/setters. (#1062)
Fixes: #1033 Context: dotnet/roslyn#32571 When we are converting Java get-method and set-method pairs to C# properties, we can hit an interesting scenario where a getter may be `@Deprecated` and the setter is not, or vice versa: class Example { public boolean hasOptionsMenu () { ... } @deprecated public void setHasOptionsMenu (boolean hasMenu) { ... } } C# has traditionally not allowed `[Obsolete]` to be placed on just a `get` or a `set` block; it could only be on the entire property: partial class Example { [Obsolete] public bool HasOptionsMenu { get; set; } } This can lead to confusion because using the getter will report an obsolete warning when it is not obsolete. Thus, for properties, we only add `[Obsolete]` in 2 cases: 1. The `get` is obsolete and there is no `set` 2. Both the `get` and `set` are obsolete We have this comment in our code: // Unlike [Register], [Obsolete] cannot be put on property accessors, so we can apply them only under limited condition... However, the C# compiler team has determined that preventing `[Obsolete]` on property accessors was a bug, and has fixed it in C#8 via dotnet/roslyn#32571. Update `generator` to support scenarios in which only the Java getter or setter is marked as `@Deprecated`, by placing `[Obsolete]` on the appropriate `get` or `set` block: partial class Example { public bool HasOptionsMenu { get => … [Obsolete] set =>… } }
1 parent 5e6209e commit f8d77fa

File tree

3 files changed

+201
-7
lines changed

3 files changed

+201
-7
lines changed

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

Lines changed: 162 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,8 @@ public void ObsoletedOSPlatformAttributeSupport ()
565565
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a field deprecated since 25!\")]"), writer.ToString ());
566566
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a constructor deprecated since 25!\")]"), writer.ToString ());
567567
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a method deprecated since 25!\")]"), writer.ToString ());
568-
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property getter deprecated since 25! This is a property setter deprecated since 25!\")]"), writer.ToString ());
568+
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property getter deprecated since 25!\")]"), writer.ToString ());
569+
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property setter deprecated since 25!\")]"), writer.ToString ());
569570
}
570571

571572
[Test]
@@ -611,7 +612,152 @@ public void ObsoletedOSPlatformAttributeUnneededSupport ()
611612
}
612613

613614
[Test]
614-
[NonParallelizable] // We are setting a static property on Report
615+
public void ObsoleteGetterOnlyProperty ()
616+
{
617+
var xml = @"<api>
618+
<package name='java.lang' jni-name='java/lang'>
619+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
620+
</package>
621+
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
622+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
623+
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
624+
</class>
625+
</package>
626+
</api>";
627+
628+
var gens = ParseApiDefinition (xml);
629+
var iface = gens.Single (g => g.Name == "MyClass");
630+
631+
generator.Context.ContextTypes.Push (iface);
632+
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
633+
generator.Context.ContextTypes.Pop ();
634+
635+
// This should use [Obsolete] on the entire property because the getter is obsolete and there is no setter
636+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete (@\"deprecated\")]public virtual unsafe int Count".NormalizeLineEndings ()), writer.ToString ());
637+
638+
// Ensure we don't write getter attribute
639+
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
640+
}
641+
642+
[Test]
643+
public void ObsoletePropertyGetter ()
644+
{
645+
var xml = @"<api>
646+
<package name='java.lang' jni-name='java/lang'>
647+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
648+
</package>
649+
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
650+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
651+
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
652+
<method abstract='false' deprecated='not deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
653+
<parameter name='count' type='int' jni-type='I'></parameter>
654+
</method>
655+
</class>
656+
</package>
657+
</api>";
658+
659+
var gens = ParseApiDefinition (xml);
660+
var iface = gens.Single (g => g.Name == "MyClass");
661+
662+
generator.Context.ContextTypes.Push (iface);
663+
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
664+
generator.Context.ContextTypes.Pop ();
665+
666+
// This should use [Obsolete] on just the property getter since the setter is not obsolete
667+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
668+
}
669+
670+
[Test]
671+
public void ObsoletePropertySetter ()
672+
{
673+
var xml = @"<api>
674+
<package name='java.lang' jni-name='java/lang'>
675+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
676+
</package>
677+
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
678+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
679+
<method abstract='false' deprecated='not deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
680+
<method abstract='false' deprecated='deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
681+
<parameter name='count' type='int' jni-type='I'></parameter>
682+
</method>
683+
</class>
684+
</package>
685+
</api>";
686+
687+
var gens = ParseApiDefinition (xml);
688+
var iface = gens.Single (g => g.Name == "MyClass");
689+
690+
generator.Context.ContextTypes.Push (iface);
691+
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
692+
generator.Context.ContextTypes.Pop ();
693+
694+
// This should use [Obsolete] on just the property setter since the getter is not obsolete
695+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]set"), writer.ToString ());
696+
}
697+
698+
[Test]
699+
public void ObsoleteBothPropertyMethods ()
700+
{
701+
var xml = @"<api>
702+
<package name='java.lang' jni-name='java/lang'>
703+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
704+
</package>
705+
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
706+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
707+
<method abstract='false' deprecated='getter_message' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
708+
<method abstract='false' deprecated='setter_message' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
709+
<parameter name='count' type='int' jni-type='I'></parameter>
710+
</method>
711+
</class>
712+
</package>
713+
</api>";
714+
715+
var gens = ParseApiDefinition (xml);
716+
var iface = gens.Single (g => g.Name == "MyClass");
717+
718+
generator.Context.ContextTypes.Push (iface);
719+
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
720+
generator.Context.ContextTypes.Pop ();
721+
722+
// This should use [Obsolete] on both property methods because the deprecation messages are different
723+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"getter_message\")]get"), writer.ToString ());
724+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"setter_message\")]set"), writer.ToString ());
725+
}
726+
727+
[Test]
728+
public void ObsoleteEntireProperty ()
729+
{
730+
var xml = @"<api>
731+
<package name='java.lang' jni-name='java/lang'>
732+
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
733+
</package>
734+
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
735+
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
736+
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
737+
<method abstract='false' deprecated='deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
738+
<parameter name='count' type='int' jni-type='I'></parameter>
739+
</method>
740+
</class>
741+
</package>
742+
</api>";
743+
744+
var gens = ParseApiDefinition (xml);
745+
var iface = gens.Single (g => g.Name == "MyClass");
746+
747+
generator.Context.ContextTypes.Push (iface);
748+
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
749+
generator.Context.ContextTypes.Pop ();
750+
751+
// This should use [Obsolete] on the entire property because the getter and setter are both obsoleted with the same message
752+
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete (@\"deprecated\")]public virtual unsafe int Count".NormalizeLineEndings ()), writer.ToString ());
753+
754+
// Ensure we don't write getter/setter attributes
755+
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
756+
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]set"), writer.ToString ());
757+
}
758+
759+
[Test]
760+
[NonParallelizable] // We are setting a static property on Report
615761
public void WarnIfTypeNameMatchesNamespace ()
616762
{
617763
var @class = new TestClass ("Object", "java.myclass.MyClass");
@@ -650,6 +796,20 @@ public void DontWarnIfNestedTypeNameMatchesNamespace ()
650796
// The warning should not be raised if the nested type matches enclosing namespace
651797
Assert.False (sb.ToString ().Contains ("warning BG8403"));
652798
}
799+
800+
static string StripRegisterAttributes (string str)
801+
{
802+
// It is hard to test if the [Obsolete] is on the setter/etc due to the [Register], so remove all [Register]s
803+
// [global::System.Obsolete (@"setter_message")]
804+
// [Register ("setCount", "(I)V", "GetSetCount_IHandler")]
805+
// set {
806+
int index;
807+
808+
while ((index = str.IndexOf ("[Register", StringComparison.Ordinal)) > -1)
809+
str = str.Substring (0, index) + str.Substring (str.IndexOf (']', index) + 1);
810+
811+
return str;
812+
}
653813
}
654814

655815
[TestFixture]

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,5 +60,32 @@ public void AutoDetectEnumifiedOverrideProperties (AncestorDescendantCache cache
6060
}
6161

6262
public string ExplicitInterface => Getter?.ExplicitInterface ?? Setter?.ExplicitInterface;
63+
64+
public bool IsWholePropertyDeprecated {
65+
get {
66+
// If the getter isn't deprecated then the property isn't
67+
if (Getter?.Deprecated is null)
68+
return false;
69+
70+
// If the getter is deprecated and there is no setter then the property is deprecated
71+
if (Setter is null)
72+
return true;
73+
74+
// If the setter isn't deprecated then the property isn't
75+
if (Setter.Deprecated is null)
76+
return false;
77+
78+
// If the getter/setter deprecation messages differ, don't use whole property deprecation
79+
if (Getter.Deprecated != Setter.Deprecated)
80+
return false;
81+
82+
// If the getter/setter deprecation versions differ, don't use whole property deprecation
83+
if (Getter.DeprecatedSince != Setter.DeprecatedSince)
84+
return false;
85+
86+
// Getter/Setter deprecation is the same, use whole property deprecation
87+
return true;
88+
}
89+
}
6390
}
6491
}

‎tools/generator/SourceWriters/BoundProperty.cs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,18 @@ public BoundProperty (GenBase gen, Property property, CodeGenerationOptions opt,
7272
IsOverride = false;
7373
}
7474

75-
// Unlike [Register], [Obsolete] cannot be put on property accessors, so we can apply them only under limited condition...
76-
if (property.Getter.Deprecated != null && (property.Setter == null || property.Setter.Deprecated != null)) {
77-
var message = property.Getter.Deprecated.Trim () + (property.Setter != null && property.Setter.Deprecated != property.Getter.Deprecated ? " " + property.Setter.Deprecated.Trim () : null);
78-
var since = property.Getter?.DeprecatedSince ?? property.Setter?.DeprecatedSince;
79-
SourceWriterExtensions.AddObsolete (Attributes, message, opt, deprecatedSince: since);
75+
// Add [Obsolete] or [ObsoletedOSPlatform]
76+
if (property.IsWholePropertyDeprecated) {
77+
// This case applies [Obsolete] to the entire property
78+
SourceWriterExtensions.AddObsolete (Attributes, property.Getter.Deprecated.Trim (), opt, deprecatedSince: property.Getter.DeprecatedSince);
79+
} else {
80+
// This case applies [Obsolete] to just the getter
81+
if (property.Getter?.Deprecated != null)
82+
SourceWriterExtensions.AddObsolete (GetterAttributes, property.Getter.Deprecated.Trim (), opt, deprecatedSince: property.Getter?.DeprecatedSince);
83+
84+
// This case applies [Obsolete] to just the setter
85+
if (property.Setter?.Deprecated != null)
86+
SourceWriterExtensions.AddObsolete (SetterAttributes, property.Setter.Deprecated.Trim (), opt, deprecatedSince: property.Setter?.DeprecatedSince);
8087
}
8188

8289
SourceWriterExtensions.AddSupportedOSPlatform (Attributes, property.Getter, opt);

0 commit comments

Comments
 (0)