Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullability annotations on NSUrl #15356

Merged
merged 8 commits into from
Aug 19, 2022
Merged

Conversation

Therzok
Copy link
Contributor

@Therzok Therzok commented Jun 27, 2022

No description provided.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@mandel-macaque mandel-macaque added enhancement The issue or pull request is an enhancement not-notes-worthy Ignore for release notes labels Jun 27, 2022
@mandel-macaque
Copy link
Member

@Therzok can you fix all the following warnings?

* Assertion at mono-threads.c:651, condition `info' not met, function:mono_thread_info_current, 
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.

@tj-devel709 give a hand if it is needed since you have experience with this changes.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming test fixes are done (my GH refresh was stale when I reviewed)

@Therzok
Copy link
Contributor Author

Therzok commented Jun 28, 2022

@Therzok can you fix all the following warnings?

* Assertion at mono-threads.c:651, condition `info' not met, function:mono_thread_info_current, 
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(716,24): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrlSessionHandler.cs(754,56): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString)'.
Foundation/NSUrl.cs(58,21): warning CS8604: Possible null reference argument for parameter 'uriString' in 'Uri.Uri(string uriString, UriKind uriKind)'.

@tj-devel709 give a hand if it is needed since you have experience with this changes.

Sure. I'm not sure about the assertion though, is it crashing on a null being passed?

@rolfbjarne
Copy link
Member

The assertion is probably csc triggering a Mono/CoreCLR bug (causing mono or coreclr to crash) - in other words not a problem with this PR, so you can ignore it.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

src/Foundation/NSUrl.cs Outdated Show resolved Hide resolved
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@mandel-macaque
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@Therzok Therzok force-pushed the dev/therzok/nsurl-nullability branch from ac7312c to c0c4d5c Compare July 10, 2022 20:22
@@ -49,13 +49,14 @@ public bool Equals (NSUrl? url)
// Converts from an NSURL to a System.Uri
public static implicit operator Uri? (NSUrl? url)
{
if (url is null) {
if (url?.AbsoluteString is not string absoluteUrl) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confused me, as it tried to create an Uri with a null string.

{
var uri = new Uri (url.AbsoluteString);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved marshalling to caller.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@Therzok
Copy link
Contributor Author

Therzok commented Aug 5, 2022

Anything else needed on this PR?

@vs-mobiletools-engineering-service2
Copy link
Collaborator

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

No test package could be found for tests on macOS M1 - Mac Big Sur (11.5)

Pipeline on Agent
Hash: 3be88f5bdb81e14e0af1a50bc4f92d9d51fc593c [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS Mac Catalina (10.15) failed ❌

No test package could be found for tests on macOS Mac Catalina (10.15)

Pipeline on Agent
Hash: 3be88f5bdb81e14e0af1a50bc4f92d9d51fc593c [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1043.Monterey'
Hash: 3be88f5bdb81e14e0af1a50bc4f92d9d51fc593c [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ 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)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 3be88f5bdb81e14e0af1a50bc4f92d9d51fc593c [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

🎉 All 172 tests passed 🎉

Failures

❌ mononative tests

🔥 Failed catastrophically on VSTS: simulator tests - mononative (no summary found).

Html Report (VSDrops) Download

❌ monotouch tests

🔥 Failed catastrophically on VSTS: simulator tests - monotouch (no summary found).

Html Report (VSDrops) Download

❌ msbuild tests

🔥 Failed catastrophically on VSTS: simulator tests - msbuild (no summary found).

Html Report (VSDrops) Download

❌ mtouch tests

🔥 Failed catastrophically on VSTS: simulator tests - mtouch (no summary found).

Html Report (VSDrops) Download

❌ xammac tests

🔥 Failed catastrophically on VSTS: simulator tests - xammac (no summary found).

Html Report (VSDrops) Download

❌ xcframework tests

🔥 Failed catastrophically on VSTS: simulator tests - xcframework (no summary found).

Html Report (VSDrops) Download

❌ xtro tests

🔥 Failed catastrophically on VSTS: simulator tests - xtro (no summary found).

Html Report (VSDrops) Download

Successes

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 223 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

@rolfbjarne rolfbjarne merged commit 4b86732 into main Aug 19, 2022
@rolfbjarne rolfbjarne deleted the dev/therzok/nsurl-nullability branch August 19, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue or pull request is an enhancement not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants