[NET Attribute Conversion][generator] Generate NET style attributes #14779
Conversation
- The integration with the existing generator code is not ideal in this commit, but given time pressures it will hopefully be improved in the future. - GetPlatformAttributesToPrint is repeating some of the logic that is #if !NET'ed out in places such as PrintPlatformAttributes because those checks were not robust enough for the NET6 attribute logic (we generate many more 'duplicate' attributes now)
…remove now unnecessary ignores on other tests
- This is an actual API inclusion, not just attribute change. - All of the other defines, and attributes imply that GetCurrentInputDevice() was expected to be on catalyst, but the define on 433 did not include it
chamons
requested review from
dalexsoto,
rolfbjarne and
mandel-macaque
as code owners
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -189,12 +189,20 @@ public enum AudioObjectPropertySelector : uint | |||
| TranslateUIDToBox = 1969841250, // 'uidb' | |||
| ClockDeviceList = 1668049699, //'clk#' | |||
| TranslateUidToClockDevice = 1969841251, // 'uidc', | |||
| #if XAMCORE_3_0 | |||
There was a problem hiding this comment.
The defines in this enum are somewhat confusing.
I think this would simplify it:
[NoTV][NoWatch]
#if NET
[NoiOS]
#endif
public enum AudioObjectPropertySelector : uint
{
...
#if !NET
[Deprecated (PlatformName.iOS, 15,0, message : "Use the 'ProcessIsMain' element instead.")]
#endif
[Deprecated (PlatformName.MacCatalyst, 15,0, message : "Use the 'ProcessIsMain' element instead.")]
[Deprecated (PlatformName.MacOSX, 12,0, message : "Use the 'ProcessIsMain' element instead.")]
[Obsolete ("Use the 'ProcessIsMain' element instead.")]
ProcessIsMaster = 1835103092, // 'mast'
[MacCatalyst (15,0), Mac (12,0)]
ProcessIsMain = 1835100526, // 'main'
...
[MacCatalyst (15,0), Mac (12,0)]
ProcessMute = 1634758765, // 'appm'
}That said, the changes here look correct, so this could go in another PR.
There was a problem hiding this comment.
Like it, let's save it for a follow up PR.
| @@ -74,6 +77,7 @@ interface CNContactPickerDelegate | |||
| void DidClose (CNContactPicker picker); | |||
| } | |||
| #else | |||
| [NoMac] | |||
There was a problem hiding this comment.
Any reason you didn't merge the CNContactPickerDelegate definitions like you did for CNContactViewController?
There was a problem hiding this comment.
This was just found later and I started running into more time pressure to get this done.
tests/cecil-tests/AttributeTest.cs
Outdated
| #if DEBUG | ||
| Console.WriteLine(assemblyPath); | ||
| #endif | ||
| if (!assemblyPath.Contains("Xamarin.Mac.dll")) { |
There was a problem hiding this comment.
There's no "Xamarin.Mac.dll" anymore for .NET (it's "Microsoft.macOS.dll"), so this seems wrong.
There was a problem hiding this comment.
This was a cludge to only run one test more quickly (from back before the rename). 🤦
Let me remove and re-run.
There was a problem hiding this comment.
So, this uncovered a few test failures.... looking into those now.
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@rolfbjarne - Requesting re-review just because the last commit was large (fixing the issue you found). |
This comment has been minimized.
This comment has been minimized.
src/generator-attributes.cs
Outdated
| @@ -931,19 +925,61 @@ public override string ToString () | |||
| builder.AppendLine ("#if __MACOS__"); | |||
| break; | |||
| case PlatformName.MacCatalyst: | |||
| builder.AppendLine ("#if __MACCATALYST__"); | |||
| builder.AppendLine ("#if __MACCATALYST__ && !__IOS__ && !__MACOS__"); | |||
There was a problem hiding this comment.
The && !__MACOS__ part shouldn't be needed, but it doesn't hurt either.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Generator test failure looks legit somehow, fixing (Introspection passes locally for me). |
|
Looks like the generator test actually caught a logic bug in the generator change. In NET6 I assume that all namespaces must be one of the Apple binding one, else we must be unsupported and thus skip attributes. This completely ignored 3rd party bindings. |
| @@ -832,7 +832,7 @@ public enum MTLFeatureSet : ulong { | |||
| [iOS (9,0)][Mac (10,11)] | |||
| [Native] | |||
| public enum MTLLanguageVersion : ulong { | |||
| [NoMac] | |||
| [NoMac][NoMacCatalyst] | |||
There was a problem hiding this comment.
Do we care about consistence between [NoMac][NoMacCatalyst] and the previous usage of [...NoMac, NoMacCatalyst]?
There was a problem hiding this comment.
No. We are absurdly inconsistent.
| @@ -931,19 +925,61 @@ public override string ToString () | |||
| builder.AppendLine ("#if __MACOS__"); | |||
| break; | |||
| case PlatformName.MacCatalyst: | |||
| builder.AppendLine ("#if __MACCATALYST__"); | |||
| builder.AppendLine ("#if __MACCATALYST__ && !__IOS__"); | |||
There was a problem hiding this comment.
I don't know the overall usage - is it guaranteed that the StringBuilder is at end-of-line at this point?
There was a problem hiding this comment.
Pretty much, though invariants are tough to pin down in the generator since it's older code with few tests.
There was a problem hiding this comment.
OK - for the future:
static class StringBuilderExtensions {
public static StringBuilder AppendLineIfNeeded (this StringBuilder sb)
{
if (sb.Length == 0) return sb;
return sb [sb.Length - 1] == '\n' ? sb : sb.Append ('\n');
}
public static StringBuilder AppendLineIfNeeded (this StringBuilder sb, string text)
=> return sb.AppendLineIfNeeded().Append(text);
}
| } | ||
| } | ||
|
|
||
| HashSet<string> GetFrameworkListForPlatform (PlatformName platform) |
There was a problem hiding this comment.
For the future, this type of code can be simplified with a switch expression:
var frameworkList = platform switch {
PlatformName.iOS => Frameworks.iosframeworks,
PlatformName.TvOS => Frameworks.tvosframeworks,
PlatformName.MacOSX => Frameworks.macosframeworks,
PlatformName.MacCatalyst => Frameworks.maccatalstframeworks,
_ => new HashSet<string>(),
};
return new HashSet<string>(frameworkList.Select(x => x.ToLower (CultureInfo.InvariantCulture)));
| default: | ||
| throw new NotImplementedException ($"FindNamespace on {item} of type {item.GetType()}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
same here:
static string FindNamespace (MemberInfo item) => item switch {
TypeInfo type => type.Namespace,
PropertyInfo prop => prop.DeclaringType.Namespace,
MethodInfo meth => meth.DeclaringType.Namespace,
_ => throw new NotImplementedException ("..."),
}
Also - should this have a case for events?
There was a problem hiding this comment.
Events are never (?) in bindings, I'd rather crash and find out than add it and hope it works.
…mespaceNotIncluded always assuming we're core
| @@ -7596,17 +7596,17 @@ interface AVFragmentedMovie_AVFragmentedMovieTrackInspection | |||
| AVFragmentedMovieTrack[] GetTracks (AVMediaCharacteristics mediaCharacteristic); | |||
|
|
|||
| [Async] | |||
| [Watch (8,0), TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)] | |||
There was a problem hiding this comment.
Does this mean is present in the default min version? What am I missing?
There was a problem hiding this comment.
The type itself is marked with NoTV, and my tests screamed about the inconsistency.
|
Generator test should be fixed. |
| static bool IgnoreElementsThatDoNotExistInThatAssembly (string member) | ||
| { | ||
| // Xkit has many platform specific bits in AppKit/UIKit and is a mess to get right | ||
| if (member.StartsWith ("Kit")) { |
There was a problem hiding this comment.
All this can be a lot nicer with a switch and since we have the new c# we could put all in the same switch.
|
@mandel-macaque - I fully the switches in AttributesTest.cs are absurd, but they should be temporary until #14802 is fixed. |
📋 [PR Build] API Diff 📋API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffAPI diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffGenerator diffℹ️ Generator Diff (please review changes) Pipeline on Agent XAMBOT-1109.Monterey' |
❌ [PR Build] Tests on macOS Mac Catalina (10.15) failed ❌Failed tests are:
Pipeline on Agent |
❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌Failed tests are:
Pipeline on Agent |
|
/sudo backport release/6.0.3xx |
|
Backport Job to branch release/6.0.3xx Created! The magic is happening here |
|
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6103169 for more details. |
🤞 the final large piece of the saga that is #10170
We won't know until landing this and testing the result but it's the last known hole.....
Summary
This PR teaches our code generator to generate .NET 6 style availability attributes, adds a 4th Cecil test to verify our generated attributes, and a metric ton of API changes to satisfy that test.
Details
The generator work is the core of this PR, and includes:
FilterMinimumVersionandPrintPlatformAttributesIs64BitiOSOnly.GetPlatformAttributesToPrint, which synthesizes many attributes "out of thing air" from:PrintPlatformAttributesNoDuplicatesandGeneratePropertybecause the existing PrintPlatformAttributes did not pass down parent context down, and the refactor was dangerous/too time consuming given time pressure.There are two intended API changes introduced by the reviews in this PR:
GetCurrentInputDevicewas obviously intended by availability attributes to exist on Catalyst but due to define confusion was excluded. It is an addition inMicrosoft.MacCatalyst.dllonly.NEAppRuleconstructors were mis-marked on platforms, and were showing up incorrectly on Mac/Catalyst. I corrected the Catalyst one unconditionally, as we have not shipped Catalyst yet, but Mac is only fixed in NET6.Known API changes
Limitations
I could spend another week or two polishing this, at minimum. It is being landed in this state due to time pressure.
There are bugs in the generated attributes:
Each of these has cases hit that are Ignored in test code (see
IgnoreElementsThatDoNotExistInThatAssembly).Other things to note (and forgive) include:
#ifhacks in generator.cs, which are signs that my hacked in code did not gel well with the existing logic. I should in theory extend each check to better handle the NET6 availability attribute reality and then remove logic from GetPlatformAttributesToPrint.Despite this, given time pressure and test coverage I think it is ready to go in.
I've filed #14802 to track the cleanup work from this PR.