SE: Do not learn Null constraint from NotNullWhenAttribute

[pavel-mikula-sonarsource]

Fixes some Peach issues:

Issue
Line 67

            var managerType = typeof(SignInManager<>).MakeGenericType(builder.UserType);
            var customType = typeof(TSignInManager);
            if (!managerType.IsAssignableFrom(customType))
            {
                throw new InvalidOperationException(Resources.FormatInvalidManagerType(customType.Name, "SignInManager", builder.UserType.Name));

Issue
Line 423

Uri uriGrammar;
bool hasUri = Uri.TryCreate(grammarName, UriKind.Absolute, out uriGrammar);
int posDll = grammarName.IndexOf(".dll", StringComparison.OrdinalIgnoreCase);

Issue
Line 61

foreach (SyntaxNode leaf in node.ChildNodes())
            {
                if (leaf.IsKind(SyntaxKind.ArgumentList) == false && leaf.ToString().EndsWith("ToDictionary"))

..and few other similar cases

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[martin-strecker-sonarsource]

I think the root cause of the issues is not [NotNullWhen] handling in all three cases:

typeof

Line 67

Should be fixed by: 6157

Line 423

Multiple attributes

grammarName has the NotNull constraint before the call to TryCreate and therefore the state should not explode. I think the root cause is the wrong handling of multiple "NotNullWhen" attributes in conjunction with the last parameter which is out and has special handling in our logic:

public static bool TryCreate(
  [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true), System.Diagnostics.CodeAnalysis.StringSyntaxAttribute("Uri")] string? uriString,
  in System.UriCreationOptions creationOptions,
  [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out System.Uri? result) { throw null; }

Source

IsKind extension method

IsKind is tested for ==false and therefore it might be null after the &&. We did not learn that leaf is NotNull before the IsKind call. The only way to learn that leaf is not null there is to also learn from the FlowState of Roslyn which would prevent this kind of FPs.

We could suppress these kinds of FPs with this PR, but please add issues for each root cause, so we can tackle them the next time we come back to SE.

[martin-strecker-sonarsource]

This kills some FPs and some TPs. I'm not sure if we want to do this, because most FPs have a different root cause.

[martin-strecker-sonarsource]

Suggested change

	ProcessIsNotNullWhen(state, invocation.WrappedOperation, invocation.Arguments[0].ToArgument(), false, true),
	ProcessIsNotNullWhen(state, invocation.WrappedOperation, invocation.Arguments[0].ToArgument(), when: false, learnNull: true),

[pavel-mikula-sonarsource]

I'd consider that in case there are 3 of them

[martin-strecker-sonarsource]

named argument for "false"?

[pavel-mikula-sonarsource]

We strongly prefer FNs over FPs.
grammarName doesn't have NotNull, I've created #6158 yesterday to build it in the future.
The IsKind was a motivation for this PR. It just creates too many FPs.

[martin-strecker-sonarsource]

LGTM

grammarName doesn't have NotNull, I've created #6158 yesterday to build it in the future.

Ah, I missed the re-assignment part. It makes sense to add support for string methods.

The IsKind was a motivation for this PR. It just creates too many FPs.

That makes sense. I think we can re-add the constraint when we also take the "NullableFlowState" into account. If Roslyn thinks, that the value "MayBeNull" we can also add the "Null" constraint case. In the case of "None" (aka no nullable analysis performed by Roslyn) or "NotNull" we should not add the null constraint case.