Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 20, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48efae1
Fix off-by-one in scriptfile implementation
Sep 18, 2018
c79b597
Remove defunct XML comment
Sep 18, 2018
a391118
Add obsolete warnings to deprecated APIs, add bad API back in
Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 37 additions & 32 deletions src/PowerShellEditorServices/Workspace/ScriptFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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];
Expand All @@ -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>
Expand All @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -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();
Expand Down
36 changes: 35 additions & 1 deletion test/PowerShellEditorServices.Test/Session/ScriptFileTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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))
Expand Down