Skip to content

Commit 690fd30

Browse files
randleeclaude
andcommitted
feat(core): Integrate impact classification into SemanticComparer
Phase 2 of non-impactful change detection: - Integrate VisibilityExtractor and ImpactClassifier into SemanticComparer - Add impact, visibility, and caveats to renamed/moved changes - Add IncludeNonImpactful and MinimumImpactLevel to DiffOptions - Add impact breakdown stats to DiffStats (BreakingPublicApiCount, etc.) - Add 5 integration tests for impact classification scenarios Test: 849 tests pass (100%) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 6476acd commit 690fd30

File tree

5 files changed

+304
-3
lines changed

5 files changed

+304
-3
lines changed

src/RoslynDiff.Core/Comparison/SemanticComparer.cs

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ private void DetectRenames(
299299
continue;
300300
}
301301

302-
// Create a Renamed change
302+
// Create a Renamed change (without impact initially for classification)
303303
var renamedChange = new Change
304304
{
305305
Type = ChangeType.Renamed,
@@ -312,6 +312,15 @@ private void DetectRenames(
312312
NewContent = candidate.AddedChange.NewContent
313313
};
314314

315+
// Classify the impact of the rename
316+
var (impact, visibility, caveats) = ClassifyChangeImpact(renamedChange, oldTree, newTree);
317+
renamedChange = renamedChange with
318+
{
319+
Impact = impact,
320+
Visibility = visibility,
321+
Caveats = caveats
322+
};
323+
315324
replacements[candidate.RemovedChange] = renamedChange;
316325
matchedRemoved.Add(candidate.RemovedChange);
317326
matchedAdded.Add(candidate.AddedChange);
@@ -342,7 +351,7 @@ private void DetectMoves(
342351
continue;
343352
}
344353

345-
// Create a Moved change
354+
// Create a Moved change (without impact initially for classification)
346355
var movedChange = new Change
347356
{
348357
Type = ChangeType.Moved,
@@ -354,9 +363,82 @@ private void DetectMoves(
354363
NewContent = candidate.AddedChange.NewContent
355364
};
356365

366+
// Classify the impact of the move (same-parent moves are non-breaking)
367+
var (impact, visibility, caveats) = ClassifyChangeImpact(
368+
movedChange,
369+
oldTree,
370+
newTree,
371+
isSameScopeMove: candidate.IsSameParent);
372+
movedChange = movedChange with
373+
{
374+
Impact = impact,
375+
Visibility = visibility,
376+
Caveats = caveats
377+
};
378+
357379
replacements[candidate.RemovedChange] = movedChange;
358380
matchedRemoved.Add(candidate.RemovedChange);
359381
matchedAdded.Add(candidate.AddedChange);
360382
}
361383
}
384+
385+
/// <summary>
386+
/// Maps a ChangeKind to the corresponding SymbolKind for impact classification.
387+
/// </summary>
388+
private static Models.SymbolKind MapToSymbolKind(ChangeKind changeKind)
389+
{
390+
return changeKind switch
391+
{
392+
ChangeKind.Namespace => Models.SymbolKind.Namespace,
393+
ChangeKind.Class => Models.SymbolKind.Type,
394+
ChangeKind.Method => Models.SymbolKind.Method,
395+
ChangeKind.Property => Models.SymbolKind.Property,
396+
ChangeKind.Field => Models.SymbolKind.Field,
397+
ChangeKind.Statement => Models.SymbolKind.Local,
398+
ChangeKind.Line => Models.SymbolKind.Unknown,
399+
ChangeKind.File => Models.SymbolKind.Unknown,
400+
_ => Models.SymbolKind.Unknown
401+
};
402+
}
403+
404+
/// <summary>
405+
/// Classifies the impact of a change and returns updated Impact, Visibility, and Caveats.
406+
/// </summary>
407+
private static (ChangeImpact Impact, Visibility Visibility, IReadOnlyList<string>? Caveats) ClassifyChangeImpact(
408+
Change change,
409+
SyntaxTree? oldTree,
410+
SyntaxTree? newTree,
411+
bool isSameScopeMove = false)
412+
{
413+
// Try to get the syntax node for visibility extraction
414+
SyntaxNode? node = null;
415+
416+
// For renames and moves, prefer the old location for visibility (original declaration)
417+
if (oldTree is not null && change.OldLocation is not null)
418+
{
419+
node = SymbolMatcher.FindNodeAtLocation(oldTree, change.OldLocation);
420+
}
421+
else if (newTree is not null && change.NewLocation is not null)
422+
{
423+
node = SymbolMatcher.FindNodeAtLocation(newTree, change.NewLocation);
424+
}
425+
426+
// Extract visibility (default to Private if no node found)
427+
var visibility = node is not null
428+
? VisibilityExtractor.Extract(node)
429+
: Visibility.Private;
430+
431+
// Map ChangeKind to SymbolKind
432+
var symbolKind = MapToSymbolKind(change.Kind);
433+
434+
// Classify the impact
435+
var (impact, caveats) = ImpactClassifier.Classify(
436+
change.Type,
437+
symbolKind,
438+
visibility,
439+
isSignatureChange: true, // Renames/moves are treated as signature changes
440+
isSameScopeMove: isSameScopeMove);
441+
442+
return (impact, visibility, caveats.Count > 0 ? caveats : null);
443+
}
362444
}

src/RoslynDiff.Core/Comparison/SymbolMatcher.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ private static string GetParameterTypesSignature(ParameterListSyntax? parameterL
247247
/// <summary>
248248
/// Finds a syntax node at a given location in a tree.
249249
/// </summary>
250-
private static SyntaxNode? FindNodeAtLocation(SyntaxTree tree, SourceLocation? location)
250+
internal static SyntaxNode? FindNodeAtLocation(SyntaxTree tree, SourceLocation? location)
251251
{
252252
if (location is null)
253253
return null;

src/RoslynDiff.Core/Models/DiffOptions.cs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,26 @@ public record DiffOptions
4747
/// This is used for display purposes and to determine the file type for automatic mode selection.
4848
/// </remarks>
4949
public string? NewPath { get; init; }
50+
51+
/// <summary>
52+
/// Gets a value indicating whether non-impactful changes should be included in results.
53+
/// </summary>
54+
/// <remarks>
55+
/// When <c>true</c>, all changes are included regardless of impact level.
56+
/// When <c>false</c>, non-breaking and formatting-only changes are filtered out.
57+
/// Default is <c>true</c> for the core library; output formatters may apply their own filtering.
58+
/// </remarks>
59+
public bool IncludeNonImpactful { get; init; } = true;
60+
61+
/// <summary>
62+
/// Gets the minimum impact level to include in results.
63+
/// </summary>
64+
/// <remarks>
65+
/// Only changes with an impact level at or above this threshold are included.
66+
/// The hierarchy from most to least impactful is:
67+
/// <see cref="ChangeImpact.BreakingPublicApi"/> > <see cref="ChangeImpact.BreakingInternalApi"/>
68+
/// > <see cref="ChangeImpact.NonBreaking"/> > <see cref="ChangeImpact.FormattingOnly"/>.
69+
/// Default includes all changes.
70+
/// </remarks>
71+
public ChangeImpact MinimumImpactLevel { get; init; } = ChangeImpact.FormattingOnly;
5072
}

src/RoslynDiff.Core/Models/DiffResult.cs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,41 @@ public record DiffStats
8585
/// Gets the number of renamed symbols or elements.
8686
/// </summary>
8787
public int Renames { get; init; }
88+
89+
// Impact breakdown statistics
90+
91+
/// <summary>
92+
/// Gets the number of breaking public API changes.
93+
/// </summary>
94+
public int BreakingPublicApiCount { get; init; }
95+
96+
/// <summary>
97+
/// Gets the number of breaking internal API changes.
98+
/// </summary>
99+
public int BreakingInternalApiCount { get; init; }
100+
101+
/// <summary>
102+
/// Gets the number of non-breaking changes.
103+
/// </summary>
104+
public int NonBreakingCount { get; init; }
105+
106+
/// <summary>
107+
/// Gets the number of formatting-only changes.
108+
/// </summary>
109+
public int FormattingOnlyCount { get; init; }
110+
111+
/// <summary>
112+
/// Gets a value indicating whether there are any breaking changes (public or internal API).
113+
/// </summary>
114+
public bool HasBreakingChanges => BreakingPublicApiCount > 0 || BreakingInternalApiCount > 0;
115+
116+
/// <summary>
117+
/// Gets a value indicating whether the changes require review.
118+
/// </summary>
119+
/// <remarks>
120+
/// Returns <c>true</c> if there are any changes that are not purely formatting-only.
121+
/// </remarks>
122+
public bool RequiresReview => BreakingPublicApiCount > 0 || BreakingInternalApiCount > 0 || NonBreakingCount > 0;
88123
}
89124

90125
/// <summary>

tests/RoslynDiff.Core.Tests/SemanticComparerTests.cs

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,4 +554,166 @@ public void AddedMethod()
554554
}
555555

556556
#endregion
557+
558+
#region Impact Classification
559+
560+
[Fact]
561+
public void EnhanceWithSemantics_PublicMethodRenamed_ClassifiesAsBreakingPublicApi()
562+
{
563+
var oldCode = """
564+
namespace Test;
565+
public class Calculator
566+
{
567+
public int OldMethod(int a, int b) => a + b;
568+
}
569+
""";
570+
var newCode = """
571+
namespace Test;
572+
public class Calculator
573+
{
574+
public int NewMethod(int a, int b) => a + b;
575+
}
576+
""";
577+
var options = new DiffOptions();
578+
579+
var syntaxChanges = _syntaxComparer.CompareSource(oldCode, newCode, options);
580+
var result = _semanticComparer.EnhanceWithSemantics(syntaxChanges, oldCode, newCode, options);
581+
582+
var allChanges = FlattenAllChanges(result);
583+
var renamed = allChanges.FirstOrDefault(c => c.Type == ChangeType.Renamed && c.Kind == ChangeKind.Method);
584+
585+
renamed.Should().NotBeNull();
586+
renamed!.Impact.Should().Be(ChangeImpact.BreakingPublicApi);
587+
renamed.Visibility.Should().Be(Visibility.Public);
588+
renamed.Caveats.Should().BeNullOrEmpty();
589+
}
590+
591+
[Fact]
592+
public void EnhanceWithSemantics_PrivateMethodRenamed_ClassifiesAsNonBreaking()
593+
{
594+
var oldCode = """
595+
namespace Test;
596+
public class Calculator
597+
{
598+
private int OldPrivateMethod(int a, int b) => a + b;
599+
}
600+
""";
601+
var newCode = """
602+
namespace Test;
603+
public class Calculator
604+
{
605+
private int NewPrivateMethod(int a, int b) => a + b;
606+
}
607+
""";
608+
var options = new DiffOptions();
609+
610+
var syntaxChanges = _syntaxComparer.CompareSource(oldCode, newCode, options);
611+
var result = _semanticComparer.EnhanceWithSemantics(syntaxChanges, oldCode, newCode, options);
612+
613+
var allChanges = FlattenAllChanges(result);
614+
var renamed = allChanges.FirstOrDefault(c => c.Type == ChangeType.Renamed && c.Kind == ChangeKind.Method);
615+
616+
renamed.Should().NotBeNull();
617+
renamed!.Impact.Should().Be(ChangeImpact.NonBreaking);
618+
renamed.Visibility.Should().Be(Visibility.Private);
619+
renamed.Caveats.Should().NotBeNull();
620+
renamed.Caveats.Should().Contain(c => c.Contains("reflection") || c.Contains("serialization"));
621+
}
622+
623+
[Fact]
624+
public void EnhanceWithSemantics_InternalMethodRenamed_ClassifiesAsBreakingInternalApi()
625+
{
626+
var oldCode = """
627+
namespace Test;
628+
internal class Calculator
629+
{
630+
internal int OldMethod(int a, int b) => a + b;
631+
}
632+
""";
633+
var newCode = """
634+
namespace Test;
635+
internal class Calculator
636+
{
637+
internal int NewMethod(int a, int b) => a + b;
638+
}
639+
""";
640+
var options = new DiffOptions();
641+
642+
var syntaxChanges = _syntaxComparer.CompareSource(oldCode, newCode, options);
643+
var result = _semanticComparer.EnhanceWithSemantics(syntaxChanges, oldCode, newCode, options);
644+
645+
var allChanges = FlattenAllChanges(result);
646+
var renamed = allChanges.FirstOrDefault(c => c.Type == ChangeType.Renamed && c.Kind == ChangeKind.Method);
647+
648+
renamed.Should().NotBeNull();
649+
renamed!.Impact.Should().Be(ChangeImpact.BreakingInternalApi);
650+
renamed.Visibility.Should().Be(Visibility.Internal);
651+
}
652+
653+
[Fact]
654+
public void EnhanceWithSemantics_MethodMovedWithinSameClass_ClassifiesAsNonBreaking()
655+
{
656+
var oldCode = """
657+
namespace Test;
658+
public class Calculator
659+
{
660+
public int Add(int a, int b) => a + b;
661+
public int Multiply(int a, int b) => a * b;
662+
}
663+
""";
664+
// Same class, just reordered
665+
var newCode = """
666+
namespace Test;
667+
public class Calculator
668+
{
669+
public int Multiply(int a, int b) => a * b;
670+
public int Add(int a, int b) => a + b;
671+
}
672+
""";
673+
var options = new DiffOptions();
674+
675+
var syntaxChanges = _syntaxComparer.CompareSource(oldCode, newCode, options);
676+
var result = _semanticComparer.EnhanceWithSemantics(syntaxChanges, oldCode, newCode, options);
677+
678+
var allChanges = FlattenAllChanges(result);
679+
680+
// Reordering within the same class should produce either no changes or Moved with NonBreaking
681+
var moved = allChanges.FirstOrDefault(c => c.Type == ChangeType.Moved);
682+
if (moved is not null)
683+
{
684+
moved.Impact.Should().Be(ChangeImpact.NonBreaking);
685+
}
686+
}
687+
688+
[Fact]
689+
public void EnhanceWithSemantics_PublicClassRenamed_ClassifiesAsBreakingPublicApi()
690+
{
691+
var oldCode = """
692+
namespace Test;
693+
public class OldClassName
694+
{
695+
public int Value { get; set; }
696+
}
697+
""";
698+
var newCode = """
699+
namespace Test;
700+
public class NewClassName
701+
{
702+
public int Value { get; set; }
703+
}
704+
""";
705+
var options = new DiffOptions();
706+
707+
var syntaxChanges = _syntaxComparer.CompareSource(oldCode, newCode, options);
708+
var result = _semanticComparer.EnhanceWithSemantics(syntaxChanges, oldCode, newCode, options);
709+
710+
var allChanges = FlattenAllChanges(result);
711+
var renamed = allChanges.FirstOrDefault(c => c.Type == ChangeType.Renamed && c.Kind == ChangeKind.Class);
712+
713+
renamed.Should().NotBeNull();
714+
renamed!.Impact.Should().Be(ChangeImpact.BreakingPublicApi);
715+
renamed.Visibility.Should().Be(Visibility.Public);
716+
}
717+
718+
#endregion
557719
}

0 commit comments

Comments
 (0)