Porting code to Use C# 8 Nullable Reference Types

[moh-hassan]

MoreLinq is supporting c# 8 and is built using vs 2019.

I did a quick test for using c#8 Nullable Reference Types by adding the next line to morlinq.csproj:

<Nullable>enable</Nullable>

I get many warning (~207 warning).

Is there a plan to use c# 8 Nullable Reference Types and fix all related warning?

[Orace]

PR #582 from @atifaziz addresses this.

[atifaziz]

As @Orace pointed out, I started on this in PR #582 (when C# 8 was still in beta) but got no feedback from other contributors who seem to have become inactive lately. I'd be very happy if you guys want to help review although it's not a very high priority item.

[moh-hassan]

@atifaziz I 'm happy to review the PR and feedback.

[moh-hassan]

I did the following steps:

• cloning PR #582 to a new branch Pr582, Inspecting the project, it's version: 4.4.0

• build the project Compilation Errors: 316 error Most error is: CS8632, The annotation for nullable reference types should only be used in code within a '#nullable'

• Fixing these errors by adding the next line to the project

        <Nullable>enable</Nullable>  
```		
- rebuild
  Compilation errors:  ~316~ 77  error

Taking into account that this PR is six month old (May 3, 2019  with 14-commits and changed 58 file) and trying to resolve these c#8 nullable errors will be lost after pulling the changes from the master branch  and resolving the confliction.
Also, there is changes in  c#8 nullable reference in the final c# RTM version.

The ideal solution is: 
- New PR for c#8 nullable starting from the last commit.
- Resolving is done in several commits.
- Insuring a complete test PASS (Exceptions can be discussed).
 
If you agree, I can start the new PR.

[Orace]

@moh-hassan, Sorry to have to say that: did you try to turn it of and on again ?

Indeed after adding <Nullable>enable</Nullable> I have to unload and reload the project to have the changes taken into account.

I did try a merge from master, a lot of conflict from the introduction of ??= but nothing really hard. But I got a lot of nullable warning (as error). I think it's because of some Assert(stuf != null) that should be removed.

You can take a look of the merge here : https://github.com/Orace/MoreLINQ/tree/nullability

[Orace]

There is also a lot of default(TKey) => default(TKey)!

[Orace]

I managed to build the solution. I probably added to much !. But an important issue is the Subject class that is referenced in both main and test project. So main and test project have to activate Nullable. So I removed the reference to Subject.cs in the test project and made it public (I have to add XML docs, empty for now). Removing Debug.Assert(stuf != null) helped a lot.

[moh-hassan]

@Orace, good work. I review your fork (branch nullability ) and it's build without errors and pass all tests. I consider the parts you use forgiving symbol (!) is correct. It was a surprise that c# 8 nullable reference is not completely supported in net4x, nestandard1.0 and nestandard2.0, especially for handling Generics, so you are correct in using Symbol !. It's completely supported in netstandard2.1 and netcore 3.0. Have a look to this limitation in dotnet/csharplang/issue

So, Your fork can be considered stage1 in porting to c#8 and the forgiving part may be modified in the future if a new nuget package cover the nullable reference generics. BTW, There is a typo in my comment and I modified it.

[Orace]

For the generic part, see this link. https://stackoverflow.com/questions/58863932/how-to-mark-a-generic-delegate-to-accept-null-values

We may need to add where T : notnull constraint on some (many ?) methods.

[moh-hassan]

I looked at the link you point,and your suggested solution:

if T is a class it's a sugar to add a NullableAttribute

This solution is completly applicable in netstandard2.1 and netcore3.0 as documented here

Trying to use it in net4x, netstandard1.0 and netstandard2.0 raise a compilation error:

CS0246 The type or namespace name 'AllowNullAttribute' could not be found (are you missing a using directive or an assembly reference?) (netstandard2.0)

and that what I was trying to do in your fork before posting my previous comment and get the error.

Also, in dotnet/csharplang

Officially, C# 8.0 is only supported on runtimes that adhere to the .NET Standard 2.1. That does not (and will not) include .NET Framework 4.x.

The same is applied for:

   T? Result<T>() where T : notnull;
   

raise a compilation error I think the forgiving operator ! is the workaround solution for such cases. Or annotate the class /code by:

        #nullable disable

[moh-hassan]

I found a solution for Nullable attributes by using this Nullable nuget package

 <ItemGroup>
 <PackageReference Include="Nullable" Version="1.1.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
 </ItemGroup>	

I tried it in a demo library supporting netstandard2.0 and net451 and it's working. I think we may complete stage2 using nullable attributes :) I will try it in your fork.

[Orace]

I will try it in your fork.

Ok for me, good luck with that ;)

[moh-hassan]

@Orace I fixed parts of the forgiving operator !. build success and Test pass. The others default()! are correct and not modified. The attached file is the Patch of changes of your fork which you can apply it and push PR here. Let me know if any help is needed.

c8_MoreLINQ_patch.zip

[Orace]

@moh-hassan can't you open a PR here? It will keep the work history.

[moh-hassan]

@Orace I have pushed PR #713.

[atifaziz]

This was closed by #582 and #803. Nullable reference annotations have been available since release 3.4.