Skip to content

Commit 404383d

Browse files
mdekreymdekrey
committed
Split Must Remove and Must Refactor analyzers
1 parent 0746739 commit 404383d

10 files changed

Lines changed: 1049 additions & 862 deletions

‎Analyzers/Analyzers.Test/MustNotUsePlannedForRemovalAnalyzerShould.cs‎

Lines changed: 0 additions & 793 deletions
This file was deleted.

‎Analyzers/Analyzers.Test/MustRefactorPlannedForRemovalAnalyzerShould.cs‎

Lines changed: 560 additions & 0 deletions
Large diffs are not rendered by default.

‎Analyzers/Analyzers.Test/MustRemovePlannedForRemovalAnalyzerShould.cs‎

Lines changed: 283 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,283 @@
1+
using System.Threading.Tasks;
2+
using AnalyzerTesting.CSharp.Extensions;
3+
using DarkPatterns.Refactoring.Verifiers;
4+
using Microsoft.VisualStudio.TestTools.UnitTesting;
5+
6+
namespace DarkPatterns.Refactoring;
7+
8+
[TestClass]
9+
public class MustRemovePlannedForRemovalAnalyzerShould
10+
{
11+
public TestContext TestContext { get; set; }
12+
13+
[TestMethod]
14+
public async Task Produce_no_diagnostics_by_default()
15+
{
16+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
17+
.AddSources(@"")
18+
.RunAsync(TestContext.CancellationToken);
19+
}
20+
21+
#region fields
22+
[TestMethod]
23+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_private_field_when_marked_for_refactor()
24+
{
25+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
26+
.AddSources("""
27+
[PlannedRemoval("1", "Remove this class")]
28+
class OutdatedClass { }
29+
30+
[PlannedRefactor("1", "Needs rework")]
31+
class SomeClass
32+
{
33+
private OutdatedClass shouldBeRefactored;
34+
}
35+
""")
36+
.RunAsync(TestContext.CancellationToken);
37+
}
38+
39+
[TestMethod]
40+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_field_when_marked_for_refactor()
41+
{
42+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
43+
.AddSources("""
44+
[PlannedRemoval("1", "Use NewClass instead")]
45+
class OutdatedClass { }
46+
47+
[PlannedRefactor("1", "Needs rework")]
48+
class SomeClass
49+
{
50+
protected {|#0:OutdatedClass|} shouldBeRefactored;
51+
}
52+
""")
53+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "shouldBeRefactored", "1", "Use NewClass instead"))
54+
.RunAsync(TestContext.CancellationToken);
55+
}
56+
57+
// TODO: test field initialization
58+
#endregion fields
59+
60+
#region properties
61+
[TestMethod]
62+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_property_when_marked_for_refactor()
63+
{
64+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
65+
.AddSources("""
66+
[PlannedRemoval("1", "Use NewClass instead")]
67+
class OutdatedClass { }
68+
69+
[PlannedRefactor("1", "Needs rework")]
70+
class SomeClass
71+
{
72+
protected {|#0:OutdatedClass|} ShouldBeRefactored { get; set; }
73+
}
74+
""")
75+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
76+
.RunAsync(TestContext.CancellationToken);
77+
}
78+
79+
[TestMethod]
80+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_property_when_marked_for_refactor()
81+
{
82+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
83+
.AddSources("""
84+
[PlannedRemoval("1", "Remove this class")]
85+
class OutdatedClass { }
86+
87+
[PlannedRemoval("1", "Remove this class too")]
88+
class SomeClass
89+
{
90+
protected OutdatedClass ShouldBeRefactored { get; set; }
91+
}
92+
""")
93+
.RunAsync(TestContext.CancellationToken);
94+
}
95+
96+
// TODO: test property initialization
97+
#endregion properties
98+
99+
#region method return type
100+
[TestMethod]
101+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_method_return_type_when_marked_for_refactor()
102+
{
103+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
104+
.AddSources("""
105+
[PlannedRemoval("1", "Use NewClass instead")]
106+
class OutdatedClass { }
107+
108+
[PlannedRefactor("1", "Needs rework")]
109+
class SomeClass
110+
{
111+
protected {|#0:OutdatedClass|} ShouldBeRefactored() => null;
112+
}
113+
""")
114+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
115+
.RunAsync(TestContext.CancellationToken);
116+
}
117+
118+
[TestMethod]
119+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_method_return_type_when_marked_for_removal()
120+
{
121+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
122+
.AddSources("""
123+
[PlannedRemoval("1", "Remove this class")]
124+
class OutdatedClass { }
125+
126+
[PlannedRemoval("1", "Remove this too")]
127+
class SomeClass
128+
{
129+
protected {|#0:OutdatedClass|} ShouldBeRefactored() => null;
130+
}
131+
""")
132+
.RunAsync(TestContext.CancellationToken);
133+
}
134+
#endregion method return type
135+
136+
#region method parameter type
137+
[TestMethod]
138+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_method_parameter_type_when_marked_for_refactor()
139+
{
140+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
141+
.AddSources("""
142+
[PlannedRemoval("1", "Use NewClass instead")]
143+
class OutdatedClass { }
144+
145+
[PlannedRefactor("1", "Needs rework")]
146+
class SomeClass
147+
{
148+
protected void ShouldBeRefactored({|#0:OutdatedClass|} outdatedClass) { }
149+
}
150+
""")
151+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
152+
.RunAsync(TestContext.CancellationToken);
153+
}
154+
155+
[TestMethod]
156+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_method_parameter_type_when_marked_for_removal()
157+
{
158+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
159+
.AddSources("""
160+
[PlannedRemoval("1", "Remove this class")]
161+
class OutdatedClass { }
162+
163+
class SomeClass
164+
{
165+
[PlannedRemoval("1", "Needs rework")]
166+
protected void ShouldBeRefactored({|#0:OutdatedClass|} outdatedClass) { }
167+
}
168+
""")
169+
.RunAsync(TestContext.CancellationToken);
170+
}
171+
#endregion method parameter type
172+
173+
#region method type parameter constraint
174+
[TestMethod]
175+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_method_type_parameter_constraint_when_marked_for_refactor()
176+
{
177+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
178+
.AddSources("""
179+
[PlannedRemoval("1", "Use NewClass instead")]
180+
class OutdatedClass { }
181+
182+
[PlannedRefactor("1", "Needs rework")]
183+
class SomeClass
184+
{
185+
protected void ShouldBeRefactored<T>()
186+
where T : {|#0:OutdatedClass|}
187+
{
188+
}
189+
}
190+
""")
191+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
192+
.RunAsync(TestContext.CancellationToken);
193+
}
194+
195+
[TestMethod]
196+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_method_type_parameter_constraint_when_marked_for_removal()
197+
{
198+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
199+
.AddSources("""
200+
[PlannedRemoval("1", "Remove this class")]
201+
class OutdatedClass { }
202+
203+
class SomeClass
204+
{
205+
[PlannedRemoval("1", "Needs rework")]
206+
protected void ShouldBeRefactored<T>()
207+
where T : {|#0:OutdatedClass|}
208+
{
209+
}
210+
}
211+
""")
212+
.RunAsync(TestContext.CancellationToken);
213+
}
214+
#endregion method type parameter constraint
215+
216+
#region class type parameter constraint
217+
[TestMethod]
218+
public async Task Warn_when_using_classes_being_removed_as_a_nonprivate_class_type_parameter_constraint_when_marked_for_refactor()
219+
{
220+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
221+
.AddSources("""
222+
[PlannedRemoval("1", "Use NewClass instead")]
223+
public class OutdatedClass { }
224+
225+
[PlannedRefactor("1", "Needs rework")]
226+
public class ShouldBeRefactored<T>
227+
where T : {|#0:OutdatedClass|}
228+
{
229+
}
230+
""")
231+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
232+
.RunAsync(TestContext.CancellationToken);
233+
}
234+
235+
[TestMethod]
236+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_class_type_parameter_constraint_when_marked_for_removal()
237+
{
238+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
239+
.AddSources("""
240+
[PlannedRemoval("1", "Remove this class")]
241+
public class OutdatedClass { }
242+
243+
[PlannedRemoval("1", "Needs rework")]
244+
public class ShouldBeRefactored<T>
245+
where T : {|#0:OutdatedClass|}
246+
{
247+
}
248+
""")
249+
.RunAsync(TestContext.CancellationToken);
250+
}
251+
#endregion class type parameter constraint
252+
253+
#region delegate
254+
[TestMethod]
255+
public async Task Warn_when_using_classes_being_removed_as_a_public_delegate()
256+
{
257+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
258+
.AddSources("""
259+
[PlannedRemoval("1", "Use NewClass instead")]
260+
public class OutdatedClass { }
261+
262+
delegate {|#0:OutdatedClass|} ShouldBeRefactored({|#1:OutdatedClass|} data);
263+
""")
264+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(0).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
265+
.AddExpectedDiagnostics(MustRemovePlannedForRemovalAnalyzer.Rule.AsResult().WithLocation(1).WithArguments("OutdatedClass", "ShouldBeRefactored", "1", "Use NewClass instead"))
266+
.RunAsync(TestContext.CancellationToken);
267+
}
268+
269+
[TestMethod]
270+
public async Task Produce_no_diagnostic_when_using_classes_being_removed_as_a_nonprivate_delegate_when_marked_for_removal()
271+
{
272+
await TestBuilder.TestCSharp<MustRemovePlannedForRemovalAnalyzer>()
273+
.AddSources("""
274+
[PlannedRemoval("1", "Remove this class")]
275+
public class OutdatedClass { }
276+
277+
[PlannedRemoval("1", "Needs rework")]
278+
delegate {|#0:OutdatedClass|} ShouldBeRefactored({|#1:OutdatedClass|} data);
279+
""")
280+
.RunAsync(TestContext.CancellationToken);
281+
}
282+
#endregion delegate
283+
}

‎Analyzers/Analyzers/AnalyzerReleases.Unshipped.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
Rule ID | Category | Severity | Notes
44
---------|--------------------------|----------|--------------------
55
DPDREF01 | DarkPatterns.Refactoring | Error | Indicates that the Annotations and Analyzers have a version mismatch
6-
DPDREF02 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRemoval` is used in a containing symbol that is not marked with the appropriate other attribute
6+
DPDREF02 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRemoval` does not have the corresponding ticket in the manifest
77
DPDREF03 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRefactor` does not have the corresponding ticket in the manifest
8-
DPDREF04 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRemoval` does not have the corresponding ticket in the manifest
8+
DPDREF04 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRemoval` is used in a context that must be `PlannedRefactor`
9+
DPDREF05 | DarkPatterns.Refactoring | Warning | A symbol marked with `PlannedRemoval` is used in a context that must also be `PlannedRemoval`

Analyzers/Analyzers/MustNotUsePlannedForRemovalAnalyzer.cs renamed to Analyzers/Analyzers/MustRefactorPlannedForRemovalAnalyzer.cs

Lines changed: 14 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
using System.Collections.Immutable;
44
using System.Linq;
55
using DarkPatterns.Refactoring.Attributes;
6-
using DarkPatterns.Refactoring.Manifest;
76
using DarkPatterns.Refactoring.Utilities;
87
using Microsoft.CodeAnalysis;
98
using Microsoft.CodeAnalysis.CSharp;
@@ -13,17 +12,16 @@
1312
namespace DarkPatterns.Refactoring;
1413

1514
[DiagnosticAnalyzer(LanguageNames.CSharp)]
16-
public class MustNotUsePlannedForRemovalAnalyzer : DiagnosticAnalyzer
15+
public class MustRefactorPlannedForRemovalAnalyzer : DiagnosticAnalyzer
1716
{
18-
public const string DiagnosticId = "DPDREF02";
17+
public const string DiagnosticId = "DPDREF04";
1918

20-
private static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.MustNotUsePlannedForRemovalTitle), Resources.ResourceManager, typeof(Resources));
21-
private static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.MustNotUsePlannedForRemovalMessageFormat), Resources.ResourceManager, typeof(Resources));
22-
private static readonly LocalizableString Description = new LocalizableResourceString(nameof(Resources.MustNotUsePlannedForRemovalDescription), Resources.ResourceManager, typeof(Resources));
19+
private static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.MustRefactorPlannedForRemovalTitle), Resources.ResourceManager, typeof(Resources));
20+
private static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.MustRefactorPlannedForRemovalMessageFormat), Resources.ResourceManager, typeof(Resources));
21+
private static readonly LocalizableString Description = new LocalizableResourceString(nameof(Resources.MustRefactorPlannedForRemovalDescription), Resources.ResourceManager, typeof(Resources));
2322
private const string Category = "DarkPatterns.Refactoring";
2423

2524
public static readonly DiagnosticDescriptor Rule = new(DiagnosticId, Title, MessageFormat, Category, DiagnosticSeverity.Warning, isEnabledByDefault: true, description: Description);
26-
private static readonly Action<Diagnostic> noopReportDiagnostics = _ => { /* Intentionally not logging here; should be caught by another analyzer */ };
2725

2826
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get { return [Rule]; } }
2927

@@ -32,20 +30,15 @@ public override void Initialize(AnalysisContext context)
3230
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
3331
context.EnableConcurrentExecution();
3432

33+
// uses in
3534
context.RegisterCodeBlockStartAction<SyntaxKind>(codeBlockStartContext =>
3635
{
3736
// Determine owning symbols and gather planned tickets (refactor/removal)
3837

3938
// If the code is planned for refactor, it can use things planned for removal in the same ticket
40-
var refactorTickets =
41-
from symbol in codeBlockStartContext.OwningSymbol.AndAllContainers()
42-
from ticketNumber in symbol.FindAttributes<PlannedRefactorAttribute>(noopReportDiagnostics).Select(attr => attr.TicketNumber)
43-
select ticketNumber;
39+
var refactorTickets = codeBlockStartContext.OwningSymbol.GetAllRefactorAttributes().Select(attr => attr.TicketNumber);
4440
// If the code is planned for removal, it can use things planned for removal in the same ticket
45-
var removalTickets =
46-
from symbol in codeBlockStartContext.OwningSymbol.AndAllContainers()
47-
from ticketNumber in symbol.FindAttributes<PlannedRemovalAttribute>(noopReportDiagnostics).Select(attr => attr.TicketNumber)
48-
select ticketNumber;
41+
var removalTickets = codeBlockStartContext.OwningSymbol.GetAllRemovalAttributes().Select(attr => attr.TicketNumber);
4942

5043
var tickets = refactorTickets.Concat(removalTickets);
5144

@@ -65,25 +58,14 @@ from ticketNumber in symbol.FindAttributes<PlannedRemovalAttribute>(noopReportDi
6558

6659
private static void AnalyzeSymbolNode(SymbolStartAnalysisContext symbolContext)
6760
{
61+
if (symbolContext.Symbol.IsVisible()) return;
62+
6863
// If the code is planned for refactor, it can use things planned for removal in the same ticket
69-
var refactorTickets =
70-
from symbol in symbolContext.Symbol.AndAllContainers()
71-
from ticketNumber in symbol.FindAttributes<PlannedRefactorAttribute>(noopReportDiagnostics).Select(attr => attr.TicketNumber)
72-
select ticketNumber;
64+
var refactorTickets = symbolContext.Symbol.GetAllRefactorAttributes().Select(attr => attr.TicketNumber);
7365
// If the code is planned for removal, it can use things planned for removal in the same ticket
74-
var removalTickets =
75-
from symbol in symbolContext.Symbol.AndAllContainers()
76-
from ticketNumber in symbol.FindAttributes<PlannedRemovalAttribute>(noopReportDiagnostics).Select(attr => attr.TicketNumber)
77-
select ticketNumber;
78-
79-
// "Friend" is `internal`; if the containing symbol is a namespace or assembly, `internal` is the best C# does.
80-
var privateAccessibility = symbolContext.Symbol.ContainingSymbol.Kind is SymbolKind.Namespace or SymbolKind.Assembly
81-
? Accessibility.Friend
82-
: Accessibility.Private;
66+
var removalTickets = symbolContext.Symbol.GetAllRemovalAttributes().Select(attr => attr.TicketNumber);
8367

84-
var tickets = symbolContext.Symbol.DeclaredAccessibility == privateAccessibility
85-
? refactorTickets.Concat(removalTickets)
86-
: removalTickets;
68+
var tickets = refactorTickets.Concat(removalTickets);
8769

8870
symbolContext.RegisterSyntaxNodeAction(ctx =>
8971
{
@@ -109,7 +91,7 @@ private static void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context, ISymbol
10991
continue;
11092

11193
// This doesn't care about the manifest; it requires that the containing type has planned for refactor OR removal using the same TicketNumber
112-
context.ReportDiagnostic(Diagnostic.Create(Rule, context.Node.GetLocation(), symbol.Name, owningSymbol.Name, plannedRemoval.TicketNumber));
94+
context.ReportDiagnostic(Diagnostic.Create(Rule, context.Node.GetLocation(), symbol.Name, owningSymbol.Name, plannedRemoval.TicketNumber, plannedRemoval.RecommendedAlternative));
11395
}
11496
}
11597
}

0 commit comments

Comments
 (0)