Roslyn-analyzers: CA1062 should be NRT (nullable reference type) aware

[MithrilMan]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.8 (Latest)

Diagnostic ID

Example: CA1062

Repro steps

create a method that accept a non nullable reference like this

public void Handle(EventBase theEvent){
   theEvent!.DoAnything();
}

theEvent!.DoAnything() will complain about 1062 even if the ! is applied to the expression

Expected behavior

CA1062 should be NRT (nullable reference type) aware in order to not trigger when are are confident about never pass a null value from external libraries.

The C#8 nullable feature is nice, but it still requires to write lot of boilerplate code to ensure it's happy about public method arguments that may be null when invoked from external libraries.

When we are confident we'll use NRT in all project using such library, having a way to not write all null checks would be valuable.

Another very common scenario is when using classes in dependency injection scenarios, where a great percentage of classes shouldn't need an explicit null check

probably this behavior should be supported by a language feature, like using ! on method arguments to suppress this message implicitly (or even let the compiler generate the standard null check)

Actual behavior

there is no way to prevent CA1062 to trigger (except of course to nullcheck or disable the rule) even using the notnull ! operator in the expression

[mavasani]

Duplicate of https://github.com/dotnet/roslyn-analyzers/issues/3015#issuecomment-550413093. Quoting:

The [NotNull] attribute is not a guarantee, even when C# 8 is used with nullable reference types enabled. Arguments at the public API entry points should still be validated before use.

[mavasani]

Also see https://github.com/dotnet/roslyn-analyzers/issues/2875#issuecomment-536408486

[333fred]

@mavasani the request is to support the ! operator to suppress the warning, as I understood it.

[MithrilMan]

I'm aware of these comments but the need of an implicit way to handle case when we want to suppress CA1062 without a suppressor but by convention should imo get considered.

Yes, ! would be a solution to the problem

[333fred]

@MithrilMan please with the scenario you linked in discord.

[MithrilMan]

[mavasani]

@MithrilMan Thanks for the screenshot. Yes, that is something that our DFA framework should understand.

[sharwell]

Using the ! to suppress this warning is in _direct_ conflict with https://github.com/dotnet/roslyn/issues/34714. We should not use it here, because we will definitely not be recognizing this case on the other side and the tools will keep removing it.

[MithrilMan]

I agree that ! in the code is avoidable and I'd like to be able to use it in the argument declaration
public void Handle(EventBase! theEvent){
or
public void Handle(EventBase theEvent!){

[mavasani]

@sharwell has a good argument here. I think we have 2 broad scenarios here:

1. Public API with nullable enable for the file/project, where we know for sure that all API clients are also nullable enabled. CA1062 should be disabled for such a project/file.
2. Public API with nullable enable for the file/project, where the API author is not sure that all API clients are nullable enabled or not and hence there ought to be a null check in the public API, either explicit null check statement in code or the new C# 9 ! operator, which will cause the compiler to generate null check code. CA1062 should recognize both these null checks and not fire.

[333fred]

That last point is a c# language request. See https://github.com/dotnet/csharplang/issues/2145.

[mavasani]

Based on the last set of comments, I think the resolution is there is nothing to be done from analyzer side here. The compiler feature https://github.com/dotnet/csharplang/issues/2145 should address @MithrilMan's scenario?

[mavasani]

Closing as by design. Request tracked with dotnet/csharplang#2145