diff --git a/Directory.Packages.props b/Directory.Packages.props index 7c87955..ed2ca33 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -2,7 +2,7 @@ true true - true + true diff --git a/README.md b/README.md index 6ebb3d9..f925e46 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ The following warnings are generated by this package: | RP0003 | No dependency rules matched the current project | | RP0004 | Project reference 'x' ==> 'y' violates dependency rule or one of its exceptions | | RP0005 | Package reference 'x' ==> 'y' violates dependency rule or one of its exceptions | +| RP0006 | Tech debt exception 'x' ==> 'y' no longer matches any declared reference and can be removed | ## How to use Add a package reference to the [ReferenceProtector](https://www.nuget.org/packages/ReferenceProtector) package in your projects, or as a common package reference in the repo's [`Directory.Packages.props`](./Directory.Build.props). @@ -72,6 +73,8 @@ Schema of the rules file is as follows: } ``` +Note: for stability - use the version in the schema url, instead of `main`. For example for version `v1.2.3` - use `"$schema": https://raw.githubusercontent.com/olstakh/ReferenceProtector/v1.2.3/src/Build/DependencyRules.schema.json` + Top `ProjectDependencies` object will contain a list of rules to validate against. Each rule has the following schema: - `From` / `To` - Full path regex for source and target projects to be matched. @@ -82,6 +85,8 @@ Top `ProjectDependencies` object will contain a list of rules to validate agains Top `PackageDependences` object will have the same format as `ProjectDependencies` with `LinkType` omitted, since only direct package references will be considered. Also, `Description` section will be part of `RP0005` warning (as opposed to `RP0004`) +Adding `IsTechDebt: true` to an exception (if the `Policy: "Forbidden"`) will mark the exception to the rule as undesireable and analyzer will produce a warning when such an exception no longer holds and can be removed from the rules. + ## Matching logic Each reference between the projects / packages during the build is evaluated against provided list of policies. First each pair of dependencies is evaluated against `From` and `To` patterns, based on their full path. For project dependencies - if the match is successful - their link type is evaluated: if current pair has a direct dependency on each other and `LinkType` value is `Direct` or `DirectOrTransient` - the match is successful, otherwise (the dependency is transient) - `LinkType` should be `Transient` or `DirectOrTransient` for the match to be successful. Package dependencies are only viewed as direct references. Then the exceptions are evaluated using the same pattern matching logic with `From` and `To` fields. The decision logic is as follows diff --git a/samples/DependencyRules.json b/samples/DependencyRules.json index 75b9e4c..9f1c4f2 100644 --- a/samples/DependencyRules.json +++ b/samples/DependencyRules.json @@ -12,13 +12,19 @@ "From": "*\\ClassA.csproj", "To": "*\\ClassB.csproj", "Justification": "This is an exception for testing purposes" + }, + { + "From": "*\\ClassA.csproj", + "To": "*\\OldClass.csproj", + "IsTechDebt": true, + "Justification": "This is a tech debt exception for testing purposes. [Waning is expected]" } ] }, { "From": "*\\ClassA.csproj", "To": "*\\ClassC.csproj", - "Description": "Can't reference this project transitively", + "Description": "Can't reference this project transitively [Warning is expected]", "Policy": "Forbidden", "LinkType": "Transitive", "Exceptions": [ diff --git a/samples/Directory.Build.props b/samples/Directory.Build.props index 47be018..24cb87f 100644 --- a/samples/Directory.Build.props +++ b/samples/Directory.Build.props @@ -8,6 +8,7 @@ true $(MSBuildThisFileDirectory)DependencyRules.json buildResult.sarif,version=2.1 + false \ No newline at end of file diff --git a/src/Analyzers/ReferenceProtector.Analyzers.Tests/ReferenceProtectorAnalyzerTests.cs b/src/Analyzers/ReferenceProtector.Analyzers.Tests/ReferenceProtectorAnalyzerTests.cs index 0b0f207..8219930 100644 --- a/src/Analyzers/ReferenceProtector.Analyzers.Tests/ReferenceProtectorAnalyzerTests.cs +++ b/src/Analyzers/ReferenceProtector.Analyzers.Tests/ReferenceProtectorAnalyzerTests.cs @@ -231,4 +231,229 @@ private AnalyzerTest GetAnalyzer() => }, ReferenceAssemblies = ReferenceAssemblies.Net.Net90 }; + + /// + /// Verifies that the analyzer reports RP0006 when a tech debt exception no longer matches any declared project reference. + /// + [Fact] + public async Task TechDebtException_NoLongerNeeded_ShouldReportDiagnostic_Async() + { + var test = GetAnalyzer(); + test.TestState.AdditionalFiles.Add( + ("DependencyRules.json", """ + { + "ProjectDependencies": [ + { + "From": "*", + "To": "*", + "Description": "No direct project references allowed", + "Policy": "Forbidden", + "LinkType": "Direct", + "Exceptions": [ + { + "From": "TestProject.csproj", + "To": "OldProject.csproj", + "Justification": "Legacy dependency to be removed", + "IsTechDebt": true + } + ] + } + ] + } + """)); + + // OldProject.csproj is NOT in the declared references + test.TestState.AdditionalFiles.Add( + (ReferenceProtectorAnalyzer.DeclaredReferencesFile, """ + TestProject.csproj ProjectReferenceDirect SomeOtherProject.csproj + """)); + + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0004") + .WithNoLocation() + .WithMessage("Project reference 'TestProject.csproj' ==> 'SomeOtherProject.csproj' violates dependency rule 'No direct project references allowed' or one of its exceptions. Please remove the dependency or update 'DependencyRules.json' file to allow it.")); + + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0006") + .WithNoLocation() + .WithMessage("Tech debt exception 'TestProject.csproj' ==> 'OldProject.csproj' in rule 'No direct project references allowed' no longer matches any declared reference and can be removed from 'DependencyRules.json'")); + + await test.RunAsync(TestContext.Current.CancellationToken); + } + + /// + /// Verifies that the analyzer does NOT report RP0006 when a tech debt exception still matches a declared project reference. + /// + [Fact] + public async Task TechDebtException_StillNeeded_ShouldNotReportDiagnostic_Async() + { + var test = GetAnalyzer(); + test.TestState.AdditionalFiles.Add( + ("DependencyRules.json", """ + { + "ProjectDependencies": [ + { + "From": "*", + "To": "*", + "Description": "No direct project references allowed", + "Policy": "Forbidden", + "LinkType": "Direct", + "Exceptions": [ + { + "From": "TestProject.csproj", + "To": "ReferencedProject.csproj", + "Justification": "Legacy dependency to be removed", + "IsTechDebt": true + } + ] + } + ] + } + """)); + + test.TestState.AdditionalFiles.Add( + (ReferenceProtectorAnalyzer.DeclaredReferencesFile, """ + TestProject.csproj ProjectReferenceDirect ReferencedProject.csproj + """)); + + // No diagnostics expected - the tech debt exception is still needed + await test.RunAsync(TestContext.Current.CancellationToken); + } + + /// + /// Verifies that a non-tech-debt exception that no longer matches does NOT trigger RP0006. + /// + [Fact] + public async Task NonTechDebtException_NoLongerNeeded_ShouldNotReportDiagnostic_Async() + { + var test = GetAnalyzer(); + test.TestState.AdditionalFiles.Add( + ("DependencyRules.json", """ + { + "ProjectDependencies": [ + { + "From": "*", + "To": "*", + "Description": "No direct project references allowed", + "Policy": "Forbidden", + "LinkType": "Direct", + "Exceptions": [ + { + "From": "TestProject.csproj", + "To": "OldProject.csproj", + "Justification": "Legitimate exception" + } + ] + } + ] + } + """)); + + test.TestState.AdditionalFiles.Add( + (ReferenceProtectorAnalyzer.DeclaredReferencesFile, """ + TestProject.csproj ProjectReferenceDirect SomeOtherProject.csproj + """)); + + // RP0004 for the violating reference, but NO RP0006 since the exception is not tech debt + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0004") + .WithNoLocation() + .WithMessage("Project reference 'TestProject.csproj' ==> 'SomeOtherProject.csproj' violates dependency rule 'No direct project references allowed' or one of its exceptions. Please remove the dependency or update 'DependencyRules.json' file to allow it.")); + + await test.RunAsync(TestContext.Current.CancellationToken); + } + + /// + /// Verifies that the analyzer reports RP0006 when a tech debt exception for a package reference no longer matches. + /// + [Fact] + public async Task TechDebtPackageException_NoLongerNeeded_ShouldReportDiagnostic_Async() + { + var test = GetAnalyzer(); + test.TestState.AdditionalFiles.Add( + ("DependencyRules.json", """ + { + "PackageDependencies": [ + { + "From": "TestProject.csproj", + "To": "*", + "Description": "No packages allowed", + "Policy": "Forbidden", + "Exceptions": [ + { + "From": "TestProject.csproj", + "To": "OldPackage", + "Justification": "Legacy package to be removed", + "IsTechDebt": true + } + ] + } + ], + "ProjectDependencies": [ + { + "From": "TestProject.csproj", + "To": "*", + "Description": "Allowed", + "Policy": "Allowed", + "LinkType": "DirectOrTransitive" + } + ] + } + """)); + + test.TestState.AdditionalFiles.Add( + (ReferenceProtectorAnalyzer.DeclaredReferencesFile, """ + TestProject.csproj PackageReferenceDirect SomeOtherPackage + """)); + + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0005") + .WithNoLocation() + .WithMessage("Package reference 'TestProject.csproj' ==> 'SomeOtherPackage' violates dependency rule 'No packages allowed' or one of its exceptions. Please remove the dependency or update 'DependencyRules.json' file to allow it.")); + + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0006") + .WithNoLocation() + .WithMessage("Tech debt exception 'TestProject.csproj' ==> 'OldPackage' in rule 'No packages allowed' no longer matches any declared reference and can be removed from 'DependencyRules.json'")); + + await test.RunAsync(TestContext.Current.CancellationToken); + } + + /// + /// Verifies IsTechDebt defaults to false when not specified in JSON. + /// + [Fact] + public async Task TechDebtException_DefaultFalse_NoFlagSpecified_ShouldNotReportDiagnostic_Async() + { + var test = GetAnalyzer(); + test.TestState.AdditionalFiles.Add( + ("DependencyRules.json", """ + { + "ProjectDependencies": [ + { + "From": "*", + "To": "*", + "Description": "No direct project references allowed", + "Policy": "Forbidden", + "LinkType": "Direct", + "Exceptions": [ + { + "From": "TestProject.csproj", + "To": "OldProject.csproj", + "Justification": "Legitimate exception", + "IsTechDebt": false + } + ] + } + ] + } + """)); + + test.TestState.AdditionalFiles.Add( + (ReferenceProtectorAnalyzer.DeclaredReferencesFile, """ + TestProject.csproj ProjectReferenceDirect SomeOtherProject.csproj + """)); + + // Only RP0004 for SomeOtherProject, no RP0006 since IsTechDebt is explicitly false + test.ExpectedDiagnostics.Add(DiagnosticResult.CompilerWarning("RP0004") + .WithNoLocation() + .WithMessage("Project reference 'TestProject.csproj' ==> 'SomeOtherProject.csproj' violates dependency rule 'No direct project references allowed' or one of its exceptions. Please remove the dependency or update 'DependencyRules.json' file to allow it.")); + + await test.RunAsync(TestContext.Current.CancellationToken); + } } diff --git a/src/Analyzers/ReferenceProtector.Analyzers/AnalyzerReleases.Shipped.md b/src/Analyzers/ReferenceProtector.Analyzers/AnalyzerReleases.Shipped.md index 6c9b9c9..bee4377 100644 --- a/src/Analyzers/ReferenceProtector.Analyzers/AnalyzerReleases.Shipped.md +++ b/src/Analyzers/ReferenceProtector.Analyzers/AnalyzerReleases.Shipped.md @@ -12,3 +12,10 @@ RP0002 | Usage | Warning | Make sure the dependency rules file '{0}' is in the c RP0003 | Usage | Info | No dependency rules matched the current project '{0}' RP0004 | Usage | Warning | Project reference '{0}' ==> '{1}' violates dependency rule '{2}' or one of its exceptions RP0005 | Usage | Warning | Package reference '{0}' ==> '{1}' violates dependency rule '{2}' or one of its exceptions + +## Release 1.3.7 + +### New Rules +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +RP0006 | Usage | Warning | Technical debt dependency can be removed from the rules exception \ No newline at end of file diff --git a/src/Analyzers/ReferenceProtector.Analyzers/DiagnosticDescriptors.cs b/src/Analyzers/ReferenceProtector.Analyzers/DiagnosticDescriptors.cs index 619d960..0d71eab 100644 --- a/src/Analyzers/ReferenceProtector.Analyzers/DiagnosticDescriptors.cs +++ b/src/Analyzers/ReferenceProtector.Analyzers/DiagnosticDescriptors.cs @@ -45,4 +45,12 @@ internal static class Descriptors category: "Usage", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); + + public static readonly DiagnosticDescriptor TechDebtExceptionNoLongerNeeded = new( + id: "RP0006", + title: "Tech debt exception is no longer needed", + messageFormat: "Tech debt exception '{0}' ==> '{1}' in rule '{2}' no longer matches any declared reference and can be removed from '{3}'", + category: "Usage", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true); } \ No newline at end of file diff --git a/src/Analyzers/ReferenceProtector.Analyzers/Models/DependencyRules.cs b/src/Analyzers/ReferenceProtector.Analyzers/Models/DependencyRules.cs index 2b3e38d..3533d3c 100644 --- a/src/Analyzers/ReferenceProtector.Analyzers/Models/DependencyRules.cs +++ b/src/Analyzers/ReferenceProtector.Analyzers/Models/DependencyRules.cs @@ -25,7 +25,8 @@ internal record PackageDependency( internal record Exceptions( string From, string To, - string Justification); + string Justification, + bool IsTechDebt = false); internal enum Policy { diff --git a/src/Analyzers/ReferenceProtector.Analyzers/ReferenceProtector.Analyzers.cs b/src/Analyzers/ReferenceProtector.Analyzers/ReferenceProtector.Analyzers.cs index a052ba8..be63a2a 100644 --- a/src/Analyzers/ReferenceProtector.Analyzers/ReferenceProtector.Analyzers.cs +++ b/src/Analyzers/ReferenceProtector.Analyzers/ReferenceProtector.Analyzers.cs @@ -42,7 +42,8 @@ public class ReferenceProtectorAnalyzer : DiagnosticAnalyzer Descriptors.InvalidDependencyRulesFormat, Descriptors.NoDependencyRulesMatchedCurrentProject, Descriptors.ProjectReferenceViolation, - Descriptors.PackageReferenceViolation + Descriptors.PackageReferenceViolation, + Descriptors.TechDebtExceptionNoLongerNeeded ]; /// @@ -151,6 +152,7 @@ private void AnalyzeDependencyRules(CompilationAnalysisContext context) } AnalyzeDeclaredProjectReferences(context, declaredReferences.ToImmutableArray(), thisProjectDependencyRules.ToImmutableArray(), Descriptors.ProjectReferenceViolation, dependencyRulesFile.Path); + ReportStaleTechDebtProjectExceptions(context, declaredReferences.ToImmutableArray(), thisProjectDependencyRules.ToImmutableArray(), dependencyRulesFile.Path); } // Analyze package dependencies @@ -163,6 +165,7 @@ private void AnalyzeDependencyRules(CompilationAnalysisContext context) .Where(r => r.LinkType == ReferenceKind.PackageReferenceDirect); AnalyzeDeclaredPackageReferences(context, packageReferences.ToImmutableArray(), thisPackageDependencyRules.ToImmutableArray(), Descriptors.PackageReferenceViolation, dependencyRulesFile.Path); + ReportStaleTechDebtPackageExceptions(context, packageReferences.ToImmutableArray(), thisPackageDependencyRules.ToImmutableArray(), dependencyRulesFile.Path); } } @@ -252,6 +255,76 @@ private void AnalyzeDeclaredPackageReferences( } } + private void ReportStaleTechDebtProjectExceptions( + CompilationAnalysisContext context, + ImmutableArray declaredReferences, + ImmutableArray dependencyRules, + string dependencyRulesFile) + { + foreach (var rule in dependencyRules) + { + if (rule.Policy != Policy.Forbidden || rule.Exceptions == null) + continue; + + foreach (var exception in rule.Exceptions) + { + if (!exception.IsTechDebt) + continue; + + var exceptionStillNeeded = declaredReferences.Any(reference => + IsMatchByName(exception.From, reference.Source) && + IsMatchByName(exception.To, reference.Target) && + (rule.LinkType != LinkType.Transitive && reference.LinkType == ReferenceKind.ProjectReferenceDirect || + rule.LinkType != LinkType.Direct && reference.LinkType == ReferenceKind.ProjectReferenceTransitive)); + + if (!exceptionStillNeeded) + { + context.ReportDiagnostic(Diagnostic.Create( + Descriptors.TechDebtExceptionNoLongerNeeded, + Location.None, + exception.From, + exception.To, + rule.Description, + dependencyRulesFile)); + } + } + } + } + + private void ReportStaleTechDebtPackageExceptions( + CompilationAnalysisContext context, + ImmutableArray declaredReferences, + ImmutableArray dependencyRules, + string dependencyRulesFile) + { + foreach (var rule in dependencyRules) + { + if (rule.Policy != Policy.Forbidden || rule.Exceptions == null) + continue; + + foreach (var exception in rule.Exceptions) + { + if (!exception.IsTechDebt) + continue; + + var exceptionStillNeeded = declaredReferences.Any(reference => + IsMatchByName(exception.From, reference.Source) && + IsMatchByName(exception.To, reference.Target)); + + if (!exceptionStillNeeded) + { + context.ReportDiagnostic(Diagnostic.Create( + Descriptors.TechDebtExceptionNoLongerNeeded, + Location.None, + exception.From, + exception.To, + rule.Description, + dependencyRulesFile)); + } + } + } + } + private static bool IsMatchByName(string pattern, string project) { var regex = Regex.Escape(pattern).Replace("\\*", ".*") + "$"; diff --git a/src/Build/DependencyRules.schema.json b/src/Build/DependencyRules.schema.json index 5907ce5..ccdac95 100644 --- a/src/Build/DependencyRules.schema.json +++ b/src/Build/DependencyRules.schema.json @@ -138,6 +138,11 @@ "Justification": { "type": "string", "description": "A human-readable justification for why this exception exists." + }, + "IsTechDebt": { + "type": "boolean", + "default": false, + "description": "When true, marks this exception as temporary technical debt. The analyzer will warn (RP0006) when the exception no longer matches any declared reference and can be removed." } } } diff --git a/tests/ReferenceProtector.IntegrationTests/ReferenceProtector.IntegrationTests.cs b/tests/ReferenceProtector.IntegrationTests/ReferenceProtector.IntegrationTests.cs index c4dfeda..36610d7 100644 --- a/tests/ReferenceProtector.IntegrationTests/ReferenceProtector.IntegrationTests.cs +++ b/tests/ReferenceProtector.IntegrationTests/ReferenceProtector.IntegrationTests.cs @@ -169,4 +169,139 @@ public async Task PackageReference_DependencyRuleViolated_FeatureDisabled_NoWarn Assert.Empty(warnings); } + + /// + /// Validates that a stale tech debt exception (one that no longer matches any declared reference) produces an RP0006 warning. + /// + [Fact] + public async Task TechDebtException_NoLongerNeeded_ProducesWarning_Async() + { + var projectA = CreateProject("A"); + var projectB = CreateProject("B"); + await AddProjectReference("A", "B"); + var testRulesPath = Path.Combine(TestDirectory, "testRules.json"); + File.WriteAllText(testRulesPath, $$""" + { + "ProjectDependencies": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "*", + "LinkType": "Direct", + "Policy": "Forbidden", + "Description": "test rule", + "Exceptions": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "{{projectB.Replace("\\", "\\\\")}}", + "Justification": "Current tech debt", + "IsTechDebt": true + }, + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "*OldProject.csproj", + "Justification": "Stale tech debt", + "IsTechDebt": true + } + ] + } + ] + } + """); + + var warnings = await Build(additionalArgs: + $"/p:DependencyRulesFile={testRulesPath}"); + + // The exception for A→B still matches, so no RP0006 for it. + // The exception for A→*OldProject.csproj is stale, so RP0006 is expected. + var warning = Assert.Single(warnings); + Assert.Equal(new Warning() + { + Message = $"RP0006: Tech debt exception '{projectA}' ==> '*OldProject.csproj' in rule 'test rule' no longer matches any declared reference and can be removed from '{testRulesPath}'", + Project = "A/A.csproj", + }, warning); + } + + /// + /// Validates that a tech debt exception that still matches a declared reference does NOT produce an RP0006 warning. + /// + [Fact] + public async Task TechDebtException_StillNeeded_NoWarning_Async() + { + var projectA = CreateProject("A"); + var projectB = CreateProject("B"); + await AddProjectReference("A", "B"); + var testRulesPath = Path.Combine(TestDirectory, "testRules.json"); + File.WriteAllText(testRulesPath, $$""" + { + "ProjectDependencies": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "*", + "LinkType": "Direct", + "Policy": "Forbidden", + "Description": "test rule", + "Exceptions": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "{{projectB.Replace("\\", "\\\\")}}", + "Justification": "Current tech debt - still needed", + "IsTechDebt": true + } + ] + } + ] + } + """); + + var warnings = await Build(additionalArgs: + $"/p:DependencyRulesFile={testRulesPath}"); + + // The tech debt exception still matches A→B, so no RP0006. + // And the exception suppresses the RP0004 violation. + Assert.Empty(warnings); + } + + /// + /// Validates that a non-tech-debt exception that no longer matches does NOT produce an RP0006 warning. + /// + [Fact] + public async Task NonTechDebtException_NoLongerNeeded_NoWarning_Async() + { + var projectA = CreateProject("A"); + var projectB = CreateProject("B"); + await AddProjectReference("A", "B"); + var testRulesPath = Path.Combine(TestDirectory, "testRules.json"); + File.WriteAllText(testRulesPath, $$""" + { + "ProjectDependencies": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "*", + "LinkType": "Direct", + "Policy": "Forbidden", + "Description": "test rule", + "Exceptions": [ + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "{{projectB.Replace("\\", "\\\\")}}", + "Justification": "Legitimate exception" + }, + { + "From": "{{projectA.Replace("\\", "\\\\")}}", + "To": "*OldProject.csproj", + "Justification": "Legitimate exception, not tech debt" + } + ] + } + ] + } + """); + + var warnings = await Build(additionalArgs: + $"/p:DependencyRulesFile={testRulesPath}"); + + // No RP0006 because neither exception has IsTechDebt=true. + // A→B is covered by the first exception, so no RP0004 either. + Assert.Empty(warnings); + } } \ No newline at end of file