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

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

Open
ArcanoxDragon opened this issue Oct 13, 2021 · 1 comment
Labels
bug If an issue is a bug or a pull request a bug fix iOS Issues affecting iOS
Milestone

Comments

@ArcanoxDragon
Copy link

ArcanoxDragon commented Oct 13, 2021

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!");
  1. 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

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)

XamariniOSNullabilityReproduction.zip

@chamons
Copy link
Contributor

chamons commented Oct 13, 2021

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.

@chamons chamons added the iOS Issues affecting iOS label Oct 13, 2021
@chamons chamons added this to the Future milestone Oct 13, 2021
@chamons chamons added the bug If an issue is a bug or a pull request a bug fix label Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix iOS Issues affecting iOS
Projects
None yet
Development

No branches or pull requests

2 participants