[coretext] Add nullability to (generated and manual) bindings #14978

tj-devel709 · 2022-05-10T23:16:36Z

This PR aims to bring nullability changes to CoreText.
Following the steps here:

I am adding nullable enable to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes
Changing all throw new ArgumentNullException ("object")); to ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object)); for size saving optimization as well to mark that this framework contains nullability changes
Changing any == null or != null to is null and is not null

I decided to mark the GetStringConstants that deal with handles with a ! since these GetX (handle) methods tend to be safe with nullability from previous PRs and there were already some files in CoreText following this pattern.
Dlfcn.GetStringConstant (handle, "kCTFontCopyrightNameKey")!;

src/CoreText/CTBaselineClass.cs

src/CoreText/CTFontManager.cs

src/CoreText/CTFontNameKey.cs

src/CoreText/CTFontTrait.cs

src/CoreText/CTStringAttributes.cs

src/CoreText/ConstructorError.cs

tj-devel709 · 2022-05-11T14:44:08Z

@rolfbjarne before I go through all the files and make these changes, I wanted to confirm that I am on the right track with this file? https://gist.github.com/tj-devel709/1ad14fd0c52616dd2bd684c07c57ca6a

vs-mobiletools-engineering-service2 · 2022-05-11T20:39:49Z

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent XAMBOT-1044.Monterey
Merge 094b3f0 into 22ba05b

rolfbjarne · 2022-05-12T08:28:21Z

@rolfbjarne before I go through all the files and make these changes, I wanted to confirm that I am on the right track with this file? gist.github.com/tj-devel709/1ad14fd0c52616dd2bd684c07c57ca6a

Yes, that's on the right track!

I think you can simplify things a bit though:

Make Adapter.GetStringValue return null if the key is null. This should avoid changing numerous getters.
Make Adapter.SetValue throw ArgumentOutOfRangeException if the key is null. This should avoid changing numerous setters.
Maybe add an Adapter.GetObjectValue that checks for null key (and returns null) before getting the entry in the dictionary, this would simplify some getters that don't already call into the Adapter class.
You can't make a struct return type nullable (it's a breaking change) - for lines 465-483 in your diff. That should just return false if there's no entry in the dictionary.

tj-devel709 · 2022-05-12T18:46:24Z

@rolfbjarne for the first two suggestions, if I instead add the null checks in the Adapter.GetStringValue and Adapter.SetValue methods, I believe I will need to either change those signatures to say that key can be null and then throw an exception when it is null (which I think is probably not the right way) or put an exclamation for the key argument when calling those methods. Were either of these options what you were intending?

rolfbjarne · 2022-05-13T08:58:06Z

@tj-devel709 for the first suggestion, you can change the Adapter.GetStringValue to accept a null key (in which case you can return null).

For the second suggestion, I'd put an exclamation on all the callsites to Adapter.SetValue

It's a bit weird that the Get and Set methods have different semantics, but these are internal methods, so it doesn't matter all that much.

tj-devel709 · 2022-05-16T23:02:16Z

Apologies, did not mean to push yet, not ready to be merged!

tj-devel709 · 2022-05-17T19:53:03Z

@rolfbjarne I think I made all the changes. I wasn't quite sure of where to do the following however:

3. Maybe add an Adapter.GetObjectValue that checks for null key (and returns null) before getting the entry in the dictionary, this would simplify some getters that don't already call into the Adapter class.

vs-mobiletools-engineering-service2 · 2022-06-09T18:00:08Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

❗ API diff vs stable (Breaking changes)

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: eb544046d6de1103f2efadc6a02913a6fa063a19 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-13T20:15:56Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 5f199c3f6de52106fd6cfaf39a8e12024505c313 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-14T23:43:26Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Tests on macOS M1 - Mac Big Sur (11.5) failed for unknown reasons.

Pipeline on Agent
Hash: 1726846e601c57c879d6bd797607f291beb5edf9 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T00:01:24Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 1726846e601c57c879d6bd797607f291beb5edf9 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T00:03:01Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 1726846e601c57c879d6bd797607f291beb5edf9 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T00:10:21Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1103.Monterey'
Hash: 1726846e601c57c879d6bd797607f291beb5edf9 [PR build]

vs-mobiletools-engineering-service2 · 2022-06-15T06:14:22Z

✅ [CI Build] Tests passed on VSTS: simulator tests iOS. ✅

Tests passed on VSTS: simulator tests iOS.

🎉 All 148 tests passed 🎉

Pipeline on Agent XAMBOT-1108.Monterey'
Merge 1726846 into f2d0a16

TJ Lambert added 3 commits May 10, 2022 17:31

Add nullability

93299b7

throw better exceptions

fdfe8c5

use is null

ced66d5

tj-devel709 added the not-notes-worthy Ignore for release notes label May 10, 2022

tj-devel709 added this to the Future milestone May 10, 2022

This comment has been minimized.

Sign in to view

rolfbjarne requested changes May 11, 2022

View reviewed changes

mandel-macaque requested changes May 11, 2022

View reviewed changes

src/CoreText/ConstructorError.cs Outdated Show resolved Hide resolved

TJ Lambert added 2 commits May 11, 2022 07:26

revert a new throw

094b3f0

remove unused ConstructionError Exception

cd3d510

This comment has been minimized.

Sign in to view

started the nullable gethandle changes

8e02b16

TJ Lambert added 2 commits May 16, 2022 17:56

Update the nullable NSStrings

15f361a

Merge branch 'Nullable-CoreText1' into Nullable-CoreText

917c825

tj-devel709 added the do-not-merge Do not merge this pull request label May 16, 2022

This comment has been minimized.

Sign in to view

revert the bool property nullability and other small fix

111b743

tj-devel709 removed the do-not-merge Do not merge this pull request label May 17, 2022

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-CoreText

99a268b

mandel-macaque approved these changes Jun 6, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-CoreText

eb54404

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-CoreText

5f199c3

This comment has been minimized.

Sign in to view

Merge branch 'main' into Nullable-CoreText

1726846

tj-devel709 merged commit 3e6fdd8 into xamarin:main Jun 15, 2022

[coretext] Add nullability to (generated and manual) bindings #14978

[coretext] Add nullability to (generated and manual) bindings #14978

Conversation

tj-devel709 commented May 10, 2022

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.

tj-devel709 commented May 11, 2022 • edited Loading

vs-mobiletools-engineering-service2 commented May 11, 2022

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

rolfbjarne commented May 12, 2022

tj-devel709 commented May 12, 2022

rolfbjarne commented May 13, 2022 • edited Loading

tj-devel709 commented May 16, 2022

This comment has been minimized.

This comment has been minimized.

tj-devel709 commented May 17, 2022

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.

vs-mobiletools-engineering-service2 commented Jun 9, 2022

✅ API diff for current PR / commit

❗ API diff vs stable (Breaking changes)

ℹ️ Generator diff

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.

vs-mobiletools-engineering-service2 commented Jun 13, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Jun 14, 2022

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

vs-mobiletools-engineering-service2 commented Jun 15, 2022

✅ API diff for current PR / commit

✅ API diff vs stable

✅ Generator diff

vs-mobiletools-engineering-service2 commented Jun 15, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

vs-mobiletools-engineering-service2 commented Jun 15, 2022

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Jun 15, 2022

✅ [CI Build] Tests passed on VSTS: simulator tests iOS. ✅

🎉 All 148 tests passed 🎉

tj-devel709 commented May 11, 2022 •

edited

Loading

rolfbjarne commented May 13, 2022 •

edited

Loading