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 1 commit
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
46 changes: 14 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,7 @@ public ScriptFile(
/// </summary>
/// <param name="text">Input string to be split up into lines.</param>
/// <returns>The lines in the string.</returns>
public static IList<string> GetLines(string text)
public static List<string> GetLines(string text)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise this is a breaking change, but I without RemoveRange this gets trickier, and more importantly, less performant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof that's an odd one. Kind of seems like that shouldn't have been public.

I think it's a pretty safe bet that no one is using it... I would have never thought to look for a static get lines method on that class. That said, you could make an alternate private GetLinesInternal that is typed as List<string> then keep this method the same and have it implicitly cast to IList<string>

Either way, might consider decorating with Obsolete. That should probably either be moved to a utility class or just made private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the impression that there are quite a few APIs exposed as public that really should be internal PSES, which is something I would ideally like to address with v2. It's a bit tricky because the way the project is broken up means some things need to be shared between libraries, but on the other hand those things are opportunities to design a real module API. But yeah, a lot of our public APIs I think need to be retired or changed.

{
if (text == null)
{
Expand Down Expand Up @@ -309,40 +309,15 @@ public void ValidatePosition(BufferPosition bufferPosition)
/// </summary>
/// <param name="line">The 1-based line to be validated.</param>
/// <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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this is also a breaking change -- so maybe we ought to keep it in? I'd prefer not personally, but the preference isn't that strong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have something like this?

public void ValidatePosition(int line, int column)
{
    // Actual logic
}

[Obsolete("Use ValidatePosition(int, int) instead")]
public void ValidatePosition(int line, int column, bool isInsertion)
{
    ValidatePosition(line, column);
}

I think it's worth avoiding the breaking change here, even if the risk is low.

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];
Expand Down Expand Up @@ -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
Expand All @@ -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]
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