Ensure Location.None in additional locations can round trip correctly (…

…#76623)

If an analyzer adds `Location.None` in 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

src/EditorFeatures/Test/Diagnostics/DiagnosticDataTests.cs

@@ -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);
+        Assert.Null(noneAdditionalLocation.DocumentId);
+        Assert.Null(roundTripAdditionalLocation.DocumentId);
+        Assert.Equal(noneAdditionalLocation.UnmappedFileSpan, roundTripAdditionalLocation.UnmappedFileSpan);
+        Assert.Same(diagnostic.AdditionalLocations.Single(), Location.None);
+    }
+
     [Fact]
     public async Task DiagnosticData_SourceGeneratedDocumentLocationIsPreserved()
     {

src/Workspaces/Core/Portable/Diagnostics/DiagnosticData.cs

@@ -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));
             }
             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();