Skip to content

Commit 6476acd

Browse files
randleeclaude
andcommitted
feat(core): Add non-impactful change detection infrastructure
- Add ChangeImpact enum (BreakingPublicApi, BreakingInternalApi, NonBreaking, FormattingOnly) - Add Visibility enum for symbol accessibility tracking - Add SymbolKind enum for categorizing symbol types - Update Change record with Impact, Visibility, and Caveats properties - Add VisibilityExtractor to extract visibility from Roslyn syntax nodes - Add ImpactClassifier to classify change impact based on visibility rules - Add comprehensive unit tests (59 new tests) Part of DESIGN-004: Non-Impactful Change Detection Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent d025e91 commit 6476acd

File tree

8 files changed

+1387
-0
lines changed

8 files changed

+1387
-0
lines changed
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
using RoslynDiff.Core.Models;
2+
3+
namespace RoslynDiff.Core.Comparison;
4+
5+
/// <summary>
6+
/// Classifies the impact level of code changes based on visibility and change type.
7+
/// </summary>
8+
public static class ImpactClassifier
9+
{
10+
/// <summary>
11+
/// Classifies the impact of a change.
12+
/// </summary>
13+
/// <param name="changeType">The type of change (Added, Removed, Modified, Renamed, Moved).</param>
14+
/// <param name="symbolKind">The kind of symbol being changed.</param>
15+
/// <param name="visibility">The visibility of the symbol.</param>
16+
/// <param name="isSignatureChange">Whether this is a signature change vs body-only change.</param>
17+
/// <param name="isSameScopeMove">For moves, whether it's within the same containing type.</param>
18+
/// <returns>A tuple of (impact level, list of caveats).</returns>
19+
public static (ChangeImpact Impact, List<string> Caveats) Classify(
20+
ChangeType changeType,
21+
SymbolKind symbolKind,
22+
Visibility visibility,
23+
bool isSignatureChange = true,
24+
bool isSameScopeMove = false)
25+
{
26+
var caveats = new List<string>();
27+
28+
// Renamed symbols
29+
if (changeType == ChangeType.Renamed)
30+
{
31+
return ClassifyRename(symbolKind, visibility, caveats);
32+
}
33+
34+
// Moved symbols
35+
if (changeType == ChangeType.Moved)
36+
{
37+
return ClassifyMove(visibility, isSameScopeMove, caveats);
38+
}
39+
40+
// Added/Removed/Modified
41+
return ClassifyModification(changeType, visibility, isSignatureChange, caveats);
42+
}
43+
44+
private static (ChangeImpact, List<string>) ClassifyRename(
45+
SymbolKind symbolKind,
46+
Visibility visibility,
47+
List<string> caveats)
48+
{
49+
// Public API renames are breaking
50+
if (VisibilityExtractor.IsPublicApi(visibility))
51+
{
52+
return (ChangeImpact.BreakingPublicApi, caveats);
53+
}
54+
55+
// Internal API renames are breaking for internal consumers
56+
if (VisibilityExtractor.IsInternalApi(visibility))
57+
{
58+
return (ChangeImpact.BreakingInternalApi, caveats);
59+
}
60+
61+
// Parameter renames have a caveat about named arguments
62+
if (symbolKind == SymbolKind.Parameter)
63+
{
64+
caveats.Add("Parameter rename may break callers using named arguments");
65+
}
66+
67+
// Private member renames have a caveat about reflection/serialization
68+
if (visibility == Visibility.Private && symbolKind is SymbolKind.Field or SymbolKind.Property or SymbolKind.Method)
69+
{
70+
caveats.Add("Private member rename may break reflection or serialization");
71+
}
72+
73+
return (ChangeImpact.NonBreaking, caveats);
74+
}
75+
76+
private static (ChangeImpact, List<string>) ClassifyMove(
77+
Visibility visibility,
78+
bool isSameScopeMove,
79+
List<string> caveats)
80+
{
81+
// Same-scope moves are non-breaking (just reordering)
82+
if (isSameScopeMove)
83+
{
84+
caveats.Add("Code reordering within same scope");
85+
return (ChangeImpact.NonBreaking, caveats);
86+
}
87+
88+
// Cross-scope moves follow visibility rules
89+
if (VisibilityExtractor.IsPublicApi(visibility))
90+
{
91+
return (ChangeImpact.BreakingPublicApi, caveats);
92+
}
93+
94+
if (VisibilityExtractor.IsInternalApi(visibility))
95+
{
96+
return (ChangeImpact.BreakingInternalApi, caveats);
97+
}
98+
99+
return (ChangeImpact.NonBreaking, caveats);
100+
}
101+
102+
private static (ChangeImpact, List<string>) ClassifyModification(
103+
ChangeType changeType,
104+
Visibility visibility,
105+
bool isSignatureChange,
106+
List<string> caveats)
107+
{
108+
// Body-only modifications are non-breaking
109+
if (changeType == ChangeType.Modified && !isSignatureChange)
110+
{
111+
return (ChangeImpact.NonBreaking, caveats);
112+
}
113+
114+
// Signature changes and additions/removals follow visibility rules
115+
if (VisibilityExtractor.IsPublicApi(visibility))
116+
{
117+
return (ChangeImpact.BreakingPublicApi, caveats);
118+
}
119+
120+
if (VisibilityExtractor.IsInternalApi(visibility))
121+
{
122+
return (ChangeImpact.BreakingInternalApi, caveats);
123+
}
124+
125+
// Private changes are non-breaking
126+
return (ChangeImpact.NonBreaking, caveats);
127+
}
128+
129+
/// <summary>
130+
/// Determines if a change should be considered formatting-only.
131+
/// </summary>
132+
public static bool IsFormattingOnly(string? oldContent, string? newContent)
133+
{
134+
if (oldContent == null || newContent == null)
135+
return false;
136+
137+
// Normalize whitespace and compare
138+
var normalizedOld = NormalizeWhitespace(oldContent);
139+
var normalizedNew = NormalizeWhitespace(newContent);
140+
141+
return normalizedOld == normalizedNew;
142+
}
143+
144+
private static string NormalizeWhitespace(string content)
145+
{
146+
// Remove all whitespace for comparison
147+
return string.Concat(content.Where(c => !char.IsWhiteSpace(c)));
148+
}
149+
}
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
using Microsoft.CodeAnalysis;
2+
using Microsoft.CodeAnalysis.CSharp;
3+
using Microsoft.CodeAnalysis.CSharp.Syntax;
4+
using RoslynDiff.Core.Models;
5+
6+
namespace RoslynDiff.Core.Comparison;
7+
8+
/// <summary>
9+
/// Extracts visibility/accessibility information from Roslyn syntax nodes.
10+
/// </summary>
11+
public static class VisibilityExtractor
12+
{
13+
/// <summary>
14+
/// Extracts the visibility from a syntax node.
15+
/// </summary>
16+
public static Visibility Extract(SyntaxNode node)
17+
{
18+
return node switch
19+
{
20+
MemberDeclarationSyntax member => ExtractFromMember(member),
21+
ParameterSyntax => Visibility.Local,
22+
LocalDeclarationStatementSyntax => Visibility.Local,
23+
VariableDeclaratorSyntax => Visibility.Local,
24+
_ => Visibility.Private // Default for unknown nodes
25+
};
26+
}
27+
28+
/// <summary>
29+
/// Extracts visibility from a member declaration.
30+
/// </summary>
31+
private static Visibility ExtractFromMember(MemberDeclarationSyntax member)
32+
{
33+
var modifiers = member.Modifiers;
34+
35+
bool hasPublic = modifiers.Any(SyntaxKind.PublicKeyword);
36+
bool hasPrivate = modifiers.Any(SyntaxKind.PrivateKeyword);
37+
bool hasProtected = modifiers.Any(SyntaxKind.ProtectedKeyword);
38+
bool hasInternal = modifiers.Any(SyntaxKind.InternalKeyword);
39+
40+
// Check combinations first
41+
if (hasProtected && hasInternal)
42+
return Visibility.ProtectedInternal;
43+
if (hasPrivate && hasProtected)
44+
return Visibility.PrivateProtected;
45+
46+
// Single modifiers
47+
if (hasPublic)
48+
return Visibility.Public;
49+
if (hasProtected)
50+
return Visibility.Protected;
51+
if (hasInternal)
52+
return Visibility.Internal;
53+
if (hasPrivate)
54+
return Visibility.Private;
55+
56+
// Default visibility depends on containing context
57+
return GetDefaultVisibility(member);
58+
}
59+
60+
/// <summary>
61+
/// Gets the default visibility when no explicit modifier is specified.
62+
/// </summary>
63+
private static Visibility GetDefaultVisibility(MemberDeclarationSyntax member)
64+
{
65+
// Top-level types default to internal
66+
if (member is TypeDeclarationSyntax && member.Parent is CompilationUnitSyntax)
67+
return Visibility.Internal;
68+
69+
// Interface members default to public (check BEFORE TypeDeclarationSyntax since interface IS a type)
70+
if (member.Parent is InterfaceDeclarationSyntax)
71+
return Visibility.Public;
72+
73+
// Nested types and members default to private
74+
if (member.Parent is TypeDeclarationSyntax)
75+
return Visibility.Private;
76+
77+
// Enum members are public
78+
if (member is EnumMemberDeclarationSyntax)
79+
return Visibility.Public;
80+
81+
return Visibility.Private;
82+
}
83+
84+
/// <summary>
85+
/// Determines if a visibility level is considered public API.
86+
/// </summary>
87+
public static bool IsPublicApi(Visibility visibility)
88+
{
89+
return visibility is Visibility.Public or Visibility.Protected or Visibility.ProtectedInternal;
90+
}
91+
92+
/// <summary>
93+
/// Determines if a visibility level is considered internal API.
94+
/// </summary>
95+
public static bool IsInternalApi(Visibility visibility)
96+
{
97+
return visibility is Visibility.Internal or Visibility.PrivateProtected;
98+
}
99+
}

src/RoslynDiff.Core/Models/Change.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,30 @@ public record Change
7878
/// can have child changes representing modified methods, properties, or fields within it.
7979
/// </remarks>
8080
public IReadOnlyList<Change>? Children { get; init; }
81+
82+
/// <summary>
83+
/// Gets the impact level of this change.
84+
/// </summary>
85+
/// <remarks>
86+
/// Indicates whether this change affects public API, internal API, or is non-breaking.
87+
/// </remarks>
88+
public ChangeImpact Impact { get; init; }
89+
90+
/// <summary>
91+
/// Gets the visibility/accessibility of the affected symbol.
92+
/// </summary>
93+
/// <remarks>
94+
/// This is <c>null</c> for changes that don't involve symbols with visibility modifiers
95+
/// (e.g., line-based diffs or certain statement-level changes).
96+
/// </remarks>
97+
public Visibility? Visibility { get; init; }
98+
99+
/// <summary>
100+
/// Gets any caveats or warnings about the impact classification.
101+
/// </summary>
102+
/// <remarks>
103+
/// Caveats provide additional context about the impact analysis, such as cases where
104+
/// the impact might be understated or where further review is recommended.
105+
/// </remarks>
106+
public IReadOnlyList<string>? Caveats { get; init; }
81107
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
namespace RoslynDiff.Core.Models;
2+
3+
/// <summary>
4+
/// Categorizes the impact level of a code change.
5+
/// </summary>
6+
public enum ChangeImpact
7+
{
8+
/// <summary>
9+
/// Changes to public API surface that could break external consumers.
10+
/// </summary>
11+
BreakingPublicApi,
12+
13+
/// <summary>
14+
/// Changes to internal API surface that could break internal consumers.
15+
/// </summary>
16+
BreakingInternalApi,
17+
18+
/// <summary>
19+
/// Changes that don't affect code execution or API contracts.
20+
/// </summary>
21+
NonBreaking,
22+
23+
/// <summary>
24+
/// Whitespace-only or comment-only changes.
25+
/// </summary>
26+
FormattingOnly
27+
}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
namespace RoslynDiff.Core.Models;
2+
3+
/// <summary>
4+
/// Specifies the kind of symbol being analyzed for impact classification.
5+
/// </summary>
6+
public enum SymbolKind
7+
{
8+
/// <summary>
9+
/// A namespace declaration.
10+
/// </summary>
11+
Namespace,
12+
13+
/// <summary>
14+
/// A type declaration (class, struct, record, interface, enum).
15+
/// </summary>
16+
Type,
17+
18+
/// <summary>
19+
/// A method or function declaration.
20+
/// </summary>
21+
Method,
22+
23+
/// <summary>
24+
/// A property declaration.
25+
/// </summary>
26+
Property,
27+
28+
/// <summary>
29+
/// A field declaration.
30+
/// </summary>
31+
Field,
32+
33+
/// <summary>
34+
/// An event declaration.
35+
/// </summary>
36+
Event,
37+
38+
/// <summary>
39+
/// A parameter in a method or constructor.
40+
/// </summary>
41+
Parameter,
42+
43+
/// <summary>
44+
/// A local variable.
45+
/// </summary>
46+
Local,
47+
48+
/// <summary>
49+
/// A constructor.
50+
/// </summary>
51+
Constructor,
52+
53+
/// <summary>
54+
/// An indexer.
55+
/// </summary>
56+
Indexer,
57+
58+
/// <summary>
59+
/// An operator overload.
60+
/// </summary>
61+
Operator,
62+
63+
/// <summary>
64+
/// A delegate declaration.
65+
/// </summary>
66+
Delegate,
67+
68+
/// <summary>
69+
/// An enum member.
70+
/// </summary>
71+
EnumMember,
72+
73+
/// <summary>
74+
/// Unknown or unclassified symbol.
75+
/// </summary>
76+
Unknown
77+
}

0 commit comments

Comments
 (0)