Don't return null Jdk locations #157
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VSMac has a first-chance exception because we're returning null here for some attributes
Member
Author
|
My xml looks like this:
|
jonpryor
reviewed
Feb 25, 2022
| foreach (var java_sdk in config.Root.Elements ("java-sdk")) { | ||
| var path = (string) java_sdk.Attribute ("path"); | ||
| yield return path; | ||
| var path = (string) java_sdk.Attribute ("path"); |
Contributor
There was a problem hiding this comment.
Nullable reference types are enabled, and the original code -- as well as this updated code -- is "disabling" NRT by asserting that string and not string? is returned.
Member
Author
There was a problem hiding this comment.
I opted for the minimal surgical fix to solve the problem since I'm in an unfamiliar codebase. I can leave more comprehensive fix options to the maintainers. Thanks!
Contributor
|
@KirillOsenkov: do you have a full stack trace for the VSMac first-chance exception? |
jonpryor
added a commit
to jonpryor/xamarin-android-tools
that referenced
this pull request
Feb 25, 2022
Context: dotnet#157 Context: 2d3690e Commit 2d3690e added Nullable Reference Type support to `Xamarin.Android.Tools.AndroidSdk.dll`. Unfortunately, it was largely "surface level", updating *public* API, but not all method implementations were appropriately updated. Case in point: if `$HOME/.config/xbuild/monodroid-config.xml` contains *no* value in `<java-sdk/>`, e.g. <monodroid> <android-sdk path="/path/to/android/sdk" /> <java-sdk /> <!-- empty! --> </monodroid> then Visual Studio for Mac may report a first-chance exception (only reported when debugging Visual Studio for Mac, as the exception is caught internally): TODO: exception + stack trace A major reason to adopt Nullable Reference Types is to *prevent* the occurrence of `NullReferenceException`s, so what went wrong? What went wrong with 2d3690e is that when XML attributes don't exist, [`XElement.Attribute()`][0] will return `null`, and most of our `XElement.Attribute()` invocations cast the `XAttribute` return value to `string`, "asserting" that a *non-`null`* is returned. Review the codebase for all `XElement.Attribute()` invocations, and update all casts from `(string)` to instead cast to `(string?)`. This ensures that we don't circumvent the C# compilers Nullable Reference Type checks, catches the circumvention which was present in `JdkLocations.GetUnixConfiguredJdkPaths()`, and thus avoids the first-chance exception that VSMac could see. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.xml.linq.xelement.attribute?view=net-6.0
jonpryor
added a commit
to jonpryor/xamarin-android-tools
that referenced
this pull request
Feb 25, 2022
Context: dotnet#157 Context: 2d3690e Commit 2d3690e added Nullable Reference Type support to `Xamarin.Android.Tools.AndroidSdk.dll`. Unfortunately, it was largely "surface level", updating *public* API, but not all method implementations were appropriately updated. Case in point: if `$HOME/.config/xbuild/monodroid-config.xml` contains *no* value in `<java-sdk/>`, e.g. <monodroid> <android-sdk path="/path/to/android/sdk" /> <java-sdk /> <!-- empty! --> </monodroid> then Visual Studio for Mac may report a first-chance exception (only reported when debugging Visual Studio for Mac, as the exception is caught internally): TODO: exception + stack trace A major reason to adopt Nullable Reference Types is to *prevent* the occurrence of `NullReferenceException`s, so what went wrong? What went wrong with 2d3690e is that when XML attributes don't exist, [`XElement.Attribute()`][0] will return `null`, and most of our `XElement.Attribute()` invocations cast the `XAttribute` return value to `string`, "asserting" that a *non-`null`* is returned. Review the codebase for all `XElement.Attribute()` invocations, and update all casts from `(string)` to instead cast to `(string?)`. This ensures that we don't circumvent the C# compilers Nullable Reference Type checks, catches the circumvention which was present in `JdkLocations.GetUnixConfiguredJdkPaths()`, and thus avoids the first-chance exception that VSMac could see. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.xml.linq.xelement.attribute?view=net-6.0 Co-Authored by: @KirillOsenkov
Contributor
|
@KirillOsenkov: i consider this PR to be superseded by PR #158, but I would love to have a version of the first-chance exception that VSMac is seeing for the #158 commit message. |
jonpryor
added a commit
that referenced
this pull request
Feb 25, 2022
Context: #157 Context: 2d3690e Commit 2d3690e added Nullable Reference Type support to `Xamarin.Android.Tools.AndroidSdk.dll`. Unfortunately, it was largely "surface level", updating *public* API, but not all method implementations were appropriately updated. Case in point: if `$HOME/.config/xbuild/monodroid-config.xml` contains *no* value in `<java-sdk/>`, e.g. <monodroid> <android-sdk path="/path/to/android/sdk" /> <java-sdk /> <!-- empty! --> </monodroid> then Visual Studio for Mac may report a first-chance exception (only reported when debugging Visual Studio for Mac, as the exception is caught internally): {System.ArgumentNullException: Value cannot be null. (Parameter 'homePath') at Xamarin.Android.Tools.JdkInfo..ctor(String homePath, String locator, Action`2 logger)} at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.JdkInfo.JdkInfo Line 55 at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.JdkInfo.JdkInfo Line 99 at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.JdkInfo.TryGetJdkInfo Line 347 at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.JdkLocations.GetUnixPreferredJdks.AnonymousMethod__0 Line 14 at System.Linq.dll!System.Linq.Enumerable.SelectEnumerableIterator<string, Xamarin.Android.Tools.JdkInfo>.MoveNext at System.Linq.dll!System.Linq.Enumerable.WhereSelectEnumerableIterator<Xamarin.Android.Tools.JdkInfo, Xamarin.Android.Tools.JdkInfo>.ToArray at System.Linq.dll!System.Linq.Buffer<Xamarin.Android.Tools.JdkInfo>.Buffer at System.Linq.dll!System.Linq.OrderedEnumerable<Xamarin.Android.Tools.JdkInfo>.GetEnumerator at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.JdkLocations.GetPreferredJdks Line 19 at System.Linq.dll!System.Linq.Enumerable.ConcatIterator<Xamarin.Android.Tools.JdkInfo>.MoveNext at System.Linq.dll!System.Linq.Enumerable.SelectEnumerableIterator<Xamarin.Android.Tools.JdkInfo, string>.MoveNext at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.AndroidSdkBase.GetValidPath Line 112 at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.AndroidSdkBase.Initialize Line 69 at Xamarin.Android.Tools.AndroidSdk.Ide.dll!Xamarin.Android.Tools.AndroidSdkInfo.AndroidSdkInfo at Xamarin.AndroidTools.Ide.dll!Xamarin.AndroidTools.AndroidSdk.Refresh Line 53 … A major reason to adopt Nullable Reference Types is to *prevent* the occurrence of `NullReferenceException`s, so what went wrong? What went wrong with 2d3690e is that when XML attributes don't exist, [`XElement.Attribute()`][0] will return `null`, and most of our `XElement.Attribute()` invocations cast the `XAttribute` return value to `string`, "asserting" that a *non-`null`* is returned. Review the codebase for all `XElement.Attribute()` invocations, and update all casts from `(string)` to instead cast to `(string?)`. This ensures that we don't circumvent the C# compilers Nullable Reference Type checks, catches the circumvention which was present in `JdkLocations.GetUnixConfiguredJdkPaths()`, and thus avoids the first-chance exception that VSMac could see. [0]: https://docs.microsoft.com/en-us/dotnet/api/system.xml.linq.xelement.attribute?view=net-6.0 Co-Authored by: @KirillOsenkov
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VSMac has a first-chance exception because we're returning null here for some attributes