Fix S2219 FP: "Use the is operator" reports "unfixable" code

[antonioaversa]

Fixes #6616

Made into a draft because it should be merged after the release.

Remark:
As stated in the reporting of #6616, conditionals like typeof(ISet<>).IsInstanceOfType(obj) are guaranteed to always return false for any obj.
While this PR disables the rule (and related quick fix) in such scenarios, another approach could be to report that the check is always false.
This seems, however, a different rule, since it doesn't involve the is operator, and would not offer a quickfix.

[pavel-mikula-sonarsource]

Let's try to do this on syntax level

[pavel-mikula-sonarsource]

These comments would be way more readable when aligned on the same tab ditance

[pavel-mikula-sonarsource]

Same below

[antonioaversa]

I have tried to line those comments up, but I bumped into the following issue by the code fix test GetTypeWithIsAssignableFrom_CodeFix: Expected ActualCodeWithReplacedComments().ToUnixLineEndings() to be X with a length of 5756 because VerifyWhileDocumentChanges updates the document until all issues are fixed, even if the fix itself creates a new issue again. Language: CSharp7, but Y has a length of 5819, differs near " " (index 3683), where the only differences are in whitespaces:

I think that the test just preserves whitespaces when replacing // Noncompliant with // Fixed on a per-line basis, so lines end up misaligned in GetTypeWithIsAssignableFrom.Fixed.cs and GetTypeWithIsAssignableFrom.Fixed.Batch.cs (because fixes are of different length) and comparisons are sensitive to the difference in the number of whitespaces.

So we basically:

1. either have comments lined up in the main test-case file, and scrambled in Fixed.cs and Fixed.Batch.cs;
2. or have comments starting 1 chars from the ; (end of instruction), on the three files.

While not optimal, I think option 2 is more consistent and makes Fixed.cs and Fixed.Batch.cs easier to edit.

If you still prefer option 1, I can make the change.

[pavel-mikula-sonarsource]

Having the // Fixed stuff misaligned is (not nice but) acceptable. We'll eventually improve the test framework to ignore the whitespace differences for these comments (for other comments, it might be significant)

So let's go for 1.

[pavel-mikula-sonarsource]

Basically the reasoning is that now, it's not readable anywhere. After, it will be readable at least on the main source. While we don't care too much about the comments of the fixed part.

[pavel-mikula-sonarsource]

LGTM. There's whitespace and one more UT to be added before merging

[pavel-mikula-sonarsource]

DescendantNodes() is typically an expensive operation. Especially on a method level. Inside a typeof, it's fine, as we're sure that it will not contain a large sub-tree. So the implementation is good here.

The question is then, what about nested generic types? Can you have IDictionary<string, ISet<>>? Let's add UT

[antonioaversa]

Tests added.

I've found a property of GenericNameSyntax called IsUnboundGenericName:

public bool IsUnboundGenericName
{
    get
    {
        return this.TypeArgumentList.Arguments.Any(SyntaxKind.OmittedTypeArgument);
    }
}

c.p. https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Syntax/GenericNameSyntax.cs#L19

The method above has a very similar implementation to the one we have drafted together. I used that method instead.

One additional note: the actual check is slightly more complex since a QualifiedNameSyntax does not derive from GenericNameSyntax, but contains it as Right member for namespaced type:

typeof(System.Collections.Generic.ISet<>)

typeof(ISet<>)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information