Report hidden diagnostic for unnecessary !

[cston]

Consider reporting a warning for unnecessary !.

object x = MayBeNull()!; // ok
object y = NotNull()!;   // warning

[jcouv]

LDM decided not to produce such a warning for un-necessary !.
https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-08-20.md#remaining-questions-on-suppression-operator

[jcouv]

I'm thinking about this again.
Users may need to temporarily add suppressions. The danger is that they are left there forever, even after they become irrelevant.
We should consider producing a hidden diagnostic when a suppression doesn't affect the top-level nullability and doesn't suppress any conversion warnings.

Updated title accordingly.

[stephentoub]

An analyzer + auto-fix for this (#34714) would be very welcome.

[mavasani]

We should consider producing a hidden diagnostic when a suppression doesn't affect the top-level nullability and doesn't suppress any conversion warnings.

@jcouv I think we should follow the same pattern as other compiler + analyzer diagnostics. Report a diagnostic with correct severity, but with IsSuppressed property set to true. We can even go a step further and only do this when CompilationOptions.ReportSuppressedDiagnostics is true. This flag is always false for command line builds, so ensures no CI impact. This approach also has following advantages:

1. You can view suppressed nullable diagnostics from ! in error list by changing the "Suppression State" column filter to check "Suppressed" checkbox. IDE live analysis always computes suppressed diagnostics.
2. These are not reported on command line
3. I can easily enhance the analyzer/fixer added in Unnecessary pragma/SuppressMessage suppressions for analyzer diagnostics #44178 to report unnecessary ! operators

@jcouv I am going to try and prototype this if it sounds reasonable to you.

[chylex]

I ran into a (probably very) uncommon case where a warning would've been helpful - after a while of using Kotlin, I got the languages mixed up a bit and tried using Kotlin's !is operator in C#. Of course,

if (obj !is Something)

is understood as

if (obj! is Something)

rather than an error or a warning, so at least a warning would've saved me some confusion at compile time :P

[CyrusNajmabadi]

@chylex You can definitely write an analyzer for that purpose.

[chylex]

@chylex You can definitely write an analyzer for that purpose.

Of course, I'm just pointing out a possible case where silently accepting ! when it does nothing caused confusion, and would be resolved by what this issue's author proposed - Consider reporting a warning for unnecessary !.

[CyrusNajmabadi]

Yup. We're discussing this internally right now :)

[CyrusNajmabadi]

I wrote up a very specific fix for the x !is Pattern case here: #44878

[mavasani]

I took a stab at the approach I mentioned in #25372 (comment). I was able to update the Nullability walker to track and report suppressed diagnostics in presence of ! operators (could also have been hidden diagnostics, that part is not relevant). However, the case that seems difficult to handle is when ! operator changes the nullability of a resultant expression, and that expression is used down the line in some context where it would generate a nullable diagnostic if ! operator was not used in previous statement. For example,

#nullable enable

class C<T> { }
class C
{
    static void F(C<object>? x, C<object?> y, bool c)
    {
        var b = x!;   // Removing the ! here causes a CS8600 on 'c ? b : y!'
        C<object> a = c ? b : y!;
    }
}

So the above code snippet reports no suppressed nullable diagnostics from x!, even though it is indeed changing nullable analysis in a way that would cause nullable warnings in subsequent statements if x! was changed to x.

Basically, it seems the only way to detect a redundant ! would be to speculatively bind the method body with the ! removed, which seems to re-run the nullable walker on speculated code, and see if any new nullability diagnostics were generated. I am going to sync up with @jcouv offline to see if he has any alternate suggestions.

[jnm2]

@mavasani To make things worse, I bet it's possible to be in a situation where removing two unnecessary ! operators at the same time would result in warning-free code, but testing just one at a time would not. It seems like a comprehensive approach can't be based around try-and-see but rather would have to be coordinating throughout the whole process. Maybe it would be prohibitively slow or difficult to build.

I'm less sure about this because I can't think of an example yet.

[mavasani]

It seems like a comprehensive approach can't be based around try-and-see

One possible approach would be to support GetDiagnsotics on speculative semantic model - we already hook up re-running the nullable analysis when creating a speculated semantic model (see http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Compilation/MemberSemanticModel.cs,1971), but right now GetDiagnostics is not supported for speculative semantic model (http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Compilation/MemberSemanticModel.cs,574). If this support was added, then the analyzer can just speculate removal of ! and check if nullable diagnostics did not change.

Tagging @333fred as I believe he did the work of hooking up nullable analysis for speculative semantic model.

[jnm2]

I found an example where removing exactly one ! suppression causes a new warning to appear, making removal appear invalid, but where removing both suppressions at once does not cause a new warning. Not sure how likely this is to appear in non-contrived code. And it might be fine for the analyzer to never detect this even if such changes would ordinarily be preferred.

For the try-and-see black box approach to detect this, it would have to speculatively bind up to 2^(operator count) times, comparing every combination of removals to the baseline till it finds something.

class C
{
    void M(string? p)
    {
        var x = new[] { p! };
        var y = new[] { p! };

        y = x;
        x = y;
    }
}

[333fred]

There's a very simple example: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+BiAdgVwBtCJhC4ACOXU8gWACgABAZgqYCYKBhCgb0YUh7NkwAsFALIAKAJT9BwpUwCMABgD8FKBQC8FAsQCEAbkVKhUAHQAVAPYBlGAgCWuAOZyzDJQF9GvkA==

I'll have to think about the speculative semantic model diagnostics.

[333fred]

Basically, it seems the only way to detect a redundant ! would be to speculatively bind the method body with the ! removed, which seems to re-run the nullable walker on speculated code, and see if any new nullability diagnostics were generated.

I'm worried by this because we know that speculative analysis isn't super precise. There are a number of places in the compiler with the speculative model that we take shortcuts because we know we won't have to provide diagnostics on speculative models. We don't fully rerun nullable analysis, for example: we only run nullable analysis on the node given to speculate, not the entire body.

[RikkiGibson]

Self-assigning due to interest in the issue, but anyone else wishing to do design/implementation work should feel free

[manfred-brands]

I tried this in an analyzer, but the TypeInfo is the same for nullable and non-nullable types.
NullabilityInfo is: NullableAnnotation.NotAnnotated, NullableFlowState.NotNull.

What do I do wrong?

public override void Initialize(AnalysisContext context)
{
    context.EnableConcurrentExecution();
    context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
    context.RegisterSyntaxNodeAction(AnalyzeNullForgivenessOperator, SyntaxKind.SuppressNullableWarningExpression);
}

private void AnalyzeNullForgivenessOperator(SyntaxNodeAnalysisContext context)
{
    var node = (PostfixUnaryExpressionSyntax)context.Node;

    TypeInfo typeInfo = context.SemanticModel.GetTypeInfo(node.Operand, context.CancellationToken);
    if (typeInfo.Nullability.FlowState != NullableFlowState.MaybeNull)
    {
        var diagnostic = Diagnostic.Create(Rule, node.GetLocation());
        context.ReportDiagnostic(diagnostic);
    }
}

TheTypeInfo returned for both node and node.Operand is the same.
I expected that the operator would change this.

[luizfbicalho]

I don't think that #68267 is duplicate @Youssef1313

This only diagnostic the ! and the other looks for ! , !. and ?.