Null guard not working for c# 8 Nullable

[dominikjeske]

I'm testing library for c# 8 support but it looks like it is not working

public static string Test(string? text) { text.MustNotBeNull(); return text.Substring(0, 1); }

With proper implementation there should be no warrning for text but currently I have:

"Warning CS8602 Dereference of a possibly null reference"

Check is only woring when return value is used

public static string Test(string? text) { var x = text.MustNotBeNull(); return x.Substring(0, 1); }

but I would not like to create separate variable only for this

[feO2x]

Hey @bigdnf, unfortunately, this is currently not supported with NRT. There currently is no attribute that allows me to tell the C# compiler that a method parameter is not null when a method returns. I also face this issue in my projects at work.

To circumvent it, I usually write code like in the following example:

public void Foo(Bar? bar)
{
    bar = bar.MustNotBeNull(nameof(bar));
    // bar from here on is not considered null
}

With this pattern, you won't introduce a new variable to your method. I also think that it produces the same assembly instructions, but I have to double-check that.

[feO2x]

I'm closing this issue due to inactivity. If you have further questions, please reopen it.

[reima]

I believe it should be possible to guide the C# compiler in a NRE context with the attributes listed in https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis (specifically NotNullAttribute and NotNullWhenAttribute, maybe others).

These attributes are not available in .NET Standard 2.0, but can be backported with the Nullable package, which is already referenced by Light.GuardClauses.

One minor caveat is that JetBrains.Annotations also contains an incompatible NotNullAttribute with slightly different semantics: JetBrains.Annotations.NotNullAttribute on a parameter means that the parameter cannot be null, while System.Diagnostics.CodeAnalysis.NotNullAttribute on a parameter means that the input argument was not null when the call returns. Care must be taken not to confuse these attributes.

@feO2x If this is something you are interested to have in Light.GuardClauses, I could take a stab at adding these attributes and submit a PR. Please let me know what you think.

[feO2x]

@reima Yep, that would be nice. I was wondering if the ValidatedNotNullAttribute is currently used or if it just can be replaced with the new attributes. Maybe we should research if these are still necessary.

[feO2x]

@reima after a good night of sleep, I have the following recommendation: we should introduce new projects where we target different frameworks like .NET Framework 4.8, .NET 7 / .NET 8, Mono, etc. We then reference the Light.GuardClauses project in them and check in different IDEs / editors with different settings (NRTs on or off) whether the compiler can derive that MustNotBeNull (and maybe other assertions?) are structured in a way so that control flow analysis can pick up that a parameter is not null.

I would really like to automate this, but cannot think of a way to actually do that. With Roslyn, we could pass in source code and then check if the analyzers produce the corresponding warnings, however I don't think this approach is possible with Rider/ReSharper or OmniSharp in VS Code. At least we can do the manual testing.

I'd suggest that we also try to remove existing stuff to reduce bloat - I'd start with the JetBrains annotations and continue with the ValidatedNotNullAttribute - maybe we can remove old baggage. I'd also suggest that we only focus on new versions of IDEs and editors, and not test with Visual Studio 2017, for example.

The overall idea is to have prepared projects with corresponding code, open them up in different IDEs / editors and check if the on-the-fly analysis does not produce any errors.

What's your opinion on this approach? I'm eager to hear your thoughts.

[reima]

@feO2x ValidatedNotNullAttribute was previously used internally in .NET Core (e.g. in System.Collections.Immutable), but has since been replaced by the newer NotNullAttribute. It might still be picked up by some static analysis tools (like Sonar). I'm not sure if it should be kept around.

A preliminary test seems to indicate that the current version of Rider also takes the C# 8 nullability attributes into account. In that case the JetBrains nullability attributes ([ContractAnnotation], [NotNull]) could probably be removed safely. However they could still be valuable for scenarios where the #C 8 nullability attributes cannot express some dependency. I'm not sure if there are any in Light.GuardClauses.

The JetBrains [NoEnumeration] attribute is still required, so that Rider/ReSharper do not issue the Possible multiple enumeration of IEnumerable inspection.

Automated checking if the C# compiler and other tools correctly pick up the nullability attributes is an excellent idea.

This is totally doable for both Roslyn and ReSharper:

• Compiling code with the Roslyn API and getting the diagnostics is relatively easy to do. I've done this myself about a month ago.
• JetBrains provides the InspectCode command line tool free of charge. It provides the same analyzers that ReSharper/Rider use. The output is easily consumable by tools (XML or JSON). I have some experience with configuring and running this command line tool.

I believe OmniSharp uses the Roslyn analyzers, so we should be covered by compiling with Roslyn? I'm not certain though, as I do not use OmniSharp.

[feO2x]

Cool, I didn't know about the Inspect Code CLI from JetBrains. Then let's go the full automatic way.

Regarding OmniSharp: I don't know about their latest developments, but to my knowledge, they are also just using Roslyn analyzers. But that's something we might want to double check.

[reima]

I've made some experiments with Roslyn and the InspectCode tool. The nice thing about InspectCode is that it can also build the analyzed solution and collect build warnings into its output. This way we don't even need to run the C# compiler manually to get the Roslyn warnings.

What worked reasonably well is the following setup:

Add a new project file Code/Light.GuardClauses.Inspections/Light.GuardClauses.Inspections.csproj:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFrameworks>netstandard2.0;netstandard2.1;net6.0;net7.0</TargetFrameworks>
        <LangVersion>11.0</LangVersion>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="JetBrains.Annotations" Version="2023.2.0" />
        <ProjectReference Include="..\Light.GuardClauses\Light.GuardClauses.csproj" />
    </ItemGroup>

</Project>

Add a source file Code/Light.GuardClauses.Inspections/NullableWarnings.cs:

// ReSharper disable UnusedType.Global
// ReSharper disable UnusedMember.Global

#nullable enable

namespace Light.GuardClauses.Inspections;

internal static class NullableWarnings
{
    public static void MustNotBeNull(object? obj)
    {
        _ = obj.MustNotBeNull().ToString();
        _ = obj.ToString();
    }
}

This file uses the guard clauses in a NRT context and is meant to trigger C# build warnings if Light.GuardClauses does not have the correct annotations.

Add a source file Code/Light.GuardClauses.Inspections/JetBrainsWarnings.cs:

// ReSharper disable UnusedType.Global
// ReSharper disable UnusedMember.Global

using System.Collections;
using JetBrains.Annotations;

namespace Light.GuardClauses.Inspections;

internal static class JetBrainsAnnotationsWarnings
{
    public static void MustNotBeNull_Nullability([CanBeNull] object obj)
    {
        _ = obj.MustNotBeNull().ToString();
        _ = obj.ToString();
    }

    public static void MustNotBeNull_NoEnumeration(IEnumerable enumerable)
    {
        enumerable.MustNotBeNull();
        _ = enumerable.GetEnumerator();
    }
}

This file uses the guard clauses in a non-NRT context and is meant to trigger InspectCode warnings if Light.GuardClauses does not have the correct annotations.

In order to analyze the Light.GuardClauses.Inspections project with InspectCode we add a new solution file Code\Light.GuardClauses.InspectionProjects.sln and add the projects Light.GuardClauses and Light.GuardClauses.Inspections to it.

Now we can install the InspectCode as a dotnet tool:

dotnet tool install --global JetBrains.ReSharper.GlobalTools

Running the tool from the root of the repository:

 jb inspectcode .\Code\Light.GuardClauses.InspectionProjects.sln --project=Light.GuardClauses.Inspections --build --output=results.sarif.json --format=Sarif

The resuting JSON file results.sarif.json contains the warnings in the $.runs[0].results list. The goal should be that this list is empty.

The JSON file can also be added to GitHub's CodeQL so the inspections show up in the repo's Security tab under "code scanning alerts".

To run InspectCode and add the results to CodeQL in a GitHub workflow, the following step can be used:

    - name: Inspect code
      uses: JetBrains/[email protected]
      with:
        tool-version: 2023.2
        solution: ./Code/Light.GuardClauses.InspectionProjects.sln
        project: Light.GuardClauses.Inspections
        build: true

Alternatively the tool can be installed and run manually in a GitHub workflow (e.g. if we don't want the results to be added to CodeQL and instead process them some other way):

    - name: Setup .NET
      uses: actions/setup-dotnet@v3
      with:
        dotnet-version: 7.0.x
    - name: Install ReSharper command line tools
      run: dotnet tool install --global JetBrains.ReSharper.GlobalTools --version 2023.2
    - name: Inspect code
      run: jb inspectcode ./Code/Light.GuardClauses.InspectionProjects.sln --project=Light.GuardClauses.Inspections --build --output=inspections.sarif.json --format=Sarif

@feO2x I hope this helps in setting up the inspection infrastructure. I'm not familiar enough with GitHub workflows or the overall project conventions, so I'm not comfortable working on a PR myself.

[feO2x]

Thanks for all your efforts - I'm currently thinking about letting this run in xunit. We could simply parse the resulting JSON and make assertions on it. To execute jb inspectcode we could use CLI.Wrap. jb could also be installed as a local tool using a tool manifest.