Merge pull request #2055 from OmniSharp/bugfix/highlighting-outofbounds

validation for highlighting range
OmniSharp · Jan 5, 2021 · e0f9971 · e0f9971
2 parents b8d0a28 + bf54efa
commit e0f9971
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 6 deletions.
diff --git a/src/OmniSharp.Abstractions/Models/v2/Range.cs b/src/OmniSharp.Abstractions/Models/v2/Range.cs
@@ -28,6 +28,8 @@ public bool Contains(int line, int column)
             return true;
         }
 
+        public bool IsValid() => Start != null && Start.Line > -1 && Start.Column > -1 && End != null && End.Line > -1 && End.Column > -1;
+
         public override bool Equals(object obj)
             => Equals(obj as Range);
 

diff --git a/src/OmniSharp.Roslyn.CSharp/Services/SemanticHighlight/SemanticHighlightService.cs b/src/OmniSharp.Roslyn.CSharp/Services/SemanticHighlight/SemanticHighlightService.cs
@@ -5,6 +5,7 @@
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Classification;
 using Microsoft.CodeAnalysis.Text;
+using Microsoft.Extensions.Logging;
 using OmniSharp.Mef;
 using OmniSharp.Models.SemanticHighlight;
 
@@ -13,10 +14,14 @@ namespace OmniSharp.Roslyn.CSharp.Services.SemanticHighlight
     [OmniSharpHandler(OmniSharpEndpoints.V2.Highlight, LanguageNames.CSharp)]
     public class SemanticHighlightService : IRequestHandler<SemanticHighlightRequest, SemanticHighlightResponse>
     {
+        private readonly OmniSharpWorkspace _workspace;
+        private readonly ILogger<SemanticHighlightService> _logger;
+
         [ImportingConstructor]
-        public SemanticHighlightService(OmniSharpWorkspace workspace)
+        public SemanticHighlightService(OmniSharpWorkspace workspace, ILoggerFactory loggerFactory)
         {
             _workspace = workspace;
+            _logger = loggerFactory.CreateLogger<SemanticHighlightService>();
         }
 
         public async Task<SemanticHighlightResponse> Handle(SemanticHighlightRequest request)
@@ -33,9 +38,17 @@ public async Task<SemanticHighlightResponse> Handle(SemanticHighlightRequest req
                 TextSpan textSpan;
                 if (request.Range is object)
                 {
-                    var start = text.Lines.GetPosition(new LinePosition(request.Range.Start.Line, request.Range.Start.Column));
-                    var end = text.Lines.GetPosition(new LinePosition(request.Range.End.Line, request.Range.End.Column));
-                    textSpan = new TextSpan(start, end - start);
+                    if (request.Range.IsValid())
+                    {
+                        var start = text.Lines.GetPosition(new LinePosition(request.Range.Start.Line, request.Range.Start.Column));
+                        var end = text.Lines.GetPosition(new LinePosition(request.Range.End.Line, request.Range.End.Column));
+                        textSpan = new TextSpan(start, end - start);
+                    }
+                    else
+                    {
+                        _logger.LogWarning($"Supplied highlight range {request.Range} in document {document.FilePath} is not valid.");
+                        continue;
+                    }
                 }
                 else
                 {
@@ -162,7 +175,5 @@ class ClassifiedResult
             {
                 [ClassificationTypeNames.StaticSymbol] = SemanticHighlightModifier.Static,
             };
-
-        private readonly OmniSharpWorkspace _workspace;
     }
 }
diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/SemanticHighlightFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/SemanticHighlightFacts.cs
@@ -19,6 +19,22 @@ public SemanticHighlightFacts(ITestOutputHelper output, SharedOmniSharpHostFixtu
 
         protected override string EndpointName => OmniSharpEndpoints.V2.Highlight;
 
+        [Fact]
+        public async Task InvalidPositionDoesNotThrow()
+        {
+            var testFile = new TestFile("a.cs", @"
+namespace N1
+{
+    class C1 { int n = true; }
+}
+");
+
+            var line = -1;
+            var highlights = await GetSemanticHighlightsForLineAsync(testFile, line);
+
+            Assert.Empty(highlights);
+        }
+
         [Fact]
         public async Task SemanticHighlightSingleLine()
         {