Fix index out-of-range exception when deleting script files (#748)

* Fix off-by-one in scriptfile implementation

* Add obsolete warnings to deprecated APIs

* Restore old file change validation logic

Author: rjmholt

src/PowerShellEditorServices/Workspace/ScriptFile.cs

@@ -90,7 +90,7 @@ public ScriptFileMarker[] SyntaxMarkers
         /// <summary>
         /// Gets the list of strings for each line of the file.
         /// </summary>
-        internal IList<string> FileLines
+        internal List<string> FileLines
         {
             get;
             private set;
@@ -197,7 +197,18 @@ public ScriptFile(
         /// </summary>
         /// <param name="text">Input string to be split up into lines.</param>
         /// <returns>The lines in the string.</returns>
+        [Obsolete("This method is not designed for public exposure and will be retired in later versions of EditorServices")]
         public static IList<string> GetLines(string text)
+        {
+            return GetLinesInternal(text);
+        }
+
+        /// <summary>
+        /// Get the lines in a string.
+        /// </summary>
+        /// <param name="text">Input string to be split up into lines.</param>
+        /// <returns>The lines in the string.</returns>
+        internal static List<string> GetLinesInternal(string text)
         {
             if (text == null)
             {
@@ -311,38 +322,12 @@ public void ValidatePosition(BufferPosition bufferPosition)
         /// <param name="column">The 1-based column to be validated.</param>
         public void ValidatePosition(int line, int column)
         {
-            ValidatePosition(line, column, isInsertion: false);
-        }
-
-        /// <summary>
-        /// 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.
-        /// </summary>
-        /// <param name="line">The 1-based line to be validated.</param>
-        /// <param name="column">The 1-based column to be validated.</param>
-        /// <param name="isInsertion">If true, the position to validate is for an applied change.</param>
-        public void ValidatePosition(int line, int column, bool isInsertion)
-        {
-            // 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];
@@ -354,6 +339,19 @@ public void ValidatePosition(int line, int column, bool isInsertion)
             }
         }
 
+
+        /// <summary>
+        /// Defunct ValidatePosition method call. The isInsertion parameter is ignored.
+        /// </summary>
+        /// <param name="line"></param>
+        /// <param name="column"></param>
+        /// <param name="isInsertion"></param>
+        [Obsolete("Use ValidatePosition(int, int) instead")]
+        public void ValidatePosition(int line, int column, bool isInsertion)
+        {
+            ValidatePosition(line, column);
+        }
+
         /// <summary>
         /// Applies the provided FileChange to the file's contents
         /// </summary>
@@ -373,9 +371,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 +382,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]
@@ -576,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();

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,14 +198,31 @@ public void ThrowsExceptionWithEditOutsideOfRange()
                         new FileChange
                         {
                             Line = 3,
-                            EndLine = 7,
+                            EndLine = 8,
                             Offset = 1,
                             EndOffset = 1,
                             InsertString = ""
                         });
                 });
         }
 
+        [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))