From 48efae1b0853cd653dd59e2820f5d328b3ac780d Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Tue, 18 Sep 2018 11:13:01 -0700 Subject: [PATCH 1/3] Fix off-by-one in scriptfile implementation --- .../Workspace/ScriptFile.cs | 46 ++++++------------- .../Session/ScriptFileTests.cs | 36 ++++++++++++++- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index b34985476..444cece5c 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -90,7 +90,7 @@ public ScriptFileMarker[] SyntaxMarkers /// /// Gets the list of strings for each line of the file. /// - internal IList FileLines + internal List FileLines { get; private set; @@ -197,7 +197,7 @@ public ScriptFile( /// /// Input string to be split up into lines. /// The lines in the string. - public static IList GetLines(string text) + public static List GetLines(string text) { if (text == null) { @@ -309,40 +309,15 @@ public void ValidatePosition(BufferPosition bufferPosition) /// /// The 1-based line to be validated. /// The 1-based column to be validated. - public void ValidatePosition(int line, int column) - { - ValidatePosition(line, column, isInsertion: false); - } - - /// - /// Throws ArgumentOutOfRangeException if the given position is outside - /// of the file's buffer extents. If the position is for an insertion (an applied change) - /// the index may be 1 past the end of the file, which is just appended. - /// - /// The 1-based line to be validated. - /// The 1-based column to be validated. /// If true, the position to validate is for an applied change. - public void ValidatePosition(int line, int column, bool isInsertion) + public void ValidatePosition(int line, int column) { - // If new content is being added, VSCode sometimes likes to add it at (FileLines.Count + 1), - // which used to crash EditorServices. Now we append it on to the end of the file. - // See https://github.com/PowerShell/vscode-powershell/issues/1283 - int maxLine = isInsertion ? this.FileLines.Count + 1 : this.FileLines.Count; + int maxLine = this.FileLines.Count; if (line < 1 || line > maxLine) { throw new ArgumentOutOfRangeException($"Position {line}:{column} is outside of the line range of 1 to {maxLine}."); } - // If we are inserting at the end of the file, the column should be 1 - if (isInsertion && line == maxLine) - { - if (column != 1) - { - throw new ArgumentOutOfRangeException($"Insertion at the end of a file must occur at column 1"); - } - return; - } - // The maximum column is either **one past** the length of the string // or 1 if the string is empty. string lineString = this.FileLines[line - 1]; @@ -373,9 +348,6 @@ public void ApplyChange(FileChange fileChange) } else { - this.ValidatePosition(fileChange.Line, fileChange.Offset, isInsertion: true); - this.ValidatePosition(fileChange.EndLine, fileChange.EndOffset, isInsertion: true); - // VSCode sometimes likes to give the change start line as (FileLines.Count + 1). // This used to crash EditorServices, but we now treat it as an append. // See https://github.com/PowerShell/vscode-powershell/issues/1283 @@ -387,9 +359,19 @@ public void ApplyChange(FileChange fileChange) this.FileLines.Add(finalLine); } } + // Similarly, when lines are deleted from the end of the file, + // VSCode likes to give the end line as (FileLines.Count + 1). + else if (fileChange.EndLine == this.FileLines.Count + 1 && String.Empty.Equals(fileChange.InsertString)) + { + int lineIndex = fileChange.Line - 1; + this.FileLines.RemoveRange(lineIndex, this.FileLines.Count - lineIndex); + } // Otherwise, the change needs to go between existing content else { + this.ValidatePosition(fileChange.Line, fileChange.Offset); + this.ValidatePosition(fileChange.EndLine, fileChange.EndOffset); + // Get the first fragment of the first line string firstLineFragment = this.FileLines[fileChange.Line - 1] diff --git a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs index 620454bb1..4d8ec3f80 100644 --- a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs +++ b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs @@ -143,6 +143,23 @@ public void CanApplyEditsToEndOfFile() }); } + [Fact] + public void CanAppendToEndOfFile() + { + this.AssertFileChange( + "line1\r\nline2\r\nline3", + "line1\r\nline2\r\nline3\r\nline4\r\nline5", + new FileChange + { + Line = 4, + EndLine = 5, + Offset = 1, + EndOffset = 1, + InsertString = "line4\r\nline5" + } + ); + } + [Fact] public void FindsDotSourcedFiles() { @@ -181,7 +198,7 @@ public void ThrowsExceptionWithEditOutsideOfRange() new FileChange { Line = 3, - EndLine = 7, + EndLine = 8, Offset = 1, EndOffset = 1, InsertString = "" @@ -189,6 +206,23 @@ public void ThrowsExceptionWithEditOutsideOfRange() }); } + [Fact] + public void CanDeleteFromEndOfFile() + { + this.AssertFileChange( + "line1\r\nline2\r\nline3\r\nline4", + "line1\r\nline2", + new FileChange + { + Line = 3, + EndLine = 5, + Offset = 1, + EndOffset = 1, + InsertString = "" + } + ); + } + internal static ScriptFile CreateScriptFile(string initialString) { using (StringReader stringReader = new StringReader(initialString)) From c79b5977ac6c0d20da1b503219e01a78b0673922 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Tue, 18 Sep 2018 11:22:51 -0700 Subject: [PATCH 2/3] Remove defunct XML comment --- src/PowerShellEditorServices/Workspace/ScriptFile.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 444cece5c..0bbd53654 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -309,7 +309,6 @@ public void ValidatePosition(BufferPosition bufferPosition) /// /// The 1-based line to be validated. /// The 1-based column to be validated. - /// If true, the position to validate is for an applied change. public void ValidatePosition(int line, int column) { int maxLine = this.FileLines.Count; From a391118cae3704d7fbbba79572a816ac97e9e0e8 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Tue, 18 Sep 2018 13:15:48 -0700 Subject: [PATCH 3/3] Add obsolete warnings to deprecated APIs, add bad API back in --- .../Workspace/ScriptFile.cs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 0bbd53654..e8cb7aac1 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -197,7 +197,18 @@ public ScriptFile( /// /// Input string to be split up into lines. /// The lines in the string. - public static List GetLines(string text) + [Obsolete("This method is not designed for public exposure and will be retired in later versions of EditorServices")] + public static IList GetLines(string text) + { + return GetLinesInternal(text); + } + + /// + /// Get the lines in a string. + /// + /// Input string to be split up into lines. + /// The lines in the string. + internal static List GetLinesInternal(string text) { if (text == null) { @@ -328,6 +339,19 @@ public void ValidatePosition(int line, int column) } } + + /// + /// Defunct ValidatePosition method call. The isInsertion parameter is ignored. + /// + /// + /// + /// + [Obsolete("Use ValidatePosition(int, int) instead")] + public void ValidatePosition(int line, int column, bool isInsertion) + { + ValidatePosition(line, column); + } + /// /// Applies the provided FileChange to the file's contents /// @@ -557,7 +581,7 @@ private void SetFileContents(string fileContents) { // Split the file contents into lines and trim // any carriage returns from the strings. - this.FileLines = GetLines(fileContents); + this.FileLines = GetLinesInternal(fileContents); // Parse the contents to get syntax tree and errors this.ParseFileContents();