diff --git a/src/Microsoft.SqlTools.LanguageService/Formatter/TSqlFormatterService.cs b/src/Microsoft.SqlTools.LanguageService/Formatter/TSqlFormatterService.cs index 405c312df1..7aa85f8ba8 100644 --- a/src/Microsoft.SqlTools.LanguageService/Formatter/TSqlFormatterService.cs +++ b/src/Microsoft.SqlTools.LanguageService/Formatter/TSqlFormatterService.cs @@ -159,8 +159,17 @@ private Task FormatRangeAndReturnEdits(DocumentRangeForma { return CreateSkippedResult(stopwatch); } + BufferRange bufferRange = range.ToBufferRange(); + if (!scriptFile.IsRangeValid(bufferRange)) + { + // The buffer here can lag the editor's copy, so a requested range may + // outrun it. Formatting what this copy holds would rewrite the wrong + // span of the client's document, so do nothing instead. + Logger.Verbose($"Skipping range format for {docFormatParams.TextDocument.Uri}; range is outside the current buffer"); + return CreateSkippedResult(stopwatch); + } TextEdit textEdit = new TextEdit { Range = range }; - string text = scriptFile.GetTextInRange(range.ToBufferRange()); + string text = scriptFile.GetTextInRange(bufferRange); return DoFormat(docFormatParams, textEdit, text, stopwatch); }); } diff --git a/src/Microsoft.SqlTools.LanguageService/Workspace/Contracts/ScriptFile.cs b/src/Microsoft.SqlTools.LanguageService/Workspace/Contracts/ScriptFile.cs index 55971d51b4..b9c924a54f 100644 --- a/src/Microsoft.SqlTools.LanguageService/Workspace/Contracts/ScriptFile.cs +++ b/src/Microsoft.SqlTools.LanguageService/Workspace/Contracts/ScriptFile.cs @@ -159,7 +159,7 @@ public string GetLine(int lineNumber) { Validate.IsWithinRange( "lineNumber", lineNumber, - 1, FileLines.Count + 1); + 1, FileLines.Count); return FileLines[lineNumber - 1]; } @@ -231,22 +231,49 @@ public void ValidatePosition(BufferPosition bufferPosition) /// The 1-based column to be validated. public void ValidatePosition(int line, int column) { - if (line < 1 || line > FileLines.Count + 1) + if (line < 1 || line > FileLines.Count) { throw new ArgumentOutOfRangeException(nameof(line), SR.WorkspaceServicePositionLineOutOfRange); } - // The maximum column is either one past the length of the string - // or 1 if the string is empty. - string lineString = FileLines[line - 1]; - int maxColumn = lineString.Length > 0 ? lineString.Length + 1 : 1; - - if (column < 1 || column > maxColumn) + if (column < 1 || column > GetMaxColumn(line)) { throw new ArgumentOutOfRangeException(nameof(column), SR.WorkspaceServicePositionColumnOutOfRange(line)); } } + /// + /// Determines whether the given range falls entirely within this file's extents. + /// A client can send a range built against a newer revision of the document than + /// the one held here, so callers that must not fail should test the range first. + /// + /// The buffer range to test. + public bool IsRangeValid(BufferRange bufferRange) + { + return bufferRange != null + && IsPositionValid(bufferRange.Start) + && IsPositionValid(bufferRange.End); + } + + private bool IsPositionValid(BufferPosition position) + { + return position != null + && position.Line >= 1 + && position.Line <= FileLines.Count + && position.Column >= 1 + && position.Column <= GetMaxColumn(position.Line); + } + + /// + /// Gets the highest valid 1-based column for the given 1-based line, which is + /// one past the length of the line, or 1 when the line is empty. + /// + private int GetMaxColumn(int line) + { + string lineString = FileLines[line - 1]; + return lineString.Length > 0 ? lineString.Length + 1 : 1; + } + /// /// Applies the provided FileChange to the file's contents /// diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/Formatter/TSqlFormatterServiceTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/Formatter/TSqlFormatterServiceTests.cs index 4f953a2127..5c479b2879 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/Formatter/TSqlFormatterServiceTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/Formatter/TSqlFormatterServiceTests.cs @@ -318,6 +318,30 @@ await TestUtils.RunAndVerify( } + [Test] + public async Task FormatRangeShouldSkipRangeBeyondBuffer() + { + // Given a request whose range runs one line past this copy of the document, + // as happens when the service's buffer lags behind the editor's buffer + SetupLanguageService(); + SetupScriptFile(defaultSqlContents); + rangeFormatParams.Range = new Range + { + Start = new Position { Line = 0, Character = 0 }, + End = new Position { Line = 1, Character = 0 } + }; + + // When format range is called + await TestUtils.RunAndVerify( + test: (requestContext) => FormatterService.HandleDocRangeFormatRequest(rangeFormatParams, requestContext), + verify: (edits => + { + // Then the request succeeds with no edits rather than failing + Assert.AreEqual(0, edits.Length); + })); + } + + [Test] public async Task FormatDocumentTelemetryShouldIncludeFormatTypeProperty() { diff --git a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ServiceHost/ScriptFileTests.cs b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ServiceHost/ScriptFileTests.cs index 4c5481ec73..95a3d3a0ca 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ServiceHost/ScriptFileTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.UnitTests/ServiceHost/ScriptFileTests.cs @@ -249,6 +249,62 @@ public void ThrowsExceptionWithEditOutsideOfRange() }); } + /// + /// A position on the line just past the end of the file is out of range, and must be + /// reported as such instead of surfacing the underlying list indexer failure. + /// + [Test] + public void ReportsLineOutOfRangeJustPastEndOfFile() + { + ScriptFile scriptFile = GetTestScriptFile("first\r\nsecond"); + + ArgumentOutOfRangeException ex = Assert.Throws(() => + scriptFile.GetLinesInRange( + new BufferRange( + new BufferPosition(1, 1), + new BufferPosition(3, 1)))); + + Assert.AreEqual("line", ex.ParamName); + } + + [Test] + public void GetLineThrowsJustPastEndOfFile() + { + ScriptFile scriptFile = GetTestScriptFile("first\r\nsecond"); + + ArgumentOutOfRangeException ex = Assert.Throws( + () => scriptFile.GetLine(3)); + + Assert.AreEqual("lineNumber", ex.ParamName); + } + + [Test] + public void IsRangeValidDetectsRangesOutsideTheFile() + { + ScriptFile scriptFile = GetTestScriptFile("first\r\nsecond"); + + Assert.True( + scriptFile.IsRangeValid(new BufferRange(1, 1, 2, 7)), + "Range within the file is valid"); + Assert.False( + scriptFile.IsRangeValid(new BufferRange(1, 1, 3, 1)), + "Range ending one line past the end of the file is not valid"); + Assert.False( + scriptFile.IsRangeValid(new BufferRange(1, 1, 2, 20)), + "Range ending past the end of the last line is not valid"); + } + + [Test] + public void TrailingNewlineCreatesValidEmptyFinalLine() + { + ScriptFile scriptFile = GetTestScriptFile("first\r\nsecond\r\n"); + + Assert.AreEqual(string.Empty, scriptFile.GetLine(3)); + Assert.True( + scriptFile.IsRangeValid(new BufferRange(3, 1, 3, 1)), + "The empty line after a trailing newline is within the file"); + } + private void AssertFileChange( string initialString, string expectedString,