New rule S1133: Deprecated code should be removed

[martin-strecker-sonarsource]

Fixes #6656
RSpec SonarSource/rspec#1512

[gregory-paidis-sonarsource]

LGTM, just added a couple of comments for more testcases

[martin-strecker-sonarsource]

There is a big overlap to ObsoleteAttributesNeedExplanation:

sonar-dotnet/analyzers/src/SonarAnalyzer.Common/Rules/ObsoleteAttributesNeedExplanationBase.cs

Lines 30 to 44 in c117567

	protected ObsoleteAttributesNeedExplanationBase() : base(DiagnosticId) { }
	protected sealed override void Initialize(SonarAnalysisContext context) =>
	context.RegisterNodeAction(
	Language.GeneratedCodeRecognizer,
	c =>
	{
	if (c.SemanticModel.GetSymbolInfo(c.Node).Symbol is { } attribute
	&& attribute.IsInType(KnownType.System_ObsoleteAttribute)
	&& !attribute.GetParameters().Any())
	{
	c.ReportIssue(Diagnostic.Create(Rule, c.Node.GetLocation()));
	}
	},
	Language.SyntaxKind.Attribute);

What should we do @andrei-epure-sonarsource

• Merge this PR into master and do follow-up to merge the two rules
• Merge the two rules as part of this PR

[andrei-epure-sonarsource]

if you mean merging the implementation of the rules, I'd suggest to do it in a separate PR

if you mean to merge the rules themselves, it should be a discussion at LT level as these are already implemented in other languages, too

[martin-strecker-sonarsource]

I meant merging the implementations. I created #6698 to track the work.

[Corniel]

What about the following case?

[Obsolete("Will be dropped when the next major version is released.")]
public class Will_be_dropped
{
    [Test]
    public void TestSomething() => //..
}

Tests that are marked as obsolete are in most (if not all) cases decorated as such because the call deprecated code, as a way to prevent CS0618. Do we want to ignore those?

[cristian-ambrosini-sonarsource]

LGTM, really extensive tests!

[andrei-epure-sonarsource]

(please don't wait for a review from me to merge it, I don't have capacity to review it all and don't want to block it ; I was just wandering around the PRs previously when I left the comment)

[martin-strecker-sonarsource]

Tests that are marked as obsolete are in most (if not all) cases decorated as such because the call deprecated code, as a way to prevent CS0618. Do we want to ignore those?

I don't think we want to make any exceptions to the rule. The rule is to enable users in SonarCloud/SonarQube to track obsolete members. CS0618 doesn't raise when obsolete code calls obsolete code. The equivalent for this rule would be obsolete code scoped within obsolete code (e.g. obsolete method inside an obsolete class). I would still raise at both declarations because they might have a different deprecation policy.

The scope for the rule is "All" and this includes tests. Tests that call obsolete code will be removed along with the obsolete member itself. So I don't see why we shouldn't track the obsolete test class.

[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

[gregory-paidis-sonarsource]

LGTM, amazing testcase coverage.

Overly nitpick suggestion, feel free to ignore it and merge, I already approved:

I saw you added a record example for versions >= C#10, maybe add these as well:

 [Obsolete]
record class RecordClass { }

[Obsolete]
record struct RecordStruct { }

[Obsolete]
ref struct RefStruct // still no idea what this does
{
    [Obsolete]
    public RefStruct()    {    }

    [Obsolete]
    ref int MeaningOfLife = 42; // C# 11 weirdness
}

[martin-strecker-sonarsource]

@gregory-paidis-sonarsource This doesn't add much value with respect to the current implementation. When I did c.ContaingSymbol it was important to test every AttributeTarget to make sure c.ContaingSymbol points to the right symbol (which as it turned out doesn't always do). Now we start with the [Obsolete] declaration itself and we don't care where it is actually applied to.

[Corniel]

The scope for the rule is "All" and this includes tests. Tests that call obsolete code will be removed along with the obsolete member itself. So I don't see why we shouldn't track the obsolete test class.

Because you will have two warnings (S1133 and CS0618) if you have one test on your obsolete code, which is noise. You do not have the risk of not removing the test, as by removing the obsolete test, your test will not longer compile.

More generic, you could argue that a method that is marked as [Obsolete] that contains any code that is obsolete itself should not be triggered. Although I would not go down that road. That being said, having this rules on unit test methods or classes seems like FP's to me.