Ensure Location.None in additional locations can round trip correctly

Author: Youssef1313

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