Skip to content

Commit

Permalink
[generator] enum map.csv can set @deprecated-since for enum values (#…
Browse files Browse the repository at this point in the history
…1070)

Context: dotnet/android#7670

While auditing the `Mono.Android.dll`, we've found that some constants
were added to the wrong enums.

Consider [`xamarin-android/src/Mono.Android/map.csv`][0],
lines 739-743:

	E,28,android/app/usage/UsageStatsManager.STANDBY_BUCKET_ACTIVE,10,Android.App.Usage.StandbyBucket,Active,remove,
	E,28,android/app/usage/UsageStatsManager.STANDBY_BUCKET_FREQUENT,30,Android.App.Usage.StandbyBucket,Frequent,remove,
	E,28,android/app/usage/UsageStatsManager.STANDBY_BUCKET_RARE,40,Android.App.Usage.StandbyBucket,Rare,remove,
	E,30,android/app/usage/UsageStatsManager.STANDBY_BUCKET_RESTRICTED,45,Android.App.Usage.UsageStatsInterval,Restricted,remove,
	E,28,android/app/usage/UsageStatsManager.STANDBY_BUCKET_WORKING_SET,20,Android.App.Usage.StandbyBucket,WorkingSet,remove,


`STANDBY_BUCKET_RESTRICTED` should have been added to `StandbyBucket`
instead of `UsageStatsInterval`.

To fix this in a backwards-compatible way, we can add it to both enums:

	E,30,android/app/usage/UsageStatsManager.STANDBY_BUCKET_RESTRICTED,45,Android.App.Usage.StandbyBucket,Restricted,remove,
	A,30,,45,Android.App.Usage.UsageStatsInterval,Restricted,remove,

However, we have no way to mark `UsageStatsInterval.Restricted` as
`[Obsolete]`, so people may still unintentionally use it.

Add a new field to our csv that allows us to set a constant's
`@deprecated-since` field, the `30` at the end:

	A,30,,45,Android.App.Usage.UsageStatsInterval,Restricted,remove,,30

This allows us to generate:

	[global::System.Runtime.Versioning.ObsoletedOSPlatform ("android30.0")]
	Restricted = 45

Additionally, when the cause is that we accidentally added an invalid
value to the enum, we should provide a better obsolete message, letting
the user know that the value will not function.  This is done by
setting `@deprecated-since` to `-1`:

	A,30,,45,Android.App.Usage.UsageStatsInterval,Restricted,remove,,-1

resulting in:

	[global::System.Obsolete (@"This value was incorrectly added to the enumeration and is not a valid value")]
	Restricted = 45

[0]: https://github.com/xamarin/xamarin-android/blob/cc70ce20aff6934532a8cb6a7bddbf3710fd7e1c/src/Mono.Android/map.csv
  • Loading branch information
jpobst authored Jan 7, 2023
1 parent f8d77fa commit 5c5dc08
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 10 deletions.
10 changes: 7 additions & 3 deletions Documentation/EnumMappingFile.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ There is now a "v2" version of defining constants. This is a line-level change
and you can mix "v1" and "v2" lines in the same file for backwards compatibility,
but for consistency it's probably better to stick to one style.

A "v2" line contains up to 8 fields:
A "v2" line contains up to 9 fields:

* **Action** - The action to perform. This is what denotes a "v2" line, if the first
character is not one of the following it will be treated as "v1".
Expand All @@ -89,10 +89,14 @@ A "v2" line contains up to 8 fields:
* `keep` - Keeps the Java constant
* **Flags** - If this field contains `flags` the enum will be created with the
`[Flags]` attribute. (Any member will `flags` will make the whole enum `[Flags]`.)

* **Deprecated Since** - This is generally only used by `Mono.Android.dll` to denote
the Android level the constant was deprecated in. Specifying "-1" will add an obsolete
message to the effect of: "This value was incorrectly added to the enumeration and is
not a valid value". Leave blank if constant is not deprecated.

Full example:
```
E,10,android/view/Window.PROGRESS_START,0,Android.Views.WindowProgress,Start,remove,flags
E,10,android/view/Window.PROGRESS_START,0,Android.Views.WindowProgress,Start,remove,flags,30
```

[0]: https://docs.microsoft.com/en-us/xamarin/android/platform/binding-java-library/customizing-bindings/java-bindings-metadata#enumfieldsxml-and-enummethodsxml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public class ConstantEntry
public string? EnumMember { get; set; }
public FieldAction FieldAction { get; set; }
public bool IsFlags { get; set; }
public int? DeprecatedSince { get; set; }

public string EnumNamespace {
get {
Expand Down Expand Up @@ -133,7 +134,8 @@ static ConstantEntry FromVersion2String (CsvParser parser)
EnumFullType = parser.GetField (4),
EnumMember = parser.GetField (5),
FieldAction = FromFieldActionString (parser.GetField (6)),
IsFlags = parser.GetField (7).ToLowerInvariant () == "flags"
IsFlags = parser.GetField (7).ToLowerInvariant () == "flags",
DeprecatedSince = parser.GetFieldAsNullableInt32 (8)
};

entry.NormalizeJavaSignature ();
Expand Down Expand Up @@ -175,7 +177,8 @@ public string ToVersion2String ()
EnumFullType,
EnumMember,
ToConstantFieldActionString (FieldAction),
IsFlags ? "flags" : string.Empty
IsFlags ? "flags" : string.Empty,
DeprecatedSince.HasValue ? DeprecatedSince.Value.ToString () : string.Empty,
};

return string.Join (",", fields);
Expand Down
14 changes: 14 additions & 0 deletions src/Java.Interop.Tools.Generator/Enumification/ConstantsParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,20 @@ public static void SaveEnumMapCsv (List<ConstantEntry> constants, string filenam

public static void SaveEnumMapCsv (List<ConstantEntry> constants, TextWriter writer)
{
var column_names = new [] {
"Action",
"API Level",
"JNI Signature",
"Enum Value",
"C# Enum Type",
"C# Member Name",
"Field Action",
"Is Flags",
"Deprecated Since",
};

writer.WriteLine ("// " + string.Join (",", column_names));

foreach (var c in Sort (constants))
writer.WriteLine (c.ToVersion2String ());
}
Expand Down
10 changes: 10 additions & 0 deletions src/Java.Interop.Tools.Generator/Utilities/CsvParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,15 @@ public int GetFieldAsInt (int index)
{
return int.Parse (GetField (index));
}

public int? GetFieldAsNullableInt32 (int index)
{
var value = GetField (index);

if (int.TryParse (value, out var val))
return val;

return default;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ public void ParseEnumMapV2 ()
Assert.AreEqual ("Cdsect", entry.EnumMember);
Assert.AreEqual (FieldAction.Keep, entry.FieldAction);
Assert.False (entry.IsFlags);
Assert.IsNull (entry.DeprecatedSince);
}

[Test]
Expand All @@ -99,13 +100,14 @@ public void ParseAddEnumMapV2 ()
Assert.AreEqual ("Org.XmlPull.V1.XmlPullParserNode", entry.EnumFullType);
Assert.AreEqual ("Cdsect", entry.EnumMember);
Assert.AreEqual (FieldAction.None, entry.FieldAction);
Assert.IsNull (entry.DeprecatedSince);
Assert.False (entry.IsFlags);
}

[Test]
public void ParseRemoveEnumMapV2 ()
{
var csv = "R,10,I:org/xmlpull/v1/XmlPullParser.CDSECT,5,,,remove";
var csv = "R,10,I:org/xmlpull/v1/XmlPullParser.CDSECT,5,,,remove,,33";
var entry = ConstantEntry.FromString (csv);

Assert.AreEqual (ConstantAction.Remove, entry.Action);
Expand All @@ -116,6 +118,7 @@ public void ParseRemoveEnumMapV2 ()
Assert.AreEqual (string.Empty, entry.EnumMember);
Assert.AreEqual (FieldAction.Remove, entry.FieldAction);
Assert.False (entry.IsFlags);
Assert.AreEqual (33, entry.DeprecatedSince.Value);
}
}
}
54 changes: 54 additions & 0 deletions tests/generator-Tests/Unit-Tests/EnumGeneratorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,60 @@ public void ObsoleteFieldButNotEnumAttributeSupport ()
Assert.False (writer.ToString ().NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]WithExcluded=1"), writer.ToString ());
}

[Test]
public void ObsoletedOSPlatformAttributeOverrideSupport ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='android.app' jni-name='android/app'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='ActivityManager' static='false' visibility='public' jni-signature='Landroid/app/ActivityManager;'>
<field deprecated='not deprecated' final='true' name='RECENT_IGNORE_UNAVAILABLE' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;android.permission.BIND_CHOOSER_TARGET_SERVICE&quot;' visibility='public' volatile='false' api-since='30' />
</class>
</package>
</api>";

options.UseObsoletedOSPlatformAttributes = true;

var enu = CreateEnum ();
enu.Value.Members.Single (m => m.EnumMember == "WithExcluded").DeprecatedSince = 33;

var gens = ParseApiDefinition (xml);

generator.WriteEnumeration (options, enu, gens.ToArray ());

// The field itself is not deprecated, but [ObsoletedOSPlatform] should be written because we set `DeprecatedSince` on the enum map
Assert.True (writer.ToString ().NormalizeLineEndings ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform(\"android33.0\")]WithExcluded=1"), writer.ToString ());
}

[Test]
public void ObsoleteAccidentalAddition ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='android.app' jni-name='android/app'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='ActivityManager' static='false' visibility='public' jni-signature='Landroid/app/ActivityManager;'>
<field deprecated='not deprecated' final='true' name='RECENT_IGNORE_UNAVAILABLE' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;android.permission.BIND_CHOOSER_TARGET_SERVICE&quot;' visibility='public' volatile='false' api-since='30' />
</class>
</package>
</api>";

options.UseObsoletedOSPlatformAttributes = true;

var enu = CreateEnum ();
enu.Value.Members.Single (m => m.EnumMember == "WithExcluded").DeprecatedSince = -1;

var gens = ParseApiDefinition (xml);

generator.WriteEnumeration (options, enu, gens.ToArray ());

// "-1" is a "magic" API level which adds a message indicating the enum value shouldn't even exist
Assert.True (writer.ToString ().NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"Thisvaluewasincorrectlyaddedtotheenumerationandisnotavalidvalue\")]WithExcluded=1"), writer.ToString ());
}

protected new string GetExpected (string testName)
{
var root = Path.GetDirectoryName (Assembly.GetExecutingAssembly ().Location);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,16 @@ EnumWriter CreateWriter (CodeGenerationOptions opt, KeyValuePair<string, EnumDes

SourceWriterExtensions.AddSupportedOSPlatform (m.Attributes, member.ApiLevel, opt);

// Some of our source fields may have been marked with:
// "This constant will be removed in the future version. Use XXX enum directly instead of this field."
// We don't want this message to propogate to the enum.
if (managedMember != null && managedMember.Value.Field?.DeprecatedComment?.Contains ("enum directly instead of this field") == false)
if (member.DeprecatedSince.HasValue) {
// If we've manually marked it as obsolete in map.csv, that takes precedence
var msg = member.DeprecatedSince.Value == -1 ? "This value was incorrectly added to the enumeration and is not a valid value" : "deprecated";
SourceWriterExtensions.AddObsolete (m.Attributes, msg, opt, deprecatedSince: member.DeprecatedSince);
} else if (managedMember != null && managedMember.Value.Field?.DeprecatedComment?.Contains ("enum directly instead of this field") == false) {
// Some of our source fields may have been marked with:
// "This constant will be removed in the future version. Use XXX enum directly instead of this field."
// We don't want this message to propogate to the enum.
SourceWriterExtensions.AddObsolete (m.Attributes, managedMember.Value.Field.DeprecatedComment, opt, deprecatedSince: managedMember.Value.Field.DeprecatedSince);
}

enoom.Members.Add (m);
}
Expand Down

0 comments on commit 5c5dc08

Please sign in to comment.