Fix crash where lines appended to end of script file causes out of bounds exception #730
Conversation
rjmholt
changed the title
| @@ -309,14 +309,24 @@ 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) | |||
| public void ValidatePosition(int line, int column, bool isInsertion = false) | |||
There was a problem hiding this comment.
Adding an optional parameter is a breaking change, can you add an overload instead?
|
|
||
| /// <summary> | ||
| /// Throws ArgumentOutOfRangeException if the given position is outside | ||
| /// of the file's buffer extents. |
There was a problem hiding this comment.
Perhaps we should add some explanation here to why column-max-index+1 and line-max-index+1 are allowed.
| /// <param name="isInsertion">If true, the position to validate is for an applied change.</param> | ||
| public void ValidatePosition(int line, int column, bool isInsertion) | ||
| { | ||
| int maxLine = isInsertion ? this.FileLines.Count + 1 : this.FileLines.Count; |
There was a problem hiding this comment.
I would add a code comment above this to make sure someone else doesn't think they see an off-by-one error with the this.FileLines.Count + 1 and attempt to fix it. :-)
| /// </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 a (FileLines.Count + 1), |
There was a problem hiding this comment.
Minor nit, should this be "add it at …"?
There was a problem hiding this comment.
Are these comments ok @rkeithhill? Want to make sure I've put them where you want and saying enough, etc.
| { | ||
| if (column != 1) | ||
| { | ||
| throw new ArgumentOutOfRangeException($"Insertion at the end of a file must occur at column 1"); |
There was a problem hiding this comment.
1 indexed columns???!?!?!? omg
There was a problem hiding this comment.
@tylerl0706 yep
$sb = [scriptblock]::Create('')
$sb.Ast.Extent.StartColumnNumber
# 1
$sb.Ast.Extent.StartLineNumber
# 1
$sb.Ast.Extent.StartOffset
# 0|
|
||
| // Should we add first or last line fragments? | ||
| if (changeIndex == 0) | ||
| foreach (string addedLine in changeLines) |
There was a problem hiding this comment.
Would there be a scenario where there's a changed line AND an added line?
There was a problem hiding this comment.
If so, the foreach might also need to handle the "between" logic as well.
There was a problem hiding this comment.
I guess a better question is... are the changed lines always consecutive
| @@ -127,6 +127,22 @@ public void CanApplyMultiLineDelete() | |||
| }); | |||
| } | |||
|
|
|||
| [Fact] | |||
| if (changeIndex == 0) | ||
| { | ||
| // Append the first line fragment | ||
| finalLine = firstLineFragment + finalLine; |
There was a problem hiding this comment.
do you think you could add a bit more comment?
Like... "append the first line fragment to the front of the change"
and... "append the last line fragment to the end of the change"
the code says that but the logic here is a tad complex
| currentLineNumber++; | ||
| } | ||
| // Build and insert the new lines | ||
| int currentLineNumber = fileChange.Line; |
There was a problem hiding this comment.
maybe consider doing the -1 here instead of line 431. I think it's a little clearer.
Fixes PowerShell/vscode-powershell#1283 and fixes PowerShell/vscode-powershell#1492.
Note: I doubt it will affect PowerShell/vscode-powershell#1453; that looks like a separate issue.
It looks like there is a tendency for VSCode to generate a text document change request with an offset that is one beyond the current lines we track under certain conditions. @rkeithhill's repro is the best I've seen.
Anyway, this PR just detects that case and bypasses all the magic insertion logic -- instead we just append the lines to the end.