From e3d46b54119d103f9cb9dee4cad35d33b3b72f9a Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 11 Dec 2015 10:08:10 -0800 Subject: [PATCH] Fix #45, #63: Completion text replacement bugs This change resolves a general class of issues around code completions coming from the language service. The JSON protocol allows the language server to specify the range of code that will get replaced by a completion item. Previously this range was being computed incorrectly. We now use the replacement range given by the PowerShell API. This also resolves PowerShell/vscode-powershell issues 45 and 12 --- .../Server/LanguageServer.cs | 39 ++-------- .../Language/AstOperations.cs | 6 +- .../Language/CompletionResults.cs | 20 +++++ .../Language/LanguageService.cs | 30 +++++--- .../PowerShellEditorServices.csproj | 2 + .../Workspace/BufferPosition.cs | 35 +++++++++ .../Workspace/BufferRange.cs | 36 +++++++++ .../Workspace/ScriptFile.cs | 75 +++++++++++++++++++ .../Completion/CompleteAttributeValue.cs | 24 ++++++ .../Completion/CompleteFilePath.cs | 30 ++++++++ .../Completion/CompletionExamples.psm1 | 8 +- ...owerShellEditorServices.Test.Shared.csproj | 2 + .../Language/LanguageServiceTests.cs | 26 +++++++ 13 files changed, 287 insertions(+), 46 deletions(-) create mode 100644 src/PowerShellEditorServices/Workspace/BufferPosition.cs create mode 100644 src/PowerShellEditorServices/Workspace/BufferRange.cs create mode 100644 test/PowerShellEditorServices.Test.Shared/Completion/CompleteAttributeValue.cs create mode 100644 test/PowerShellEditorServices.Test.Shared/Completion/CompleteFilePath.cs diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index 704380a64..32f0312cd 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -364,38 +364,13 @@ await editorSession.LanguageService.GetCompletionsInFile( int startEditColumn = textDocumentPosition.Position.Character; int endEditColumn = textDocumentPosition.Position.Character; - // Find the extents of the token under the cursor - var completedToken = - scriptFile - .ScriptAst - .FindAll( - ast => - { - return - !(ast is PipelineAst) && - ast.Extent.StartLineNumber == cursorLine && - ast.Extent.StartColumnNumber <= cursorColumn && - ast.Extent.EndColumnNumber >= cursorColumn; - }, - true) - .LastOrDefault(); // The most relevant AST will be the last - - if (completedToken != null) - { - // The edit should replace the token that was found at the cursor position - startEditColumn = completedToken.Extent.StartColumnNumber - 1; - endEditColumn = completedToken.Extent.EndColumnNumber - 1; - } - completionItems = completionResults .Completions .Select( c => CreateCompletionItem( c, - textDocumentPosition.Position.Line, - startEditColumn, - endEditColumn)) + completionResults.ReplacedRange)) .ToArray(); } else @@ -942,9 +917,7 @@ private static CompletionItemKind MapCompletionKind(CompletionType completionTyp private static CompletionItem CreateCompletionItem( CompletionDetails completionDetails, - int lineNumber, - int startColumn, - int endColumn) + BufferRange completionRange) { string detailString = null; @@ -971,13 +944,13 @@ private static CompletionItem CreateCompletionItem( { Start = new Position { - Line = lineNumber, - Character = startColumn + Line = completionRange.Start.Line - 1, + Character = completionRange.Start.Column - 1 }, End = new Position { - Line = lineNumber, - Character = endColumn + Line = completionRange.End.Line - 1, + Character = completionRange.End.Column - 1 } } } diff --git a/src/PowerShellEditorServices/Language/AstOperations.cs b/src/PowerShellEditorServices/Language/AstOperations.cs index 1d1178651..a73f550c1 100644 --- a/src/PowerShellEditorServices/Language/AstOperations.cs +++ b/src/PowerShellEditorServices/Language/AstOperations.cs @@ -38,10 +38,10 @@ internal static class AstOperations /// A CommandCompletion instance that contains completions for the /// symbol at the given offset. /// - static public CompletionResults GetCompletions( + static public CommandCompletion GetCompletions( Ast scriptAst, Token[] currentTokens, - int fileOffset, + int fileOffset, Runspace runspace) { var type = scriptAst.Extent.StartScriptPosition.GetType(); @@ -75,7 +75,7 @@ static public CompletionResults GetCompletions( } } - return CompletionResults.Create(commandCompletion); + return commandCompletion; } /// diff --git a/src/PowerShellEditorServices/Language/CompletionResults.cs b/src/PowerShellEditorServices/Language/CompletionResults.cs index 0f94e9bdf..00b3a6b44 100644 --- a/src/PowerShellEditorServices/Language/CompletionResults.cs +++ b/src/PowerShellEditorServices/Language/CompletionResults.cs @@ -24,16 +24,36 @@ public sealed class CompletionResults /// public CompletionDetails[] Completions { get; private set; } + /// + /// Gets the range in the buffer that should be replaced by this + /// completion result. + /// + public BufferRange ReplacedRange { get; private set; } + #endregion #region Constructors + /// + /// Creates an empty CompletionResults instance. + /// + public CompletionResults() + { + this.Completions = new CompletionDetails[0]; + this.ReplacedRange = new BufferRange(); + } + internal static CompletionResults Create( + ScriptFile scriptFile, CommandCompletion commandCompletion) { return new CompletionResults { Completions = GetCompletionsArray(commandCompletion), + ReplacedRange = + scriptFile.GetRangeBetweenOffsets( + commandCompletion.ReplacementIndex, + commandCompletion.ReplacementIndex + commandCompletion.ReplacementLength) }; } diff --git a/src/PowerShellEditorServices/Language/LanguageService.cs b/src/PowerShellEditorServices/Language/LanguageService.cs index 06c60386e..c68b4b62c 100644 --- a/src/PowerShellEditorServices/Language/LanguageService.cs +++ b/src/PowerShellEditorServices/Language/LanguageService.cs @@ -89,7 +89,7 @@ public async Task GetCompletionsInFile( RunspaceHandle runspaceHandle = await this.powerShellContext.GetRunspaceHandle(); - CompletionResults completionResults = + CommandCompletion commandCompletion = AstOperations.GetCompletions( scriptFile.ScriptAst, scriptFile.ScriptTokens, @@ -97,14 +97,26 @@ public async Task GetCompletionsInFile( runspaceHandle.Runspace); runspaceHandle.Dispose(); - - // save state of most recent completion - mostRecentCompletions = completionResults; - mostRecentRequestFile = scriptFile.Id; - mostRecentRequestLine = lineNumber; - mostRecentRequestOffest = columnNumber; - - return completionResults; + + if (commandCompletion != null) + { + CompletionResults completionResults = + CompletionResults.Create( + scriptFile, + commandCompletion); + + // save state of most recent completion + mostRecentCompletions = completionResults; + mostRecentRequestFile = scriptFile.Id; + mostRecentRequestLine = lineNumber; + mostRecentRequestOffest = columnNumber; + + return completionResults; + } + else + { + return new CompletionResults(); + } } /// diff --git a/src/PowerShellEditorServices/PowerShellEditorServices.csproj b/src/PowerShellEditorServices/PowerShellEditorServices.csproj index a8d718113..e3199ebef 100644 --- a/src/PowerShellEditorServices/PowerShellEditorServices.csproj +++ b/src/PowerShellEditorServices/PowerShellEditorServices.csproj @@ -102,7 +102,9 @@ + + diff --git a/src/PowerShellEditorServices/Workspace/BufferPosition.cs b/src/PowerShellEditorServices/Workspace/BufferPosition.cs new file mode 100644 index 000000000..c77bf4a97 --- /dev/null +++ b/src/PowerShellEditorServices/Workspace/BufferPosition.cs @@ -0,0 +1,35 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +namespace Microsoft.PowerShell.EditorServices +{ + /// + /// Provides details about a position in a file buffer. + /// + public struct BufferPosition + { + /// + /// Gets the line number of the position in the buffer. + /// + public int Line { get; private set; } + + /// + /// Gets the column number of the position in the buffer. + /// + public int Column { get; private set; } + + /// + /// Creates a new instance of the BufferPosition class. + /// + /// The line number of the position. + /// The column number of the position. + public BufferPosition(int line, int column) + { + this.Line = line; + this.Column = column; + } + } +} + diff --git a/src/PowerShellEditorServices/Workspace/BufferRange.cs b/src/PowerShellEditorServices/Workspace/BufferRange.cs new file mode 100644 index 000000000..83b0767cc --- /dev/null +++ b/src/PowerShellEditorServices/Workspace/BufferRange.cs @@ -0,0 +1,36 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +namespace Microsoft.PowerShell.EditorServices +{ + /// + /// Provides details about a range between two positions in + /// a file buffer. + /// + public struct BufferRange + { + /// + /// Gets the start position of the range in the buffer. + /// + public BufferPosition Start { get; private set; } + + /// + /// Gets the end position of the range in the buffer. + /// + public BufferPosition End { get; private set; } + + /// + /// Creates a new instance of the BufferRange class. + /// + /// The start position of the range. + /// The end position of the range. + public BufferRange(BufferPosition start, BufferPosition end) + { + this.Start = start; + this.End = end; + } + } +} + diff --git a/src/PowerShellEditorServices/Workspace/ScriptFile.cs b/src/PowerShellEditorServices/Workspace/ScriptFile.cs index 54b88b613..52d48c1fd 100644 --- a/src/PowerShellEditorServices/Workspace/ScriptFile.cs +++ b/src/PowerShellEditorServices/Workspace/ScriptFile.cs @@ -255,6 +255,81 @@ public int GetOffsetAtPosition(int lineNumber, int columnNumber) return offset; } + /// + /// Calculates the 1-based line and column number position based + /// on the given buffer offset. + /// + /// The buffer offset to convert. + /// A new BufferPosition containing the position of the offset. + public BufferPosition GetPositionAtOffset(int bufferOffset) + { + BufferRange bufferRange = + GetRangeBetweenOffsets( + bufferOffset, bufferOffset); + + return bufferRange.Start; + } + + /// + /// Calculates the 1-based line and column number range based on + /// the given start and end buffer offsets. + /// + /// The start offset of the range. + /// The end offset of the range. + /// A new BufferRange containing the positions in the offset range. + public BufferRange GetRangeBetweenOffsets(int startOffset, int endOffset) + { + bool foundStart = false; + int currentOffset = 0; + int searchedOffset = startOffset; + + BufferPosition startPosition = new BufferPosition(); + BufferPosition endPosition = startPosition; + + int line = 0; + while (line < this.FileLines.Count) + { + if (searchedOffset <= currentOffset + this.FileLines[line].Length) + { + int column = searchedOffset - currentOffset; + + // Have we already found the start position? + if (foundStart) + { + // Assign the end position and end the search + endPosition = new BufferPosition(line + 1, column + 1); + break; + } + else + { + startPosition = new BufferPosition(line + 1, column + 1); + + // Do we only need to find the start position? + if (startOffset == endOffset) + { + endPosition = startPosition; + break; + } + else + { + // Since the end offset can be on the same line, + // skip the line increment and continue searching + // for the end position + foundStart = true; + searchedOffset = endOffset; + continue; + } + } + } + + // Increase the current offset and include newline length + currentOffset += this.FileLines[line].Length + Environment.NewLine.Length; + line++; + } + + return new BufferRange(startPosition, endPosition); + } + #endregion #region Private Methods diff --git a/test/PowerShellEditorServices.Test.Shared/Completion/CompleteAttributeValue.cs b/test/PowerShellEditorServices.Test.Shared/Completion/CompleteAttributeValue.cs new file mode 100644 index 000000000..1b5319ac2 --- /dev/null +++ b/test/PowerShellEditorServices.Test.Shared/Completion/CompleteAttributeValue.cs @@ -0,0 +1,24 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +namespace Microsoft.PowerShell.EditorServices.Test.Shared.Completion +{ + public class CompleteAttributeValue + { + public static readonly ScriptRegion SourceDetails = + new ScriptRegion + { + File = @"Completion\CompletionExamples.psm1", + StartLineNumber = 16, + StartColumnNumber = 38 + }; + + public static readonly BufferRange ExpectedRange = + new BufferRange( + new BufferPosition(16, 33), + new BufferPosition(16, 38)); + } +} + diff --git a/test/PowerShellEditorServices.Test.Shared/Completion/CompleteFilePath.cs b/test/PowerShellEditorServices.Test.Shared/Completion/CompleteFilePath.cs new file mode 100644 index 000000000..4bc71b8bc --- /dev/null +++ b/test/PowerShellEditorServices.Test.Shared/Completion/CompleteFilePath.cs @@ -0,0 +1,30 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Microsoft.PowerShell.EditorServices.Test.Shared.Completion +{ + public class CompleteFilePath + { + public static readonly ScriptRegion SourceDetails = + new ScriptRegion + { + File = @"Completion\CompletionExamples.psm1", + StartLineNumber = 19, + StartColumnNumber = 25 + }; + + public static readonly BufferRange ExpectedRange = + new BufferRange( + new BufferPosition(19, 15), + new BufferPosition(19, 25)); + } +} + diff --git a/test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1 b/test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1 index 55e48622f..f34690cca 100644 --- a/test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1 +++ b/test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1 @@ -10,4 +10,10 @@ Get-So $testVar Import-Module PowerShellGet -Install-Mo \ No newline at end of file +Install-Mo + +function Test-Completion { + param([Parameter(Mandatory, Value)]) +} + +Get-ChildItem c:\Program diff --git a/test/PowerShellEditorServices.Test.Shared/PowerShellEditorServices.Test.Shared.csproj b/test/PowerShellEditorServices.Test.Shared/PowerShellEditorServices.Test.Shared.csproj index e74f34398..db4b337fd 100644 --- a/test/PowerShellEditorServices.Test.Shared/PowerShellEditorServices.Test.Shared.csproj +++ b/test/PowerShellEditorServices.Test.Shared/PowerShellEditorServices.Test.Shared.csproj @@ -39,8 +39,10 @@ + + diff --git a/test/PowerShellEditorServices.Test/Language/LanguageServiceTests.cs b/test/PowerShellEditorServices.Test/Language/LanguageServiceTests.cs index 33019f1a1..86e1033fb 100644 --- a/test/PowerShellEditorServices.Test/Language/LanguageServiceTests.cs +++ b/test/PowerShellEditorServices.Test/Language/LanguageServiceTests.cs @@ -78,6 +78,32 @@ await this.GetCompletionResults( completionResults.Completions[0]); } + [Fact] + public async Task LanguageServiceCompletesAttributeValue() + { + CompletionResults completionResults = + await this.GetCompletionResults( + CompleteAttributeValue.SourceDetails); + + Assert.NotEqual(0, completionResults.Completions.Length); + Assert.Equal( + CompleteAttributeValue.ExpectedRange, + completionResults.ReplacedRange); + } + + [Fact] + public async Task LanguageServiceCompletesFilePath() + { + CompletionResults completionResults = + await this.GetCompletionResults( + CompleteFilePath.SourceDetails); + + Assert.NotEqual(0, completionResults.Completions.Length); + Assert.Equal( + CompleteFilePath.ExpectedRange, + completionResults.ReplacedRange); + } + [Fact] public async Task LanguageServiceFindsParameterHintsOnCommand() {