New Rule S4545: "DebuggerDisplayAttribute" strings should reference existing members

[antonioaversa]

Fixes #6703

RSPEC: SonarSource/rspec#1571

Remarks:

Primary use is for types and assemblies.
While not recommended, DebuggerDisplayAttribute can be used on properties, fields, delegates, etc. c.p. https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debuggerdisplayattribute?view=net-7.0

class SomeClass
{
    [DebuggerDisplay("{SomeProperty}")] int Property1 => 1;
    [DebuggerDisplay("{SomeField}")] int Property2 => 1;
    // ...
}

When applied to properties or fields, the context is the one of the enclosing type:

public class SomeClass
{
    [DebuggerDisplay("{Field2 + \"b\"}")]
    private string Field1 = "a1";

    private string Field2 = "a2";

    public static void Main()
    {
        var x1 = new Class1(); // Debugger shows `a2b`
    }
}

It is unclear how it is supposed to be used/to work on enum: setting it in VS 2022 results in the debugger <Internal error evaluating expression>.

Expression of arbitrary complexity can be written in the attribute, involving 0 or more fields.

class SomeClass
{
    [DebuggerDisplay("{1 + 1}")] int Property1 => 1;
    [DebuggerDisplay(@"{""1"" + ""1""}")] int Property2 => 1;
    [DebuggerDisplay("{this.Property1}")] int Property3 => 1;
    [DebuggerDisplay("{this.Property1 + this.Property2}")] int Property4 => 1;
    // ...
}

This implementation produces false negatives due to the following limitations:

• string constants, when used as value, are not checked;
• string concatenation, when used to build value, are not evaluated;
• this.Member is not supported;
• complex expressions, such as 1 + Member, are not supported;
• aliases for DebuggerDisplayAttribute are not supported;
• constant interpolated strings not supported (C# 10+) are not supported.

[zsolt-kolbay-sonarsource]

Nicely done! Those are some really detailed test cases.
I've only found some minor issues.

[zsolt-kolbay-sonarsource]

Just a couple more findings:

[zsolt-kolbay-sonarsource]

LGTM

[pavel-mikula-sonarsource]

RSPEC needs to be updated i rspec repo and then also here

[pavel-mikula-sonarsource]

Also here (comments are out of order, the the 3rd below first)

[pavel-mikula-sonarsource]

Rest of the review.

I'll let @andrei-epure-sonarsource to do 2nd round and resolve my comments.

[andrei-epure-sonarsource]

See some comments I didn't close:

#6728 (comment)

#6728 (comment)

[andrei-epure-sonarsource]

please check the quality gate :)

[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

95.7% Coverage
0.0% Duplication

[antonioaversa]

please check the quality gate :)

Code smells

I have removed the two methods I have forgotten, in the C# and VB.NET facades.
That has removed 2 of the 4 code smells.
The two remaining code smells are about the cognitive complexity of the facades, which is now exceeding the allowed limit of 30.
Complexity is a typical problem for facades when they expose too many behaviors.
It seems that, with the current implementation of AvoidExcessiveClassCoupling, you can go around the code smell by splitting into partial classes:

    class A1 { }
    class A2 { }

    partial class A { A1 P1; } // Compliant, with threshold 2
    partial class A { A2 P2; }

    partial class B { A1 P1; A2 P2; } // Noncompliant, with threshold 2

But this seems to me just a workaround.

Any idea @andrei-epure-sonarsource ?

[antonioaversa]

please check the quality gate :)

Coverage

Removing the two unused methods in VB and C# facades has increased the coverage level, which is now > 95% overall.

DebuggerDisplayUsesExistingMembersBase is still below 95%, due to RegexMatchTimeoutException handling.

That is not easy to test, without mocking Regex.Matches.

@andrei-epure-sonarsource If you think it makes sense to cover that scenario, I would look into it.

[andrei-epure-sonarsource]

LGTM

[andrei-epure-sonarsource]

@antonioaversa you can resolve the SC S1200 issues as "won't fix" and then merge this (the gate will become green)

all good on not covering the Regex exception

Andrei took over the review