[generator] Allow a constant map to explicitly set `deprecated-since` for an enum value. #1070

jpobst · 2023-01-05T15:47:31Z

Through some auditing, we've found that some constants have been added to the wrong enums.

Example:

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 the value will not function. Thus, setting deprecated-since to -1 will result in:

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

… for an enum value.

jonpryor · 2023-01-05T20:58:40Z

src/Java.Interop.Tools.Generator/Utilities/CsvParser.cs

@@ -23,5 +23,15 @@ public int GetFieldAsInt (int index)
 		{
 			return int.Parse (GetField (index));
 		}
+
+		public int? GetFieldAsIntNullable (int index)


Naming: I would prefer GetFieldAsNullableInt32().

jonpryor · 2023-01-05T21:11:25Z

"Meta"-wise, two thoughts:

The FromEnumMapCsv() parser supports comments. We may want to start adding a "header" to our .csv files to name what all these columns are:
```
// Action, API added, JNI member, enum value, C# enum type, C# member name, …
// http://doc-url/…
```
Is [ObsoletedOSPlatform ("android30.0")] what we actually want? The problem is that the enum entry is entireyl wrong and (ideally) wouldn't be there. The semantics don't seem right to me.

I think [Obsolete("Added accidentally")] would make more sense.

However, there likely will be instances where an enum field should use [ObsoletedOSPlatform], just not "accidentally added" values.

This suggests one of two things:

The new column should be a string, not an int, and "parsed further", e.g. with values Accident for [Obsolete] and Deprecate-33 for [ObsoletedOSPlatform("android33.0")], or
"Encode" the "Accident" value into an int, e.g. -1 or 0 is "Accident, use [Obsolete]" and any other value is [ObsoletedOSPlatform(…)].

My normal preference is toward the "more verbose" (1), but (2) may be preferable.

jpobst · 2023-01-05T22:03:31Z

We may want to start adding a "header" to our .csv files to name what all these columns are:

Added to the output function, since our .csv file is regenerated during enumification.

"Encode" the "Accident" value into an int, e.g. -1 or 0 is "Accident, use [Obsolete]" and any other value is [ObsoletedOSPlatform(…)].

-1 was chosen to produce the message: This value was incorrectly added to the enumeration and is not a valid value. It will automatically be emitted as [Obsolete] because it is lower than our minimum API level of 21.

jonpryor · 2023-01-05T23:54:10Z

@jpobst wrote:

[-1] will automatically be emitted as [Obsolete] because it is lower than our minimum API level of 21.

While "cute" and efficient, i wonder if this could bite us in ~10+ years when our minimum supported API level is 30 (which is the API level frequently used in dotnet/android#7670).

Which makes for an interesting side thought: by having a "sliding window" in which members can move from [ObsoletedOSPlatform] to [Obsolete] because their API level became unsupported, does this mean that potentially existing code which uses OperatingSystem.IsAndroidVersionAtLeast() may subtly and inadvertently break? Could it break?

For the sake of argument, assume that in .NET 8 our minimum API level becomes API-22, up from API-21.

Further assume that there is existing .NET 7 code which does:

if (!OperatingSystem.IsAndroidVersionAtLeast(23)) {
    // Resources.GetDrawable(int) is Obsoleted in API-22
    var d = Android.Content.Res.Resources.System.GetDrawable(42);
}

would there be any implications if/when [ObsoletedOSPlatform] is replaced by [Obsolete]?

Perhaps not, but I do not entirely understand how OperatingSystem.IsAndroidVersionAtLeast() works with [SupportedOSPlatform] and [ObsoletedOSPlatform]…. I think it does warrant some thought, maybe?

jpobst · 2023-01-06T00:24:46Z

For your example, classic [Obsolete] is not aware of IsAndroidVersionAtLeast (...), so the user would begin getting a CS0612: GetDrawable is obsolete warning.

A few potential solutions we could consider:

This is an "off-by-one" issue, and we do not change API-22 to [Obsolete] until API-23 is our minimum. In this scenario, this warning highlights dead code because !OperatingSystem.IsAndroidVersionAtLeast(23) would never be true.
Never move the sliding window and always leave it at "21". In this scenario, the code would not produce a new warning, and the dead code would remain.

Note that the "sliding window" is an existing feature and not something this PR is adding. 😁

https://github.com/xamarin/java.interop/blob/main/tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs#L326

jonpryor · 2023-01-06T01:29:15Z

@jpobst: hilariously, the more I think about this, the more confused I get. :-)

So on the one hand, the nice thing about the "sliding window" is it means that we have a "reasonable mechanism" to stop processing older API levels. Consider:

Currently api-merge is used to process API-10 through API-33, because of the contents of src/Mono.Android/Profiles. However, if API-21 is the minimum, then why do we need to process API-10 through API-20, which requires downloading android.jar for API-10 through API-20! We could save 500MB+ of data if we didn't need to install those!

"Sliding window" means we have (part of) a reasonable mechanism to say that API-21 is our baseline, and stop downloading everything prior to API-21, and slowly increment that minimum over time.

…on the other hand, IIRC (many years ago!) there were API breaks in android.jar (?!) wherein API was removed between say API-10 and API-15. Example workflow; run:

git diff --no-index  bin/BuildDebug/api/api-10.xml.in bin/BuildDebug/api/api-15.xml.in

then within less search for ^-.*<method, looking for <method/> elements which were removed but don't have a corresponding "identical" +.*<method with deprecated set.

One of the matches is Activity.getInstanceCount(), bound as Activity.InstanceCount, but is not present in current android.app.Activity docs!. Ditto Android.App.Activity.SetPersistent().

Meaning if we took "full advantage" of our sliding window, we would remove API! API which aren't currently [Obsolete]!

Which means this is yet another issue. :-)

jonpryor · 2023-01-06T01:35:13Z

There are thus two questions on my mind right now:

What do we do with this PR?
What else should we put on our TODO list?

Regarding (1), I'm inclined to go forward with this PR "conceptually", with a few minor tweeks. I'll try to write up a commit message to summarize things.

Regarding (2), you're already auditing parts of the Mono.Android.dll binding. Now we have another thing to audit: what members have been removed, and what should we do about them? I think we should have api-merge check to see if a member was removed between API X and API X+1, and if it was removed set either @deprecated-since or a new @removed-since attribute. Ideally for .NET 8 we can mark these "removed" methods as [Obsolete], and maybe for .NET 9 we can mark them as [Obsolete(…, error:true)] (?), such that by .NET 10 we could (maybe?) remove them entirely, which would allow us to use our "sliding deprecation window" to stop processing API levels below our minimum supported API level.

…but we need to see what havoc that actually does, and what we can do to minimize harm.

…or we just never move the sliding window, leave it forever at 21, and continue processing API-10…API-20 for all time.

jonpryor · 2023-01-06T01:39:48Z

@jpobst: please update the docs (we have docs?!) to mention this new column: https://github.com/xamarin/java.interop/blob/f8d77faf55347a58030a694332ba97f0dee88246/Documentation/EnumMappingFile.md

jonpryor · 2023-01-06T01:50:52Z

Draft commit message:

Context: https://github.com/xamarin/xamarin-android/pull/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

jpobst · 2023-01-06T02:11:27Z

However, if API-21 is the minimum, then why do we need to process API-10 through API-20, which requires downloading android.jar for API-10 through API-20! We could save 500MB+ of data if we didn't need to install those!

My eventual plan for this was to commit a baseline .xml to git which could be even newer like API-31. These ~never change so there is no reason to recalculate it on every build. We would still have a manual process we could run if we wanted to look for updates.

I think we should have api-merge check to see if a member was removed between API X and API X+1, and if it was removed set either @deprecated-since or a new @removed-since attribute.

This also seems to make sense. I dabbled in something similar before, but never followed through: dotnet/android#5172.

Another thought is dotnet/android#4278 might surface missing API?

jonpryor · 2023-01-06T13:12:27Z

src/Java.Interop.Tools.Generator/Enumification/ConstantsParser.cs

+				"Deprecated Since",
+			};
+
+			writer.WriteLine (string.Join (",", column_names));


Doesn't this need to include the comment // start?

Changes: dotnet/java-interop@f8d77fa...cf80deb * dotnet/java-interop@cf80deb7: [Java.Interop.Tools.JavaCallableWrappers] use IMetadataResolver more (dotnet/java-interop#1069) * dotnet/java-interop@5c5dc086: [generator] enum map.csv can set `@deprecated-since` for enum values (dotnet/java-interop#1070) Any place we used: * `TypeDefinitionCache?` * `IMetadataResolver?` As of dotnet/java-interop@cf80deb7, Java.Interop no longer allows `null` values for these parameters, so overloads which were] `[Obsolete]` now emit errors instead of warnings: [Obsolete ("Use the TypeDefinitionCache overload for better performance.", error: true)] public static MethodDefinition GetBaseDefinition (this MethodDefinition method) => GetBaseDefinition (method, resolver: null!); This results in 3 compiler errors in xamarin-android: src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(65,11): error CS0619: 'TypeDefinitionRocks.IsSubclassOf(TypeDefinition, string)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.' src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(268,12): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.' src\Xamarin.Android.Build.Tasks\Utilities\ManifestDocumentElement.cs(28,11): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.' After these changes, it appears we will improve the performance further of apps that use: * `ApplicationAttribute.BackupAgent` * `ApplicationAttribute.Name` * `AndroidManifest.xml` attributes that take a `System.Type` values Additionally, fix a `NullReferenceException` in the linker: Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetBaseType(TypeDefinition type, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 21 at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetTypeAndBaseTypes(TypeDefinition type, IMetadataResolver resolver)+MoveNext() in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 36 at Java.Interop.Tools.Cecil.TypeDefinitionRocks.IsSubclassOf(TypeDefinition type, String typeName, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 87 at MonoDroid.Tuner.FixAbstractMethodsStep.ProcessType(TypeDefinition type) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs:line 81 at Mono.Linker.Steps.MarkStep.MarkType(TypeReference reference, DependencyInfo reason, Nullable`1 origin) at Mono.Linker.Steps.MarkStep.MarkField(FieldDefinition field, DependencyInfo& reason, MessageOrigin& origin) at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason) at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly) at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason) at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason) at Mono.Linker.Steps.MarkStep.ProcessMarkedPending() at Mono.Linker.Steps.MarkStep.Initialize() at Mono.Linker.Steps.MarkStep.Process(LinkContext context) at Mono.Linker.Pipeline.Process(LinkContext context) at Mono.Linker.Driver.Run(ILogger customLogger) at Mono.Linker.Driver.Main(String[] args) This was caused by removing `null` checks in Java.Interop. Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

[generator] Allow a constant map to explicitly set deprecated-since…

38f9698

… for an enum value.

jpobst marked this pull request as ready for review January 5, 2023 16:20

jonpryor reviewed Jan 5, 2023

View reviewed changes

Address feedback.

715282e

Update EnumMappingFile documentation.

3ac5df9

jonpryor reviewed Jan 6, 2023

View reviewed changes

Update ConstantsParser.cs

c572242

jonpryor merged commit 5c5dc08 into main Jan 7, 2023

jonpryor deleted the enum-deprecated-since branch January 7, 2023 02:57

github-actions bot locked and limited conversation to collaborators Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[generator] Allow a constant map to explicitly set `deprecated-since` for an enum value. #1070

[generator] Allow a constant map to explicitly set `deprecated-since` for an enum value. #1070

jpobst commented Jan 5, 2023 •

edited

Loading

jonpryor Jan 5, 2023

jonpryor commented Jan 5, 2023

jpobst commented Jan 5, 2023

jonpryor commented Jan 5, 2023

jpobst commented Jan 6, 2023 •

edited

Loading

jonpryor commented Jan 6, 2023

jonpryor commented Jan 6, 2023 •

edited

Loading

jonpryor commented Jan 6, 2023

jonpryor commented Jan 6, 2023

jpobst commented Jan 6, 2023 •

edited

Loading

jonpryor Jan 6, 2023

[generator] Allow a constant map to explicitly set deprecated-since for an enum value. #1070

[generator] Allow a constant map to explicitly set deprecated-since for an enum value. #1070

Conversation

jpobst commented Jan 5, 2023 • edited Loading

jonpryor Jan 5, 2023

Choose a reason for hiding this comment

jonpryor commented Jan 5, 2023

jpobst commented Jan 5, 2023

jonpryor commented Jan 5, 2023

jpobst commented Jan 6, 2023 • edited Loading

jonpryor commented Jan 6, 2023

jonpryor commented Jan 6, 2023 • edited Loading

jonpryor commented Jan 6, 2023

jonpryor commented Jan 6, 2023

jpobst commented Jan 6, 2023 • edited Loading

jonpryor Jan 6, 2023

Choose a reason for hiding this comment

[generator] Allow a constant map to explicitly set `deprecated-since` for an enum value. #1070

[generator] Allow a constant map to explicitly set `deprecated-since` for an enum value. #1070

jpobst commented Jan 5, 2023 •

edited

Loading

jpobst commented Jan 6, 2023 •

edited

Loading

jonpryor commented Jan 6, 2023 •

edited

Loading

jpobst commented Jan 6, 2023 •

edited

Loading