Improve S1144: highlight only identifiers

[zsolt-kolbay-sonarsource]

Fixes #6321

[martin-strecker-sonarsource]

We need to discuss what to do about the fadeOutCode argument of the diagnostic. The best solution would be to fade out the entire method (current behavior) while squiggling only the method name at the same time. I don't think this is possible but you may be able to find a good workaround.

[andrei-epure-sonarsource]

can you bring more detail on this, @martin-strecker-sonarsource ?

I looked in the (both the version we use and the latest) and could not find this argument

[martin-strecker-sonarsource]

This is what it looks like for me for unused methods in VS:

The fadeOutCode parameter dims the whole method which looks nice (see e.g. the difference between private and protected). But at the same time, all the squiggles are much too offensive.
So for me fading out the whole method while only squiggling the method name (or don't squiggle at all) would be the best compromise.

If we only squiggle e.g. the method name I would remove the fadeOutCode option because that would most likely look awkward.

I'm pretty sure that fading out is only supported by VS as it is just a custom property with a special name added to the diagnostic.

It is set here:

sonar-dotnet/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs

Lines 35 to 36 in 278440f

	private static readonly DiagnosticDescriptor RuleS1144 = DescriptorFactory.Create(S1144DiagnosticId, S1144MessageFormat, fadeOutCode: true);
	private static readonly DiagnosticDescriptor RuleS4487 = DescriptorFactory.Create(S4487DiagnosticId, S4487MessageFormat, fadeOutCode: true);

[zsolt-kolbay-sonarsource]

Thank you, that's good to know 😃 I actually thought we were doing the squiggly and VS is doing the fading out by itself.

I checked the code, fadeoutCode is translated to WellKnownDiagnosticTags.Unnecessary and it applies to the entire Diagnostic. I don't see any way of applying it partially. The only way I see to do this is to raise a secondary issue, which only does the fading of the code. But that sounds like an awkward solution. So I simply removed the fade out effect for S1144.
What do think?

[martin-strecker-sonarsource]

If you want to go crazy, you can do what Roslyn does in such a case:
https://github.com/dotnet/roslyn/blob/1d8f8cd8be6581a03e8379b8f146dcb5f11800c9/src/Analyzers/Core/Analyzers/Helpers/DiagnosticHelper.cs#L64-L105

See how additionalUnnecessaryLocations is handled. But you may ask @andrei-epure-sonarsource if he sees the need for us to support a scenario where the fadeout is different from the squiggles.

[andrei-epure-sonarsource]

this is nice and interesting to see @martin-strecker-sonarsource !

I would favor to keep this PR simple - adding this new functionality to the analyzer should come with a change in the testing framework too, and we'd go too wide for this issue

I think it would be a cool thing, but I would also check where else this might help and do the updates to all the rules - this should probably be an MMF on improving visibility and useability of issues inside IDEs (feel free to discuss it with @tom-howlett-sonarsource and mention the idea to the bubble)

[martin-strecker-sonarsource]

I created #6697

[zsolt-kolbay-sonarsource]

Thank you. I disabled the fade-out effect for now for both rules.

[andrei-epure-sonarsource]

great work!

I left some suggestions

[martin-strecker-sonarsource]

There might be some cases that don't work as intended.

[andrei-epure-sonarsource]

LGTM, great work on this one!

Thanks @martin-strecker-sonarsource for joining the review!

[martin-strecker-sonarsource]

I took a quick look and it seems that arrow expressions are missing in general. Could you add these after checking if the test cases are really missing?

Prop { get => 1; }
Prop { set => _Prop = value; }

Indexer are also missing and most likely are FPs

private int this[int i] => 1;
private int this[int i] { get => 1; }
private int this[int i] { set => _ = value; }
private int this[int i] { get { return 1; } set { _ = value; } }

[zsolt-kolbay-sonarsource]

Good catch. I've changed the code so that it only highlights the this keyword for unused indexers.
Also added test cases for expression-bodied properties.

[martin-strecker-sonarsource]

Some test cases might be missing.

[martin-strecker-sonarsource]

GetIdentifier should return the this token for an indexer declaration.

[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]

LGTM.