Skip to content

Commit f4c44e2

Browse files
jonpryorjonpryor
authored
[Xamarin.Android.Tools.AndroidSdk] Attributes can be null! (#158)
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
1 parent f0b3abd commit f4c44e2

File tree

4 files changed

+32
-24
lines changed

4 files changed

+32
-24
lines changed

‎src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs

+24-19
Original file line numberDiff line numberDiff line change
@@ -136,37 +136,37 @@ public void WriteToFile (string fileName)
136136
}
137137

138138
public string? PackageName {
139-
get { return (string) manifest.Attribute ("package"); }
139+
get { return (string?) manifest.Attribute ("package"); }
140140
set { manifest.SetAttributeValue ("package", NullIfEmpty (value)); }
141141
}
142142

143143
public string? ApplicationLabel {
144-
get { return (string) application.Attribute (aNS + "label"); }
144+
get { return (string?) application.Attribute (aNS + "label"); }
145145
set { application.SetAttributeValue (aNS + "label", NullIfEmpty (value)); }
146146
}
147147

148148
public string? ApplicationIcon {
149-
get { return (string) application.Attribute (aNS + "icon"); }
149+
get { return (string?) application.Attribute (aNS + "icon"); }
150150
set { application.SetAttributeValue (aNS + "icon", NullIfEmpty (value)); }
151151
}
152152

153153
public string? ApplicationTheme {
154-
get { return (string) application.Attribute (aNS + "theme"); }
154+
get { return (string?) application.Attribute (aNS + "theme"); }
155155
set { application.SetAttributeValue (aNS + "theme", NullIfEmpty (value)); }
156156
}
157157

158158
public string? VersionName {
159-
get { return (string) manifest.Attribute (aNS + "versionName"); }
159+
get { return (string?) manifest.Attribute (aNS + "versionName"); }
160160
set { manifest.SetAttributeValue (aNS + "versionName", NullIfEmpty (value)); }
161161
}
162162

163163
public string? VersionCode {
164-
get { return (string) manifest.Attribute (aNS + "versionCode"); }
164+
get { return (string?) manifest.Attribute (aNS + "versionCode"); }
165165
set { manifest.SetAttributeValue (aNS + "versionCode", NullIfEmpty (value)); }
166166
}
167167

168168
public string? InstallLocation {
169-
get { return (string) manifest.Attribute (aNS + "installLocation"); }
169+
get { return (string?) manifest.Attribute (aNS + "installLocation"); }
170170
set { manifest.SetAttributeValue (aNS + "installLocation", NullIfEmpty (value)); }
171171
}
172172

@@ -182,8 +182,8 @@ public int? TargetSdkVersion {
182182

183183
int? ParseSdkVersion (XAttribute attribute)
184184
{
185-
var version = (string)attribute;
186-
if (string.IsNullOrEmpty (version))
185+
var version = (string?) attribute;
186+
if (version == null || string.IsNullOrEmpty (version))
187187
return null;
188188
int vn;
189189
if (!int.TryParse (version, out vn)) {
@@ -198,7 +198,7 @@ public int? TargetSdkVersion {
198198
public IEnumerable<string> AndroidPermissions {
199199
get {
200200
foreach (var el in manifest.Elements ("uses-permission")) {
201-
var name = (string) el.Attribute (aName);
201+
var name = (string?) el.Attribute (aName);
202202
if (name == null)
203203
continue;
204204
var lastDot = name.LastIndexOf ('.');
@@ -211,7 +211,7 @@ public IEnumerable<string> AndroidPermissions {
211211
public IEnumerable<string> AndroidPermissionsQualified {
212212
get {
213213
foreach (var el in manifest.Elements ("uses-permission")) {
214-
var name = (string) el.Attribute (aName);
214+
var name = (string?) el.Attribute (aName);
215215
if (name != null)
216216
yield return name;
217217
}
@@ -267,7 +267,11 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
267267
{
268268
var perms = new HashSet<string> (permissions);
269269
var list = manifest.Elements ("uses-permission")
270-
.Where (el => perms.Contains ((string)el.Attribute (aName))).ToList ();
270+
.Where (el => {
271+
var name = (string?) el.Attribute (aName);
272+
return name != null && perms.Contains (name);
273+
})
274+
.ToList ();
271275
foreach (var el in list)
272276
el.Remove ();
273277
}
@@ -284,7 +288,7 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
284288
{
285289
string? first = null;
286290
foreach (var a in GetLaunchableActivities ()) {
287-
var name = (string) a.Attribute (aName);
291+
var name = (string?) a.Attribute (aName);
288292
//prefer the fastdev launcher, it's quicker
289293
if (name == "mono.android.__FastDevLauncher") {
290294
return name;
@@ -303,7 +307,7 @@ void RemoveAndroidPermissions (IEnumerable<string> permissions)
303307
public string? GetLaunchableUserActivityName ()
304308
{
305309
return GetLaunchableActivities ()
306-
.Select (a => (string) a.Attribute (aName))
310+
.Select (a => (string?) a.Attribute (aName))
307311
.FirstOrDefault (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher");
308312
}
309313

@@ -313,7 +317,7 @@ IEnumerable<XElement> GetLaunchableActivities ()
313317
var filter = activity.Element ("intent-filter");
314318
if (filter != null) {
315319
foreach (var category in filter.Elements ("category"))
316-
if (category != null && (string)category.Attribute (aName) == "android.intent.category.LAUNCHER")
320+
if (category != null && (string?)category.Attribute (aName) == "android.intent.category.LAUNCHER")
317321
yield return activity;
318322
}
319323
}
@@ -322,17 +326,18 @@ IEnumerable<XElement> GetLaunchableActivities ()
322326
public IEnumerable<string> GetAllActivityNames ()
323327
{
324328
foreach (var activity in application.Elements ("activity")) {
325-
var activityName = (string) activity.Attribute (aName);
326-
if (activityName != "mono.android.__FastDevLauncher")
329+
var activityName = (string?) activity.Attribute (aName);
330+
if (activityName != null && activityName != "mono.android.__FastDevLauncher")
327331
yield return activityName;
328332
}
329333
}
330334

331335
public IEnumerable<string> GetLaunchableActivityNames ()
332336
{
333337
return GetLaunchableActivities ()
334-
.Select (a => (string) a.Attribute (aName))
335-
.Where (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher");
338+
.Select (a => (string?) a.Attribute (aName))
339+
.Where (name => !string.IsNullOrEmpty (name) && name != "mono.android.__FastDevLauncher")
340+
.Select (name => name!);
336341
}
337342
}
338343
}

‎src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs

+1
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,7 @@ static Dictionary<string, List<string>> GetJavaProperties (Action<TraceLevel, st
285285
if (e.Data.StartsWith (ContinuedValuePrefix, StringComparison.Ordinal)) {
286286
if (curKey == null) {
287287
logger (TraceLevel.Error, $"No Java property previously seen for continued value `{e.Data}`.");
288+
return;
288289
}
289290
props [curKey].Add (e.Data.Substring (ContinuedValuePrefix.Length));
290291
return;

‎src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs

+4-2
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@ static IEnumerable<string> GetUnixConfiguredJdkPaths (Action<TraceLevel, string>
2121
{
2222
var config = AndroidSdkUnix.GetUnixConfigFile (logger);
2323
foreach (var java_sdk in config.Root.Elements ("java-sdk")) {
24-
var path = (string) java_sdk.Attribute ("path");
25-
yield return path;
24+
var path = (string?) java_sdk.Attribute ("path");
25+
if (path != null && !string.IsNullOrEmpty (path)) {
26+
yield return path;
27+
}
2628
}
2729
}
2830

‎src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs

+3-3
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public override string? PreferedAndroidSdkPath {
4949
var androidEl = config_file.Root.Element ("android-sdk");
5050

5151
if (androidEl != null) {
52-
var path = (string)androidEl.Attribute ("path");
52+
var path = (string?)androidEl.Attribute ("path");
5353

5454
if (ValidateAndroidSdkLocation (path))
5555
return path;
@@ -64,7 +64,7 @@ public override string? PreferedAndroidNdkPath {
6464
var androidEl = config_file.Root.Element ("android-ndk");
6565

6666
if (androidEl != null) {
67-
var path = (string)androidEl.Attribute ("path");
67+
var path = (string?)androidEl.Attribute ("path");
6868

6969
if (ValidateAndroidNdkLocation (path))
7070
return path;
@@ -79,7 +79,7 @@ public override string? PreferedJavaSdkPath {
7979
var javaEl = config_file.Root.Element ("java-sdk");
8080

8181
if (javaEl != null) {
82-
var path = (string)javaEl.Attribute ("path");
82+
var path = (string?)javaEl.Attribute ("path");
8383

8484
if (ValidateJavaSdkLocation (path))
8585
return path;

0 commit comments

Comments
 (0)