Ensure Location.None in additional locations can round trip correctly #76623
Conversation
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
| @@ -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)); | |||
There was a problem hiding this comment.
CreateLocation always returns non-null. So switched AddIfNotNull to simply Add.
| var diagnostic = await diagnosticData.ToDiagnosticAsync(document.Project, CancellationToken.None); | ||
| var roundTripDiagnosticData = DiagnosticData.Create(diagnostic, document); | ||
|
|
||
| var roundTripAdditionalLocation = Assert.Single(roundTripDiagnosticData.AdditionalLocations); |
There was a problem hiding this comment.
This assert would fail without the fix.
dotnet-policy-service
bot
added
Community
|
@CyrusNajmabadi I'm curious why having a separate serializable type was this the taken approach? Couldn't the compiler's That way, if a user decided, for whatever reason, to implement their own |
Youssef1313
changed the title
Youssef1313
force-pushed
the
diagnostic-roundtrip
branch
from
12a5051 to
a1c72a3
Compare
| Assert.Null(noneAdditionalLocation.DocumentId); | ||
| Assert.Null(roundTripAdditionalLocation.DocumentId); | ||
| Assert.Equal(noneAdditionalLocation.UnmappedFileSpan, roundTripAdditionalLocation.UnmappedFileSpan); | ||
| Assert.Same(diagnostic.AdditionalLocations.Single(), Location.None); |
There was a problem hiding this comment.
Shouldn't we verify that producing diagnostics from the diagnosticdata also works and gives back Location.None?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah. Gotcha. That's sufficient for me
If an analyzer adds
Location.Nonein additional location, it gets removed away in the code fix.This can be useful to preserve in case the analyzer can have multiple additional locations, some of which are kinda "optional", but each has a separate meaning that is determined by its index.
Currently, the only way around that is to not use Location.None, and instead use the properties map to determine the meanings.
NOTE: Even after this is merged, analyzer/codefix authors shouldn't really rely on the new behavior if they intend to support earlier versions of VS. This PR is intended to stabilize things in future when the fix becomes in an "old enough" VS version.
FYI @Sergio0694