xamarin / xamarin-macios

.NET for iOS, Mac Catalyst, macOS, and tvOS provide open-source bindings of the Apple SDKs for use with .NET managed languages such as C#
Other
2.44k stars 509 forks source link

Xamarin.iOS compiled with assembly-wide "Nullable Reference Types" despite missing nullability annotations? #12981

Open ArcanoxDragon opened 2 years ago

ArcanoxDragon commented 2 years ago

I'm having some issues with blatantly wrong nullability warnings for reference types in recent versions of Xamarin.iOS, and while searching for existing issues filed for the matter, I was surprised to find #6154 as an open issue, even though the Xamarin.iOS assembly seems to already be compiled with "Nullable Reference Types" enabled. ildasm.exe shows System.Runtime.CompilerServices attributes related to nullable reference types (such as NullableContextAttribute) being emitted for types that don't have #nullable enable in their source code (such as UIKit.UIViewController and LocalAuthentication.LAContext) in a way that is convincing tools such as ReSharper and Rider that certain properties and parameters are never-null even when they can be (seemingly because the source code has never received nullability attribution, but the "Nullable Reference Types" feature is being enabled assembly-wide somewhere?). Here is an example of such an occurrence:

image The warning squiggle under error is null is a warning from ReSharper saying "The expression is always false". Contrary to the code analysis warning, the Error was null message is actually written to my application log every time the containing method is called, because error is null for all successful queries.

There are currently two closed ReSharper/Rider issues (here and here) related to the same issue that the most recent comments on #6154 mention (from @rpendleton (link) and @bdurrer (link)), the former comment being over a year ago now, and the latter being 9 months ago. There doesn't seem to have been any conversation since then about this problem, but those two ReSharper/Rider issues were been closed (a while ago) by the JetBrains team, as they determined that it was a Xamarin problem and not a problem with ReSharper or Rider.

Some of my Xamarin.iOS project's code is actually being labeled as "heuristically unreachable" by ReSharper now because the code analysis engine is absolutely convinced that things like NSError parameters are definitely not null when they most certainly will be null in the case of successful calls. I am now having to put // ReSharper disable XyzInspection comments all over my iOS code to suppress the incorrect warnings.

I'm kind of perplexed why Visual Studio doesn't raise a warning. When decompiling Xamarin.iOS.dll with JetBrains dotPeek, it does indeed decompile with a #nullable enable line at the top of the source file (confirming what I saw regarding the NullableContextAttribute), but, for instance, the NSError out-parameter on LAContext.CanEvaluatePolicy does not have any sort of attribution indicating that it can be null. Perhaps ReSharper is simply more strict with its interpretation of nullability constraints than Visual Studio is, but JetBrains is certainly convinced that it is at least correct in its interpretation.

The aforementioned issue, #6154, has the ".NET 6.0" milestone assigned, but this seems like an invasive enough problem (if it truly is an issue with the Xamarin.iOS.dll assembly, and JetBrains is correct in claiming that ReSharper/Rider are not at fault) that I feel it should be patched in prior releases if possible. As the comments on #6154 mentioned, it is causing some teams' CI processes to fail now if they use the ReSharper command line tools to look for code quality warnings.

Steps to Reproduce

  1. In an instance of Visual Studio 2019 with ReSharper 2021.2 or newer installed, create a new Xamarin.Forms project with a platform target for at least iOS
  2. Somewhere in the iOS-specific project, such as AppDelegate.cs, add the following code (and required imports):
    
    using var laContext = new LAContext();
    var biometricsAvailable = laContext.CanEvaluatePolicy(LAPolicy.DeviceOwnerAuthenticationWithBiometrics, out var error);

if (error is null) Debug.WriteLine("Error is null!");


3. Observe that ReSharper highlights a warning that the `error is null` expression is always false, and that the `Debug.WriteLine` call is "heuristically unreachable":
![image](https://user-images.githubusercontent.com/816612/137042505-6bf9da4f-54ba-4d5b-9dcf-de547ad30fc4.png)

### Expected Behavior

Code analyzers should not be seeing nullability attributes in the `Xamarin.iOS.dll` assembly telling them that things are never null when they can be.

### Actual Behavior

See above.

### Environment

https://gist.github.com/chamons/65153440df5d3549c9856789f2876a8b

### Example Project (If Possible)

<!--
1. Place cursor below this comment block.
2. Drag and drop the compressed project or files needed to reproduce.
-->
[XamariniOSNullabilityReproduction.zip](https://github.com/xamarin/xamarin-macios/files/7333668/XamariniOSNullabilityReproduction.zip)

<!--
Switch to the "Preview" tab to ensure your issue renders correctly.
-->
chamons commented 2 years ago

So, we do have nullability enable on the platform assemblies, with knowledge that they are not necessarily perfect.

Many of the bindings were written long before Apple documented the nullability of attributes in the headers.

To pick one example from a linked report:

https://developer.apple.com/documentation/uikit/uiactivityviewcontrollercompletionwithitemshandler?language=objc

This is defined in this file:

https://github.com/xamarin/xamarin-macios/blob/main/src/uikit.cs#L239

which does not have #nullable enable

however it is code generated into a SupportDelegates.g.cs file which does.

Asking sharpie about it:

 sharpie query iphoneos15.0-arm64.pch -n UIActivityViewControllerCompletionWithItemsHandler
typedef void (^UIActivityViewControllerCompletionWithItemsHandler)(UIActivityType _Nullable, BOOL, NSArray * _Nullable, NSError * _Nullable);

shows that it is correctly documented today.

Research will need to be done on why the existing nullability tests are not catching this, and what binding corrections are needed.