S3900: Change parameter dereference check to top-down

[zsolt-kolbay-sonarsource]

Minor update for S3900.
Switching from bottom-up search to top-down approach when dealing with operations.
This changed resolved a couple of FPs.

[Tim-Pohlmann]

I would go with one of the following two:

        if (NullDereferenceCandidate(context.Operation.Instance) is { } candidate
            && candidate.Kind == OperationKindEx.ParameterReference
            && candidate.ToParameterReference() is { Parameter: var parameter, WrappedOperation: var wrappedOperation }
            && !parameter.Type.IsValueType
            && NullableStateIsNotKnownForParameter(parameter)
            && !parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute))
        {
            var message = SemanticModel.GetDeclaredSymbol(Node).IsConstructor()
                ? "Refactor this constructor to avoid using members of parameter '{0}' because it could be null."
                : "Refactor this method to add validation of parameter '{0}' before using it.";
            ReportIssue(wrappedOperation, string.Format(message, wrappedOperation.Syntax), context);
        }

        if (NullDereferenceCandidate(context.Operation.Instance) is { } candidate
            && IsParameterReference(candidate, out var parameter, out var wrappedOperation)
            && !parameter.Type.IsValueType
            && NullableStateIsNotKnownForParameter(parameter)
            && !parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_FromServicesAttribute))
        {
            var message = SemanticModel.GetDeclaredSymbol(Node).IsConstructor()
                ? "Refactor this constructor to avoid using members of parameter '{0}' because it could be null."
                : "Refactor this method to add validation of parameter '{0}' before using it.";
            ReportIssue(wrappedOperation, string.Format(message, wrappedOperation.Syntax), context);
        }

[Tim-Pohlmann]

LGTM. One more refactor suggestion. And you have to look into the ITs.

[Tim-Pohlmann]

Suggested change

	return candidate?.Kind == OperationKindEx.Conversion
	? ConversionOperand(candidate)
	: candidate;
	}
	private static IOperation ConversionOperand(IOperation operation) =>
	operation.Kind == OperationKindEx.Conversion
	? ConversionOperand(operation.ToConversion().Operand)
	: operation;
	return UnwrapConversions(candidate);
	}
	private static IOperation UnwrapConversions(IOperation operation) =>
	operation?.Kind == OperationKindEx.Conversion
	? UnwrapConversions(operation.ToConversion().Operand)
	: operation;

[zsolt-kolbay-sonarsource]

I'd rather keep the name ConversionOperand, as it shows what the return value will be.

[Tim-Pohlmann]

The issue I see with that is that it will only return a conversion operand if the operation is a conversion.

[pavel-mikula-sonarsource]

Taking shortcuts, will review implementaion in a minute

[pavel-mikula-sonarsource]

Code

[pavel-mikula-sonarsource]

This actually illustrates why top-to-bottom is better, due to the execution order

[pavel-mikula-sonarsource]

LGTM

[pavel-mikula-sonarsource]

Very nice UTs!

[pavel-mikula-sonarsource]

This empty line doesn't help. If we would need semicolon, new line, newline as a command separator to make the language readable, we would have to look for another language.

Suggested change

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication