Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Location.None in additional locations can round trip correctly #76623

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Ensure Location.None in additional locations can round trip correctly #76623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/EditorFeatures/Test/Diagnostics/DiagnosticDataTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,43 @@ public async Task DiagnosticData_ExternalAdditionalLocationIsPreserved()
Assert.Equal(externalAdditionalLocation.UnmappedFileSpan, roundTripAdditionalLocation.UnmappedFileSpan);
}

[Fact]
public async Task DiagnosticData_NoneAdditionalLocationIsPreserved()
{
using var workspace = new TestWorkspace(composition: EditorTestCompositions.EditorFeatures);

var additionalDocument = workspace.CurrentSolution.AddProject("TestProject", "TestProject", LanguageNames.CSharp)
.AddDocument("test.cs", "", filePath: "test.cs");

var document = additionalDocument.Project.Documents.Single();

var noneAdditionalLocation = new DiagnosticDataLocation(new FileLinePositionSpan("", default));

var diagnosticData = new DiagnosticData(
id: "test1",
category: "Test",
message: "test1 message",
severity: DiagnosticSeverity.Info,
defaultSeverity: DiagnosticSeverity.Info,
isEnabledByDefault: true,
warningLevel: 1,
projectId: document.Project.Id,
customTags: [],
properties: ImmutableDictionary<string, string>.Empty,
location: new DiagnosticDataLocation(new FileLinePositionSpan(document.FilePath, span: default), document.Id),
additionalLocations: [noneAdditionalLocation],
language: document.Project.Language);

var diagnostic = await diagnosticData.ToDiagnosticAsync(document.Project, CancellationToken.None);
var roundTripDiagnosticData = DiagnosticData.Create(diagnostic, document);

var roundTripAdditionalLocation = Assert.Single(roundTripDiagnosticData.AdditionalLocations);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert would fail without the fix.

Assert.Null(noneAdditionalLocation.DocumentId);
Assert.Null(roundTripAdditionalLocation.DocumentId);
Assert.Equal(noneAdditionalLocation.UnmappedFileSpan, roundTripAdditionalLocation.UnmappedFileSpan);
Assert.Same(diagnostic.AdditionalLocations.Single(), Location.None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we verify that producing diagnostics from the diagnosticdata also works and gives back Location.None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm interpreting this correctly, the diagnostic variable is created from diagnosticData and is asserted at line 204 to have Location.None additional location. So, both ways are already tested. We started with DiagnosticData, then to Diagnostic, then back to DiagnosticData. The original and final DiagnosticData are asserted that both have additional location with null DocumentId and have the same UnmappedFileSpan. And the Diagnostic is asserted to have a single additional location which is Location.None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Gotcha. That's sufficient for me

}

[Fact]
public async Task DiagnosticData_SourceGeneratedDocumentLocationIsPreserved()
{
Expand Down
15 changes: 13 additions & 2 deletions src/Workspaces/Core/Portable/Diagnostics/DiagnosticData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,24 @@ private static ImmutableArray<DiagnosticDataLocation> GetAdditionalLocations(Tex
{
if (location.IsInSource)
{
builder.AddIfNotNull(CreateLocation(document.Project.Solution.GetDocument(location.SourceTree), location));
builder.Add(CreateLocation(document.Project.Solution.GetDocument(location.SourceTree), location));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateLocation always returns non-null. So switched AddIfNotNull to simply Add.

}
else if (location.Kind == LocationKind.ExternalFile)
{
var textDocumentId = document.Project.GetDocumentForExternalLocation(location);
builder.AddIfNotNull(CreateLocation(document.Project.GetTextDocument(textDocumentId), location));
builder.Add(CreateLocation(document.Project.GetTextDocument(textDocumentId), location));
}
else if (location.Kind == LocationKind.None)
{
builder.Add(CreateLocation(document: null, location));
}
// TODO: Should we throw an exception in an else?
// This will be reachable if a user creates his own type inheriting Location, and
// returns, e.g, LocationKind.XmlFile in Kind override.
// The case for custom `Location`s in general will be hard (if possible at all) to
// always round trip correctly.
// Or, maybe just always create a location with null document, so at least we guarantee that
// the count of additional location created by analyzer always matches what end up being in the code fix.
}

return builder.ToImmutableAndClear();
Expand Down
Loading