Fix URI escaping in /errorlog

src/Compilers/Core/CodeAnalysisTest/Diagnostics/ErrorLoggerTests.cs

@@ -1,13 +1,14 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
-using Xunit;
-using System.IO;
 using System;
+using System.Collections.Generic;
 using System.Globalization;
+using System.IO;
 using System.Text;
-using Microsoft.CodeAnalysis.CSharp;
+
 using Microsoft.CodeAnalysis.Text;
-using System.Collections.Generic;
+
+using Xunit;
 
 namespace Microsoft.CodeAnalysis.UnitTests.Diagnostics
 {
@@ -20,15 +21,20 @@ public void AdditionalLocationsAsRelatedLocations()
             var stream = new MemoryStream();
             using (var logger = new ErrorLogger(stream, "toolName", "1.2.3.4", new Version(1, 2, 3, 4), CultureInfo.InvariantCulture))
             {
-                var mainLocation = Location.Create(@"Z:\MainLocation.cs", new TextSpan(0, 0), new LinePositionSpan(LinePosition.Zero, LinePosition.Zero));
-                var additionalLocation = Location.Create(@"Z:\AdditionalLocation.cs", new TextSpan(0, 0), new LinePositionSpan(LinePosition.Zero, LinePosition.Zero));
+                var span = new TextSpan(0, 0);
+                var position = new LinePositionSpan(LinePosition.Zero, LinePosition.Zero);
+                var mainLocation = Location.Create(@"Z:\Main Location.cs", span, position);
                 var descriptor = new DiagnosticDescriptor("TST", "_TST_", "", "", DiagnosticSeverity.Error, false);
 
-                IEnumerable<Location> additionalLocations = new[] { additionalLocation };
+                IEnumerable<Location> additionalLocations = new[] {
+                    Location.Create(@"Relative Additional\Location.cs", span, position),
+                    Location.Create(@"a:cannot/interpret/as\uri", span, position),
+                };
+
                 logger.LogDiagnostic(Diagnostic.Create(descriptor, mainLocation, additionalLocations));
             }
 
-            string expected = 
+            string expected =
 @"{
   ""$schema"": ""http://json.schemastore.org/sarif-1.0.0-beta.5"",
   ""version"": ""1.0.0-beta.5"",
@@ -47,7 +53,7 @@ public void AdditionalLocationsAsRelatedLocations()
           ""locations"": [
             {
               ""resultFile"": {
-                ""uri"": ""file:///Z:/MainLocation.cs"",
+                ""uri"": ""file:///Z:/Main%20Location.cs"",
                 ""region"": {
                   ""startLine"": 1,
                   ""startColumn"": 1,
@@ -60,7 +66,18 @@ public void AdditionalLocationsAsRelatedLocations()
           ""relatedLocations"": [
             {
               ""physicalLocation"": {
-                ""uri"": ""file:///Z:/AdditionalLocation.cs"",
+                ""uri"": ""Relative%20Additional/Location.cs"",
+                ""region"": {
+                  ""startLine"": 1,
+                  ""startColumn"": 1,
+                  ""endLine"": 1,
+                  ""endColumn"": 1
+                }
+              }
+            },
+            {
+              ""physicalLocation"": {
+                ""uri"": ""a%3Acannot%2Finterpret%2Fas%5Curi"",
                 ""region"": {
                   ""startLine"": 1,
                   ""startColumn"": 1,
@@ -93,7 +110,7 @@ public void AdditionalLocationsAsRelatedLocations()
         public void DescriptorIdCollision()
         {
             var descriptors = new[] {
-                // Toughest case: generation of TST001.001 collides with with actual TST001.001 and must be bumped to TST00.002
+                // Toughest case: generation of TST001.001 collides with with actual TST001.001 and must be bumped to TST001.002
                 new DiagnosticDescriptor("TST001.001",    "_TST001.001_",     "", "", DiagnosticSeverity.Warning, true),
                 new DiagnosticDescriptor("TST001",        "_TST001_",         "", "", DiagnosticSeverity.Warning, true),
                 new DiagnosticDescriptor("TST001",        "_TST001.002_",     "", "", DiagnosticSeverity.Warning, true),

src/Compilers/Core/Portable/CommandLine/ErrorLogger.cs

@@ -155,22 +155,34 @@ private static bool HasPath(Location location)
             return !string.IsNullOrEmpty(location.GetLineSpan().Path);
         }
 
+        private static readonly Uri _fileRoot = new Uri("file:///");
+
         private static string GetUri(string path)
         {
             Debug.Assert(!string.IsNullOrEmpty(path));
 
+            // Note that in general, these "paths" are opaque strings to be 
+            // interpreted by resolvers (see SyntaxTree.FilePath documentation).
+
+            // Common case: absolute path -> absolute URI
             Uri uri;
+            if (Uri.TryCreate(path, UriKind.Absolute, out uri))
+            {
+                // Note that URI.ToString() does not, for example, escape spaces to %20.
+                return uri.AbsoluteUri;
+            }
 
-            if (!Uri.TryCreate(path, UriKind.RelativeOrAbsolute, out uri))
+            // First fallback attempt: interpret as relative reference if possible
+            // (perhaps the resolver works that way).
+            if (Uri.TryCreate(path, UriKind.Relative, out uri))
             {
-                // The only constraint on paths are that they can be interpreted by
-                // various resolvers so there is no guarantee we can turn the arbitrary string
-                // in to a URI. If our attempt to do so fails, use the original string as the
-                // "URI".
-                return path;
+                // There is no AbsoluteUri equivalent for relative URI references and ToString() 
+                // won't escape without this relative -> absolute -> relative trick.
+                return _fileRoot.MakeRelativeUri(new Uri(_fileRoot, uri)).ToString();
             }
 
-            return uri.ToString();
+            // Last resort: UrlEncode the whole opaque string as a relative reference with one part.
+            return System.Net.WebUtility.UrlEncode(path);
         }
 
         private void WriteProperties(Diagnostic diagnostic)
@@ -393,10 +405,14 @@ private sealed class Comparer : IEqualityComparer<DiagnosticDescriptor>
                 public bool Equals(DiagnosticDescriptor x, DiagnosticDescriptor y)
                 {
                     if (ReferenceEquals(x, y))
+                    {
                         return true;
+                    }
 
                     if (ReferenceEquals(x, null) || ReferenceEquals(y, null))
+                    {
                         return false;
+                    }
 
                    return (x.Category == y.Category
                         && x.DefaultSeverity == y.DefaultSeverity
@@ -411,7 +427,9 @@ public bool Equals(DiagnosticDescriptor x, DiagnosticDescriptor y)
                 public int GetHashCode(DiagnosticDescriptor obj)
                 {
                     if (ReferenceEquals(obj, null))
+                    {
                         return 0;
+                    }
 
                     int hash = Hash.Combine(obj.Category.GetHashCode(),
                         Hash.Combine(obj.DefaultSeverity.GetHashCode(),