From 4eed1bbc09e03a6d7f6a2977c8d6cd3c50c6a215 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 23 Aug 2018 13:35:51 -0700 Subject: [PATCH 1/6] Fix crash where lines appended to end of script file cause out of bounds error --- .../Workspace/ScriptFile.cs | 104 +++++++++++------- 1 file changed, 63 insertions(+), 41 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 9d3b4aead..a15946444 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -114,7 +114,7 @@ public Token[] ScriptTokens } /// - /// Gets the array of filepaths dot sourced in this ScriptFile + /// Gets the array of filepaths dot sourced in this ScriptFile /// public string[] ReferencedFiles { @@ -227,7 +227,7 @@ public static IList GetLines(string text) } /// - /// Deterines whether the supplied path indicates the file is an "untitled:Unitled-X" + /// Deterines whether the supplied path indicates the file is an "untitled:Unitled-X" /// which has not been saved to file. /// /// The path to check. @@ -309,14 +309,24 @@ 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) + public void ValidatePosition(int line, int column, bool isInsertion = false) { - int maxLine = this.FileLines.Count; + int maxLine = isInsertion ? this.FileLines.Count + 1 : 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]; @@ -347,51 +357,63 @@ public void ApplyChange(FileChange fileChange) } 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] - .Substring(0, fileChange.Offset - 1); - - // Get the last fragment of the last line - string endLine = this.FileLines[fileChange.EndLine - 1]; - string lastLineFragment = - endLine.Substring( - fileChange.EndOffset - 1, - (this.FileLines[fileChange.EndLine - 1].Length - fileChange.EndOffset) + 1); - - // Remove the old lines - for (int i = 0; i <= fileChange.EndLine - fileChange.Line; i++) - { - this.FileLines.RemoveAt(fileChange.Line - 1); - } + this.ValidatePosition(fileChange.Line, fileChange.Offset, isInsertion: true); + this.ValidatePosition(fileChange.EndLine, fileChange.EndOffset, isInsertion: true); - // Build and insert the new lines - int currentLineNumber = fileChange.Line; - for (int changeIndex = 0; changeIndex < changeLines.Length; changeIndex++) + // If the change is a pure append to the file, we just need to add the new lines on the end + if (fileChange.EndLine == this.FileLines.Count + 1) { - // Since we split the lines above using \n, make sure to - // trim the ending \r's off as well. - string finalLine = changeLines[changeIndex].TrimEnd('\r'); - - // Should we add first or last line fragments? - if (changeIndex == 0) + foreach (string addedLine in changeLines) { - // Append the first line fragment - finalLine = firstLineFragment + finalLine; + string finalLine = addedLine.TrimEnd('\r'); + this.FileLines.Add(finalLine); } - if (changeIndex == changeLines.Length - 1) + } + // Otherwise, the change needs to go between existing content + else + { + // Get the first fragment of the first line + string firstLineFragment = + this.FileLines[fileChange.Line - 1] + .Substring(0, fileChange.Offset - 1); + + // Get the last fragment of the last line + string endLine = this.FileLines[fileChange.EndLine - 1]; + string lastLineFragment = + endLine.Substring( + fileChange.EndOffset - 1, + (this.FileLines[fileChange.EndLine - 1].Length - fileChange.EndOffset) + 1); + + // Remove the old lines + for (int i = 0; i <= fileChange.EndLine - fileChange.Line; i++) { - // Append the last line fragment - finalLine = finalLine + lastLineFragment; + this.FileLines.RemoveAt(fileChange.Line - 1); } - this.FileLines.Insert(currentLineNumber - 1, finalLine); - currentLineNumber++; - } + // Build and insert the new lines + int currentLineNumber = fileChange.Line; + for (int changeIndex = 0; changeIndex < changeLines.Length; changeIndex++) + { + // Since we split the lines above using \n, make sure to + // trim the ending \r's off as well. + string finalLine = changeLines[changeIndex].TrimEnd('\r'); + + // Should we add first or last line fragments? + if (changeIndex == 0) + { + // Append the first line fragment + finalLine = firstLineFragment + finalLine; + } + if (changeIndex == changeLines.Length - 1) + { + // Append the last line fragment + finalLine = finalLine + lastLineFragment; + } + this.FileLines.Insert(currentLineNumber - 1, finalLine); + currentLineNumber++; + } + } } // Parse the script again to be up-to-date From 08647af154d1523b5cb54740e94359c2fcbc3218 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 23 Aug 2018 13:52:20 -0700 Subject: [PATCH 2/6] Fix XML comment, add a test --- .../Workspace/ScriptFile.cs | 1 + .../Session/ScriptFileTests.cs | 22 ++++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index a15946444..88abb284c 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -309,6 +309,7 @@ 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, bool isInsertion = false) { int maxLine = isInsertion ? this.FileLines.Count + 1 : this.FileLines.Count; diff --git a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs index ccaed4ce3..04d798ae0 100644 --- a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs +++ b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs @@ -13,7 +13,7 @@ namespace PSLanguageService.Test { public class ScriptFileChangeTests { - private static readonly Version PowerShellVersion = new Version("5.0"); + private static readonly Version PowerShellVersion = new Version("5.0"); [Fact] public void CanApplySingleLineInsert() @@ -127,6 +127,22 @@ public void CanApplyMultiLineDelete() }); } + [Fact] + public void CanApplyEditsToEndOfFile() + { + this.AssertFileChange( + "line1\n\rline2\n\rline3\n\r", + "line1\n\rline2\n\rline3\n\r\n\r\n\r", + new FileChange + { + Line = 5, + EndLine = 5, + Offset = 1, + EndOffset = 1, + InsertString = "\n\r\n\r" + }); + } + [Fact] public void FindsDotSourcedFiles() { @@ -139,7 +155,7 @@ public void FindsDotSourcedFiles() using (StringReader stringReader = new StringReader(exampleScriptContents)) { - ScriptFile scriptFile = + ScriptFile scriptFile = new ScriptFile( "DotSourceTestFile.ps1", "DotSourceTestFile.ps1", @@ -178,7 +194,7 @@ internal static ScriptFile CreateScriptFile(string initialString) using (StringReader stringReader = new StringReader(initialString)) { // Create an in-memory file from the StringReader - ScriptFile fileToChange = + ScriptFile fileToChange = new ScriptFile( "TestFile.ps1", "TestFile.ps1", From 6a17b4a3c5ad0b7e42e10022cad79c507a8d0101 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 23 Aug 2018 14:14:53 -0700 Subject: [PATCH 3/6] Add a test for the script, add an XML comment for the validation parameter --- src/PowerShellEditorServices/Workspace/ScriptFile.cs | 4 ++-- .../Session/ScriptFileTests.cs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 88abb284c..a30abf684 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -309,7 +309,7 @@ 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. + /// If true, the position to validate is for an applied changnvokee. public void ValidatePosition(int line, int column, bool isInsertion = false) { int maxLine = isInsertion ? this.FileLines.Count + 1 : this.FileLines.Count; @@ -362,7 +362,7 @@ public void ApplyChange(FileChange fileChange) this.ValidatePosition(fileChange.EndLine, fileChange.EndOffset, isInsertion: true); // If the change is a pure append to the file, we just need to add the new lines on the end - if (fileChange.EndLine == this.FileLines.Count + 1) + if (fileChange.Line == this.FileLines.Count + 1) { foreach (string addedLine in changeLines) { diff --git a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs index 04d798ae0..620454bb1 100644 --- a/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs +++ b/test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs @@ -131,15 +131,15 @@ public void CanApplyMultiLineDelete() public void CanApplyEditsToEndOfFile() { this.AssertFileChange( - "line1\n\rline2\n\rline3\n\r", - "line1\n\rline2\n\rline3\n\r\n\r\n\r", + "line1\r\nline2\r\nline3\r\n\r\n", + "line1\r\nline2\r\nline3\r\n\r\n\r\n\r\n", new FileChange { Line = 5, EndLine = 5, Offset = 1, EndOffset = 1, - InsertString = "\n\r\n\r" + InsertString = "\r\n\r\n" }); } From 7bdbc2d19e6007497a5ff109315965ab2f5c9d14 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 23 Aug 2018 14:18:05 -0700 Subject: [PATCH 4/6] Add new validate method as overload rather than optional parameter --- .../Workspace/ScriptFile.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index a30abf684..d10622af4 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -309,8 +309,19 @@ 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 changnvokee. - public void ValidatePosition(int line, int column, bool isInsertion = false) + 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. + /// + /// 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) { int maxLine = isInsertion ? this.FileLines.Count + 1 : this.FileLines.Count; if (line < 1 || line > maxLine) From e1cff3d5cbc691f68dc0f3ba3347293d992433e4 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Sat, 25 Aug 2018 10:21:57 -0700 Subject: [PATCH 5/6] Add comments to explain ScriptFile position validation changes --- src/PowerShellEditorServices/Workspace/ScriptFile.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index d10622af4..5fea01862 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -316,13 +316,17 @@ public void ValidatePosition(int line, int column) /// /// Throws ArgumentOutOfRangeException if the given position is outside - /// of the file's buffer extents. + /// 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) { + // If new content is being added, VSCode sometimes likes to add it a (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; if (line < 1 || line > maxLine) { @@ -372,7 +376,9 @@ public void ApplyChange(FileChange fileChange) this.ValidatePosition(fileChange.Line, fileChange.Offset, isInsertion: true); this.ValidatePosition(fileChange.EndLine, fileChange.EndOffset, isInsertion: true); - // If the change is a pure append to the file, we just need to add the new lines on the end + // 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 if (fileChange.Line == this.FileLines.Count + 1) { foreach (string addedLine in changeLines) From 2bd9a3042d551db67aefcc8babc05804a41dc533 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Sat, 25 Aug 2018 10:47:13 -0700 Subject: [PATCH 6/6] Fix typo --- src/PowerShellEditorServices/Workspace/ScriptFile.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 5fea01862..b34985476 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -324,7 +324,7 @@ public void ValidatePosition(int line, int column) /// If true, the position to validate is for an applied change. public void ValidatePosition(int line, int column, bool isInsertion) { - // If new content is being added, VSCode sometimes likes to add it a (FileLines.Count + 1), + // 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;