Skip to content

Commit

Permalink
Merge pull request #1181 from sharwell/faded-locations
Browse files Browse the repository at this point in the history 
Add validation for additional unnecessary locations
  • Loading branch information
@sharwell
sharwell authored Aug 30, 2024
2 parents bc8a063 + cba08e2 commit 69e2fa7
Show file tree
Hide file tree
Showing 6 changed files with 239 additions and 21 deletions.
80 changes: 62 additions & 18 deletions src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using DiffPlex;
Expand All @@ -34,6 +35,7 @@ public abstract class AnalyzerTest<TVerifier>
where TVerifier : IVerifier, new()
{
private static readonly ConditionalWeakTable<Diagnostic, object> NonLocalDiagnostics = new ConditionalWeakTable<Diagnostic, object>();
private static readonly Regex EncodedIndicesSyntax = new Regex(@"^\s*\[\s*((?<Index>[0-9]+)\s*(,\s*(?<Index>[0-9]+)\s*)*)?\]\s*$", RegexOptions.ExplicitCapture | RegexOptions.Compiled);

/// <summary>
/// Gets the default verifier for the test.
Expand Down Expand Up @@ -558,16 +560,43 @@ private void VerifyDiagnosticResults(IEnumerable<(Project project, Diagnostic di
else
{
VerifyDiagnosticLocation(analyzers, actual.diagnostic, expected, actual.diagnostic.Location, expected.Spans[0], verifier);
int[] unnecessaryIndices = { };
if (actual.diagnostic.Properties.TryGetValue(WellKnownDiagnosticTags.Unnecessary, out var encodedUnnecessaryLocations))
{
verifier.True(actual.diagnostic.Descriptor.CustomTags.Contains(WellKnownDiagnosticTags.Unnecessary), "Diagnostic reported extended unnecessary locations, but the descriptor is not marked as unnecessary code.");
var match = EncodedIndicesSyntax.Match(encodedUnnecessaryLocations);
verifier.True(match.Success, $"Expected encoded unnecessary locations to be a valid JSON array of non-negative integers: {encodedUnnecessaryLocations}");
unnecessaryIndices = match.Groups["Index"].Captures.OfType<Capture>().Select(capture => int.Parse(capture.Value)).ToArray();
verifier.NotEmpty(nameof(unnecessaryIndices), unnecessaryIndices);
foreach (var index in unnecessaryIndices)
{
if (index < 0 || index >= actual.diagnostic.AdditionalLocations.Count)
{
verifier.Fail($"All unnecessary indices in the diagnostic must be valid indices in AdditionalLocations [0-{actual.diagnostic.AdditionalLocations.Count}): {encodedUnnecessaryLocations}");
}
}
}

if (!expected.Options.HasFlag(DiagnosticOptions.IgnoreAdditionalLocations))
{
var additionalLocations = actual.diagnostic.AdditionalLocations.ToArray();

message = FormatVerifierMessage(analyzers, actual.diagnostic, expected, $"Expected {expected.Spans.Length - 1} additional locations but got {additionalLocations.Length} for Diagnostic:");
verifier.Equal(expected.Spans.Length - 1, additionalLocations.Length, message);

for (var j = 0; j < additionalLocations.Length; ++j)
for (var j = 0; j < additionalLocations.Length; j++)
{
VerifyDiagnosticLocation(analyzers, actual.diagnostic, expected, additionalLocations[j], expected.Spans[j + 1], verifier);
var isActualUnnecessary = unnecessaryIndices.Contains(j);
var isExpectedUnnecessary = expected.Spans[j + 1].Options.HasFlag(DiagnosticLocationOptions.UnnecessaryCode);
if (isExpectedUnnecessary)
{
verifier.True(isActualUnnecessary, $"Expected diagnostic additional location index \"{j}\" to be marked unnecessary, but was not.");
}
else
{
verifier.False(isActualUnnecessary, $"Expected diagnostic additional location index \"{j}\" to not be marked unnecessary, but was instead marked unnecessary.");
}
}
}
}
Expand Down Expand Up @@ -815,18 +844,17 @@ private void VerifyLinePosition(ImmutableArray<DiagnosticAnalyzer> analyzers, Di
private static string FormatDiagnostics(ImmutableArray<DiagnosticAnalyzer> analyzers, string defaultFilePath, params Diagnostic[] diagnostics)
{
var builder = new StringBuilder();
for (var i = 0; i < diagnostics.Length; ++i)
foreach (var diagnostic in diagnostics)
{
var diagnosticsId = diagnostics[i].Id;
var location = diagnostics[i].Location;
var location = diagnostic.Location;

builder.Append("// ").AppendLine(diagnostics[i].ToString());
builder.Append("// ").AppendLine(diagnostic.ToString());

var applicableAnalyzer = analyzers.FirstOrDefault(a => a.SupportedDiagnostics.Any(dd => dd.Id == diagnosticsId));
var applicableAnalyzer = analyzers.FirstOrDefault(a => a.SupportedDiagnostics.Any(dd => dd.Id == diagnostic.Id));
if (applicableAnalyzer != null)
{
var analyzerType = applicableAnalyzer.GetType();
var rule = location != Location.None && location.IsInSource && applicableAnalyzer.SupportedDiagnostics.Length == 1 ? string.Empty : $"{analyzerType.Name}.{diagnosticsId}";
var rule = location != Location.None && location.IsInSource && applicableAnalyzer.SupportedDiagnostics.Length == 1 ? string.Empty : $"{analyzerType.Name}.{diagnostic.Id}";

if (location == Location.None || !location.IsInSource)
{
Expand All @@ -841,11 +869,11 @@ private static string FormatDiagnostics(ImmutableArray<DiagnosticAnalyzer> analy
else
{
builder.Append(
diagnostics[i].Severity switch
diagnostic.Severity switch
{
DiagnosticSeverity.Error => $"{nameof(DiagnosticResult)}.{nameof(DiagnosticResult.CompilerError)}(\"{diagnostics[i].Id}\")",
DiagnosticSeverity.Warning => $"{nameof(DiagnosticResult)}.{nameof(DiagnosticResult.CompilerWarning)}(\"{diagnostics[i].Id}\")",
var severity => $"new {nameof(DiagnosticResult)}(\"{diagnostics[i].Id}\", {nameof(DiagnosticSeverity)}.{severity})",
DiagnosticSeverity.Error => $"{nameof(DiagnosticResult)}.{nameof(DiagnosticResult.CompilerError)}(\"{diagnostic.Id}\")",
DiagnosticSeverity.Warning => $"{nameof(DiagnosticResult)}.{nameof(DiagnosticResult.CompilerWarning)}(\"{diagnostic.Id}\")",
var severity => $"new {nameof(DiagnosticResult)}(\"{diagnostic.Id}\", {nameof(DiagnosticSeverity)}.{severity})",
});
}

Expand All @@ -855,22 +883,35 @@ private static string FormatDiagnostics(ImmutableArray<DiagnosticAnalyzer> analy
}
else
{
AppendLocation(diagnostics[i].Location);
foreach (var additionalLocation in diagnostics[i].AdditionalLocations)
// The unnecessary code designator is ignored for the primary diagnostic location.
AppendLocation(diagnostic.Location, isUnnecessary: false);

int[] unnecessaryIndices = { };
if (diagnostic.Properties.TryGetValue(WellKnownDiagnosticTags.Unnecessary, out var encodedUnnecessaryLocations))
{
var match = EncodedIndicesSyntax.Match(encodedUnnecessaryLocations);
if (match.Success)
{
unnecessaryIndices = match.Groups["Index"].Captures.OfType<Capture>().Select(capture => int.Parse(capture.Value)).ToArray();
}
}

for (var i = 0; i < diagnostic.AdditionalLocations.Count; i++)
{
AppendLocation(additionalLocation);
var additionalLocation = diagnostic.AdditionalLocations[i];
AppendLocation(additionalLocation, isUnnecessary: unnecessaryIndices.Contains(i));
}
}

var arguments = diagnostics[i].Arguments();
var arguments = diagnostic.Arguments();
if (arguments.Count > 0)
{
builder.Append($".{nameof(DiagnosticResult.WithArguments)}(");
builder.Append(string.Join(", ", arguments.Select(a => "\"" + a?.ToString() + "\"")));
builder.Append(")");
}

if (diagnostics[i].IsSuppressed())
if (diagnostic.IsSuppressed())
{
builder.Append($".{nameof(DiagnosticResult.WithIsSuppressed)}(true)");
}
Expand All @@ -881,13 +922,16 @@ private static string FormatDiagnostics(ImmutableArray<DiagnosticAnalyzer> analy
return builder.ToString();

// Local functions
void AppendLocation(Location location)
void AppendLocation(Location location, bool isUnnecessary)
{
var lineSpan = location.GetLineSpan();
var pathString = location.IsInSource && lineSpan.Path == defaultFilePath ? string.Empty : $"\"{lineSpan.Path}\", ";
var linePosition = lineSpan.StartLinePosition;
var endLinePosition = lineSpan.EndLinePosition;
builder.Append($".WithSpan({pathString}{linePosition.Line + 1}, {linePosition.Character + 1}, {endLinePosition.Line + 1}, {endLinePosition.Character + 1})");
var unnecessaryArgument = isUnnecessary
? $", {nameof(DiagnosticLocationOptions)}.{nameof(DiagnosticLocationOptions.UnnecessaryCode)}"
: string.Empty;
builder.Append($".WithSpan({pathString}{linePosition.Line + 1}, {linePosition.Character + 1}, {endLinePosition.Line + 1}, {endLinePosition.Character + 1}{unnecessaryArgument})");
}
}

Expand Down
5 changes: 5 additions & 0 deletions ...CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/DiagnosticLocationOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,10 @@ public enum DiagnosticLocationOptions
/// </list>
/// </summary>
InterpretAsMarkupKey = 2,

/// <summary>
/// The diagnostic location is marked as unnecessary code.
/// </summary>
UnnecessaryCode = 4,
}
}
1 change: 1 addition & 0 deletions ...soft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/PublicAPI.Unshipped.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions
Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions.IgnoreLength = 1 -> Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions
Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions.InterpretAsMarkupKey = 2 -> Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions
Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions.None = 0 -> Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions
Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions.UnnecessaryCode = 4 -> Microsoft.CodeAnalysis.Testing.DiagnosticLocationOptions
Microsoft.CodeAnalysis.Testing.DiagnosticOptions
Microsoft.CodeAnalysis.Testing.DiagnosticOptions.IgnoreAdditionalLocations = 1 -> Microsoft.CodeAnalysis.Testing.DiagnosticOptions
Microsoft.CodeAnalysis.Testing.DiagnosticOptions.IgnoreSeverity = 2 -> Microsoft.CodeAnalysis.Testing.DiagnosticOptions
Expand Down
163 changes: 163 additions & 0 deletions ...ysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing.UnitTests/DiagnosticLocationTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,120 @@ Expected diagnostic to start at column "18" was actually at column "17"
new DefaultVerifier().EqualOrDiff(expected, exception.Message);
}

[Fact]
public async Task TestAdditionalUnnecessaryLocations()
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>().WithLocation(0).WithLocation(1, DiagnosticLocationOptions.UnnecessaryCode),
},
}.RunAsync();
}

[Fact]
public async Task TestAdditionalUnnecessaryLocationsIgnored()
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>().WithLocation(0).WithOptions(DiagnosticOptions.IgnoreAdditionalLocations),
},
}.RunAsync();
}

[Fact]
public async Task TestAdditionalUnnecessaryLocationRequiresExpectedMarkedUnnecessary()
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer>().WithLocation(0).WithLocation(1),
},
}.RunAsync();
});

var expected =
"""
Expected diagnostic additional location index "0" to not be marked unnecessary, but was instead marked unnecessary.
""".ReplaceLineEndings();
new DefaultVerifier().EqualOrDiff(expected, exception.Message);
}

[Fact]
public async Task TestAdditionalUnnecessaryLocationRequiresDescriptorMarkedUnnecessary()
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryButDescriptorNotAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryButDescriptorNotAnalyzer>().WithLocation(0).WithLocation(1, DiagnosticLocationOptions.UnnecessaryCode),
},
}.RunAsync();
});

var expected =
"""
Diagnostic reported extended unnecessary locations, but the descriptor is not marked as unnecessary code.
""".ReplaceLineEndings();
new DefaultVerifier().EqualOrDiff(expected, exception.Message);
}

[Fact]
public async Task TestAdditionalUnnecessaryLocationNotJsonArray()
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryNotJsonArrayAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryNotJsonArrayAnalyzer>().WithLocation(0).WithLocation(1, DiagnosticLocationOptions.UnnecessaryCode),
},
}.RunAsync();
});

var expected =
"""
Expected encoded unnecessary locations to be a valid JSON array of non-negative integers: Text
""".ReplaceLineEndings();
new DefaultVerifier().EqualOrDiff(expected, exception.Message);
}

[Fact]
public async Task TestAdditionalUnnecessaryLocationIndexOutOfRange()
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await new CSharpAnalyzerTest<HighlightBraceSpanWithEndMarkedUnnecessaryIndexOutOfRangeAnalyzer>
{
TestCode = @"class TestClass {|#0:{|} {|#1:}|}",
ExpectedDiagnostics =
{
Diagnostic<HighlightBraceSpanWithEndMarkedUnnecessaryIndexOutOfRangeAnalyzer>().WithLocation(0).WithLocation(1, DiagnosticLocationOptions.UnnecessaryCode),
},
}.RunAsync();
});

var expected =
"""
All unnecessary indices in the diagnostic must be valid indices in AdditionalLocations [0-1): [1]
""".ReplaceLineEndings();
new DefaultVerifier().EqualOrDiff(expected, exception.Message);
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class HighlightBracePositionAnalyzer : AbstractHighlightBracesAnalyzer
{
Expand All @@ -264,6 +378,55 @@ protected override Diagnostic CreateDiagnostic(SyntaxToken token)
}
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class HighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer : AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer
{
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class HighlightBraceSpanWithEndMarkedUnnecessaryNotJsonArrayAnalyzer : AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer
{
protected override string UnnecessaryLocationsValue => "Text";
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class HighlightBraceSpanWithEndMarkedUnnecessaryIndexOutOfRangeAnalyzer : AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer
{
protected override string UnnecessaryLocationsValue => "[1]";
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class HighlightBraceSpanWithEndMarkedUnnecessaryButDescriptorNotAnalyzer : AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer
{
public HighlightBraceSpanWithEndMarkedUnnecessaryButDescriptorNotAnalyzer()
: base(customTags: new string[0])
{
}
}

private abstract class AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer : AbstractHighlightBracesAnalyzer
{
public AbstractHighlightBraceSpanWithEndMarkedUnnecessaryAnalyzer(string[]? customTags = null)
: base(customTags: customTags ?? new[] { WellKnownDiagnosticTags.Unnecessary })
{
}

protected virtual string UnnecessaryLocationsValue => "[0]";

protected override Diagnostic CreateDiagnostic(SyntaxToken token)
{
var endLocation = token.Parent switch
{
CSharp.Syntax.ClassDeclarationSyntax classDeclaration => classDeclaration.CloseBraceToken.GetLocation(),
_ => throw new NotSupportedException(),
};

var additionalLocations = new[] { endLocation };
var properties = ImmutableDictionary.Create<string, string?>().Add(WellKnownDiagnosticTags.Unnecessary, UnnecessaryLocationsValue);
return CodeAnalysis.Diagnostic.Create(Descriptor, token.GetLocation(), additionalLocations, properties);
}
}

[DiagnosticAnalyzer(LanguageNames.CSharp)]
private class ReportCompilationDiagnosticAnalyzer : DiagnosticAnalyzer
{
Expand Down
Loading

0 comments on commit 69e2fa7

Please sign in to comment.