From bed8105dd02a49441df17ed7be7a7c904493510f Mon Sep 17 00:00:00 2001 From: zanaptak <53196138+zanaptak@users.noreply.github.com> Date: Wed, 18 Nov 2020 17:40:30 -0600 Subject: [PATCH 1/3] Make completion span include backticks and ident characters --- .../Completion/CompletionService.fs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs index 8eebae4dbb1..161f1b6c01a 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs +++ b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs @@ -10,9 +10,12 @@ open Microsoft.CodeAnalysis.Completion open Microsoft.CodeAnalysis.Host open Microsoft.CodeAnalysis.Host.Mef open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Completion +open Microsoft.CodeAnalysis.Text open Microsoft.VisualStudio.Shell +open FSharp.Compiler.PrettyNaming + type internal FSharpCompletionService ( workspace: Workspace, @@ -43,6 +46,41 @@ type internal FSharpCompletionService .WithDismissIfLastCharacterDeleted(true) .WithDefaultEnterKeyRule(enterKeyRule) + override _.GetDefaultCompletionListSpan( sourceText , caretIndex ) = + + let mutable startIndex = 0 + let mutable endIndex = 0 + + // Get single line text and index + let textLines = sourceText.Lines + let lineText = textLines.GetLineFromPosition(caretIndex).ToString() + let lineCaretIndex = textLines.GetLinePosition(caretIndex).Character + + // Check for enclosing backticks or leading backticks, else capture valid identifier characters + // TODO Replace naive backtick check with safer alternative + match lineText.IndexOf "``", lineText.LastIndexOf "``" with + | startTickIndex, endTickIndex when startTickIndex > -1 && endTickIndex > -1 && startTickIndex <> endTickIndex && lineCaretIndex >= startTickIndex && lineCaretIndex <= endTickIndex + 2 -> + // Cursor is at or between a pair of double ticks, select enclosed range including ticks as identifier + startIndex <- startTickIndex + endIndex <- endTickIndex + 2 + | startTickIndex, endTickIndex when startTickIndex > -1 && endTickIndex > -1 && startTickIndex = endTickIndex && lineCaretIndex >= startTickIndex -> + // Cursor is at or after double ticks with none following, select ticks and all following text as identifier + startIndex <- startTickIndex + endIndex <- lineText.Length + | _ -> + // No ticks, capture identifier-part chars backward and forward from cursor as identifier + startIndex <- lineCaretIndex + while startIndex > 0 && IsIdentifierPartCharacter lineText.[startIndex - 1] do startIndex <- startIndex - 1 + endIndex <- lineCaretIndex + if startIndex <> lineCaretIndex then + while endIndex < lineText.Length && IsIdentifierPartCharacter lineText.[endIndex] do endIndex <- endIndex + 1 + + // Translate line index back to document index + startIndex <- caretIndex - (lineCaretIndex - startIndex) + endIndex <- caretIndex + (endIndex - lineCaretIndex) + + TextSpan.FromBounds(startIndex, endIndex) + [] [, FSharpConstants.FSharpLanguageName)>] type internal FSharpCompletionServiceFactory From 154a3f53e4ee5a4c8d953c6b8d303a9b88a016a2 Mon Sep 17 00:00:00 2001 From: zanaptak <53196138+zanaptak@users.noreply.github.com> Date: Sun, 22 Nov 2020 12:00:04 -0600 Subject: [PATCH 2/3] Use Tokenizer for backtick identifiers --- .../Completion/CompletionService.fs | 94 ++++++++++++------- 1 file changed, 60 insertions(+), 34 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs index 161f1b6c01a..64fb1185de7 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs +++ b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs @@ -4,8 +4,10 @@ namespace Microsoft.VisualStudio.FSharp.Editor open System.Composition open System.Collections.Immutable +open System.Threading open Microsoft.CodeAnalysis +open Microsoft.CodeAnalysis.Classification open Microsoft.CodeAnalysis.Completion open Microsoft.CodeAnalysis.Host open Microsoft.CodeAnalysis.Host.Mef @@ -46,40 +48,64 @@ type internal FSharpCompletionService .WithDismissIfLastCharacterDeleted(true) .WithDefaultEnterKeyRule(enterKeyRule) - override _.GetDefaultCompletionListSpan( sourceText , caretIndex ) = - - let mutable startIndex = 0 - let mutable endIndex = 0 - - // Get single line text and index - let textLines = sourceText.Lines - let lineText = textLines.GetLineFromPosition(caretIndex).ToString() - let lineCaretIndex = textLines.GetLinePosition(caretIndex).Character - - // Check for enclosing backticks or leading backticks, else capture valid identifier characters - // TODO Replace naive backtick check with safer alternative - match lineText.IndexOf "``", lineText.LastIndexOf "``" with - | startTickIndex, endTickIndex when startTickIndex > -1 && endTickIndex > -1 && startTickIndex <> endTickIndex && lineCaretIndex >= startTickIndex && lineCaretIndex <= endTickIndex + 2 -> - // Cursor is at or between a pair of double ticks, select enclosed range including ticks as identifier - startIndex <- startTickIndex - endIndex <- endTickIndex + 2 - | startTickIndex, endTickIndex when startTickIndex > -1 && endTickIndex > -1 && startTickIndex = endTickIndex && lineCaretIndex >= startTickIndex -> - // Cursor is at or after double ticks with none following, select ticks and all following text as identifier - startIndex <- startTickIndex - endIndex <- lineText.Length - | _ -> - // No ticks, capture identifier-part chars backward and forward from cursor as identifier - startIndex <- lineCaretIndex - while startIndex > 0 && IsIdentifierPartCharacter lineText.[startIndex - 1] do startIndex <- startIndex - 1 - endIndex <- lineCaretIndex - if startIndex <> lineCaretIndex then - while endIndex < lineText.Length && IsIdentifierPartCharacter lineText.[endIndex] do endIndex <- endIndex + 1 - - // Translate line index back to document index - startIndex <- caretIndex - (lineCaretIndex - startIndex) - endIndex <- caretIndex + (endIndex - lineCaretIndex) - - TextSpan.FromBounds(startIndex, endIndex) + /// Indicates the text span to be replaced by a committed completion list item. + override _.GetDefaultCompletionListSpan(sourceText, caretIndex) = + + // Gets connected identifier-part characters backward and forward from caret. + let getIdentifierChars() = + let mutable startIndex = caretIndex + let mutable endIndex = caretIndex + while startIndex > 0 && IsIdentifierPartCharacter sourceText.[startIndex - 1] do startIndex <- startIndex - 1 + if startIndex <> caretIndex then + while endIndex < sourceText.Length && IsIdentifierPartCharacter sourceText.[endIndex] do endIndex <- endIndex + 1 + TextSpan.FromBounds(startIndex, endIndex) + + let line = sourceText.Lines.GetLineFromPosition(caretIndex) + if line.ToString().IndexOf "``" < 0 then + // No backticks on the line, capture standard identifier chars. + getIdentifierChars() + else + // Line contains backticks. + // Use tokenizer to check for identifier, in order to correctly handle extraneous backticks in comments, strings, etc. + + // If caret is at a backtick-identifier, then that is our span. + + // Else, check if we are after an unclosed ``, to support the common case of a manually typed leading ``. + // Tokenizer will not consider this an identifier, it will consider the bare `` a Keyword, followed by + // arbitrary tokens (Identifier, Operator, Text, etc.) depending on the trailing text. + + // Else, backticks are not involved in caret location, fall back to standard identifier character scan. + + // There may still be edge cases where backtick related spans are incorrectly captured, such as unclosed + // backticks before later valid backticks on a line, this is an acceptable compromise in order to support + // the majority of common cases. + + let documentId = workspace.GetDocumentIdInCurrentContext(sourceText.Container) + let document = workspace.CurrentSolution.GetDocument(documentId) + let defines = projectInfoManager.GetCompilationDefinesForEditingDocument(document) + let classifiedSpans = Tokenizer.getClassifiedSpans(documentId, sourceText, line.Span, Some document.FilePath, defines, CancellationToken.None) + + let isBacktickIdentifier (classifiedSpan: ClassifiedSpan) = + classifiedSpan.ClassificationType = ClassificationTypeNames.Identifier + && sourceText.ToString(classifiedSpan.TextSpan).StartsWith("``") + && sourceText.ToString(classifiedSpan.TextSpan).EndsWith("``") + let isUnclosedBacktick (classifiedSpan: ClassifiedSpan) = + classifiedSpan.ClassificationType = ClassificationTypeNames.Keyword + && sourceText.ToString(classifiedSpan.TextSpan) = "``" + + match classifiedSpans |> Seq.tryFind (fun cs -> isBacktickIdentifier cs && cs.TextSpan.IntersectsWith caretIndex) with + | Some backtickIdentifier -> + // Backtick enclosed identifier found intersecting with caret, use its span. + backtickIdentifier.TextSpan + | _ -> + match classifiedSpans |> Seq.tryFindBack (fun cs -> isUnclosedBacktick cs && caretIndex >= cs.TextSpan.Start) with + | Some unclosedBacktick -> + // Trailing unclosed backtick-pair found before caret, use span from backtick to end of line. + let lastSpan = classifiedSpans.[classifiedSpans.Count - 1] + TextSpan.FromBounds(unclosedBacktick.TextSpan.Start, lastSpan.TextSpan.End) + | _ -> + // No backticks involved at caret position, fall back to standard identifier chars. + getIdentifierChars() [] [, FSharpConstants.FSharpLanguageName)>] From ed3408848482caf9e1e6b7369d9d9a9bc765eddf Mon Sep 17 00:00:00 2001 From: zanaptak <53196138+zanaptak@users.noreply.github.com> Date: Tue, 24 Nov 2020 18:11:13 -0600 Subject: [PATCH 3/3] Add tests for GetDefaultCompletionListSpan --- .../Completion/CompletionService.fs | 65 ++----------------- .../Completion/CompletionUtils.fs | 58 ++++++++++++++++- .../LanguageService/Tokenizer.fs | 19 +++--- .../UnitTests/CompletionProviderTests.fs | 49 ++++++++++++++ 4 files changed, 120 insertions(+), 71 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs index 64fb1185de7..e2c03429d55 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs +++ b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs @@ -4,20 +4,15 @@ namespace Microsoft.VisualStudio.FSharp.Editor open System.Composition open System.Collections.Immutable -open System.Threading open Microsoft.CodeAnalysis -open Microsoft.CodeAnalysis.Classification open Microsoft.CodeAnalysis.Completion open Microsoft.CodeAnalysis.Host open Microsoft.CodeAnalysis.Host.Mef open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Completion -open Microsoft.CodeAnalysis.Text open Microsoft.VisualStudio.Shell -open FSharp.Compiler.PrettyNaming - type internal FSharpCompletionService ( workspace: Workspace, @@ -50,62 +45,10 @@ type internal FSharpCompletionService /// Indicates the text span to be replaced by a committed completion list item. override _.GetDefaultCompletionListSpan(sourceText, caretIndex) = - - // Gets connected identifier-part characters backward and forward from caret. - let getIdentifierChars() = - let mutable startIndex = caretIndex - let mutable endIndex = caretIndex - while startIndex > 0 && IsIdentifierPartCharacter sourceText.[startIndex - 1] do startIndex <- startIndex - 1 - if startIndex <> caretIndex then - while endIndex < sourceText.Length && IsIdentifierPartCharacter sourceText.[endIndex] do endIndex <- endIndex + 1 - TextSpan.FromBounds(startIndex, endIndex) - - let line = sourceText.Lines.GetLineFromPosition(caretIndex) - if line.ToString().IndexOf "``" < 0 then - // No backticks on the line, capture standard identifier chars. - getIdentifierChars() - else - // Line contains backticks. - // Use tokenizer to check for identifier, in order to correctly handle extraneous backticks in comments, strings, etc. - - // If caret is at a backtick-identifier, then that is our span. - - // Else, check if we are after an unclosed ``, to support the common case of a manually typed leading ``. - // Tokenizer will not consider this an identifier, it will consider the bare `` a Keyword, followed by - // arbitrary tokens (Identifier, Operator, Text, etc.) depending on the trailing text. - - // Else, backticks are not involved in caret location, fall back to standard identifier character scan. - - // There may still be edge cases where backtick related spans are incorrectly captured, such as unclosed - // backticks before later valid backticks on a line, this is an acceptable compromise in order to support - // the majority of common cases. - - let documentId = workspace.GetDocumentIdInCurrentContext(sourceText.Container) - let document = workspace.CurrentSolution.GetDocument(documentId) - let defines = projectInfoManager.GetCompilationDefinesForEditingDocument(document) - let classifiedSpans = Tokenizer.getClassifiedSpans(documentId, sourceText, line.Span, Some document.FilePath, defines, CancellationToken.None) - - let isBacktickIdentifier (classifiedSpan: ClassifiedSpan) = - classifiedSpan.ClassificationType = ClassificationTypeNames.Identifier - && sourceText.ToString(classifiedSpan.TextSpan).StartsWith("``") - && sourceText.ToString(classifiedSpan.TextSpan).EndsWith("``") - let isUnclosedBacktick (classifiedSpan: ClassifiedSpan) = - classifiedSpan.ClassificationType = ClassificationTypeNames.Keyword - && sourceText.ToString(classifiedSpan.TextSpan) = "``" - - match classifiedSpans |> Seq.tryFind (fun cs -> isBacktickIdentifier cs && cs.TextSpan.IntersectsWith caretIndex) with - | Some backtickIdentifier -> - // Backtick enclosed identifier found intersecting with caret, use its span. - backtickIdentifier.TextSpan - | _ -> - match classifiedSpans |> Seq.tryFindBack (fun cs -> isUnclosedBacktick cs && caretIndex >= cs.TextSpan.Start) with - | Some unclosedBacktick -> - // Trailing unclosed backtick-pair found before caret, use span from backtick to end of line. - let lastSpan = classifiedSpans.[classifiedSpans.Count - 1] - TextSpan.FromBounds(unclosedBacktick.TextSpan.Start, lastSpan.TextSpan.End) - | _ -> - // No backticks involved at caret position, fall back to standard identifier chars. - getIdentifierChars() + let documentId = workspace.GetDocumentIdInCurrentContext(sourceText.Container) + let document = workspace.CurrentSolution.GetDocument(documentId) + let defines = projectInfoManager.GetCompilationDefinesForEditingDocument(document) + CompletionUtils.getDefaultCompletionListSpan(sourceText, caretIndex, documentId, document.FilePath, defines) [] [, FSharpConstants.FSharpLanguageName)>] diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionUtils.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionUtils.fs index c5494a39fae..b87f7e6c6a6 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionUtils.fs +++ b/vsintegration/src/FSharp.Editor/Completion/CompletionUtils.fs @@ -11,6 +11,7 @@ open Microsoft.CodeAnalysis.Completion open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Completion open System.Globalization open FSharp.Compiler.SourceCodeServices +open FSharp.Compiler.PrettyNaming module internal CompletionUtils = @@ -112,4 +113,59 @@ module internal CompletionUtils = | CompletionItemKind.Event -> 4 | CompletionItemKind.Argument -> 5 | CompletionItemKind.Other -> 6 - | CompletionItemKind.Method (isExtension = true) -> 7 \ No newline at end of file + | CompletionItemKind.Method (isExtension = true) -> 7 + + /// Indicates the text span to be replaced by a committed completion list item. + let getDefaultCompletionListSpan(sourceText: SourceText, caretIndex, documentId, filePath, defines) = + + // Gets connected identifier-part characters backward and forward from caret. + let getIdentifierChars() = + let mutable startIndex = caretIndex + let mutable endIndex = caretIndex + while startIndex > 0 && IsIdentifierPartCharacter sourceText.[startIndex - 1] do startIndex <- startIndex - 1 + if startIndex <> caretIndex then + while endIndex < sourceText.Length && IsIdentifierPartCharacter sourceText.[endIndex] do endIndex <- endIndex + 1 + TextSpan.FromBounds(startIndex, endIndex) + + let line = sourceText.Lines.GetLineFromPosition(caretIndex) + if line.ToString().IndexOf "``" < 0 then + // No backticks on the line, capture standard identifier chars. + getIdentifierChars() + else + // Line contains backticks. + // Use tokenizer to check for identifier, in order to correctly handle extraneous backticks in comments, strings, etc. + + // If caret is at a backtick-identifier, then that is our span. + + // Else, check if we are after an unclosed ``, to support the common case of a manually typed leading ``. + // Tokenizer will not consider this an identifier, it will consider the bare `` a Keyword, followed by + // arbitrary tokens (Identifier, Operator, Text, etc.) depending on the trailing text. + + // Else, backticks are not involved in caret location, fall back to standard identifier character scan. + + // There may still be edge cases where backtick related spans are incorrectly captured, such as unclosed + // backticks before later valid backticks on a line, this is an acceptable compromise in order to support + // the majority of common cases. + + let classifiedSpans = Tokenizer.getClassifiedSpans(documentId, sourceText, line.Span, Some filePath, defines, CancellationToken.None) + + let isBacktickIdentifier (classifiedSpan: ClassifiedSpan) = + classifiedSpan.ClassificationType = ClassificationTypeNames.Identifier + && Tokenizer.isDoubleBacktickIdent (sourceText.ToString(classifiedSpan.TextSpan)) + let isUnclosedBacktick (classifiedSpan: ClassifiedSpan) = + classifiedSpan.ClassificationType = ClassificationTypeNames.Keyword + && sourceText.ToString(classifiedSpan.TextSpan) = "``" + + match classifiedSpans |> Seq.tryFind (fun cs -> isBacktickIdentifier cs && cs.TextSpan.IntersectsWith caretIndex) with + | Some backtickIdentifier -> + // Backtick enclosed identifier found intersecting with caret, use its span. + backtickIdentifier.TextSpan + | _ -> + match classifiedSpans |> Seq.tryFindBack (fun cs -> isUnclosedBacktick cs && caretIndex >= cs.TextSpan.Start) with + | Some unclosedBacktick -> + // Unclosed backtick found before caret, use span from backtick to end of line. + let lastSpan = classifiedSpans.[classifiedSpans.Count - 1] + TextSpan.FromBounds(unclosedBacktick.TextSpan.Start, lastSpan.TextSpan.End) + | _ -> + // No backticks involved at caret position, fall back to standard identifier chars. + getIdentifierChars() diff --git a/vsintegration/src/FSharp.Editor/LanguageService/Tokenizer.fs b/vsintegration/src/FSharp.Editor/LanguageService/Tokenizer.fs index f45ea349ecc..8a39d38ec29 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/Tokenizer.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/Tokenizer.fs @@ -1,4 +1,4 @@ -namespace Microsoft.VisualStudio.FSharp.Editor +namespace Microsoft.VisualStudio.FSharp.Editor open System open System.Collections.Generic @@ -805,15 +805,16 @@ module internal Tokenizer = | -1 | 0 -> span | index -> TextSpan(span.Start + index + 1, text.Length - index - 1) + let private doubleBackTickDelimiter = "``" + + let isDoubleBacktickIdent (s: string) = + let doubledDelimiter = 2 * doubleBackTickDelimiter.Length + if s.Length > doubledDelimiter && s.StartsWith(doubleBackTickDelimiter, StringComparison.Ordinal) && s.EndsWith(doubleBackTickDelimiter, StringComparison.Ordinal) then + let inner = s.AsSpan(doubleBackTickDelimiter.Length, s.Length - doubledDelimiter) + not (inner.Contains(doubleBackTickDelimiter.AsSpan(), StringComparison.Ordinal)) + else false + let isValidNameForSymbol (lexerSymbolKind: LexerSymbolKind, symbol: FSharpSymbol, name: string) : bool = - let doubleBackTickDelimiter = "``" - - let isDoubleBacktickIdent (s: string) = - let doubledDelimiter = 2 * doubleBackTickDelimiter.Length - if s.Length > doubledDelimiter && s.StartsWith(doubleBackTickDelimiter, StringComparison.Ordinal) && s.EndsWith(doubleBackTickDelimiter, StringComparison.Ordinal) then - let inner = s.AsSpan(doubleBackTickDelimiter.Length, s.Length - doubledDelimiter) - not (inner.Contains(doubleBackTickDelimiter.AsSpan(), StringComparison.Ordinal)) - else false let isIdentifier (ident: string) = if isDoubleBacktickIdent ident then diff --git a/vsintegration/tests/UnitTests/CompletionProviderTests.fs b/vsintegration/tests/UnitTests/CompletionProviderTests.fs index 4f7c5f3ae68..8b7f1ad24f7 100644 --- a/vsintegration/tests/UnitTests/CompletionProviderTests.fs +++ b/vsintegration/tests/UnitTests/CompletionProviderTests.fs @@ -125,6 +125,13 @@ let VerifyCompletionListExactly(fileContents: string, marker: string, expected: let VerifyNoCompletionList(fileContents: string, marker: string) = VerifyCompletionListExactly(fileContents, marker, []) +let VerifyCompletionListSpan(fileContents: string, marker: string, expected: string) = + let caretPosition = fileContents.IndexOf(marker) + marker.Length + let documentId = DocumentId.CreateNewId(ProjectId.CreateNewId()) + let sourceText = SourceText.From(fileContents) + let resultSpan = CompletionUtils.getDefaultCompletionListSpan(sourceText, caretPosition, documentId, filePath, []) + Assert.AreEqual(expected, sourceText.ToString(resultSpan)) + [] let ShouldTriggerCompletionAtCorrectMarkers() = let testCases = @@ -687,6 +694,48 @@ module Extensions = """ VerifyCompletionList(fileContents, "wrappedMessage.", ["PrintRef"], []) +[] +let ``Completion list span works with underscore in identifier``() = + let fileContents = """ +let x = A.B_C +""" + VerifyCompletionListSpan(fileContents, "A.B_C", "B_C") + +[] +let ``Completion list span works with digit in identifier``() = + let fileContents = """ +let x = A.B1C +""" + VerifyCompletionListSpan(fileContents, "A.B1C", "B1C") + +[] +let ``Completion list span works with enclosed backtick identifier``() = + let fileContents = """ +let x = A.``B C`` +""" + VerifyCompletionListSpan(fileContents, "A.``B C``", "``B C``") + +[] +let ``Completion list span works with partial backtick identifier``() = + let fileContents = """ +let x = A.``B C +""" + VerifyCompletionListSpan(fileContents, "A.``B C", "``B C") + +[] +let ``Completion list span works with first of multiple enclosed backtick identifiers``() = + let fileContents = """ +let x = A.``B C`` + D.``E F`` +""" + VerifyCompletionListSpan(fileContents, "A.``B C``", "``B C``") + +[] +let ``Completion list span works with last of multiple enclosed backtick identifiers``() = + let fileContents = """ +let x = A.``B C`` + D.``E F`` +""" + VerifyCompletionListSpan(fileContents, "D.``E F``", "``E F``") + #if EXE ShouldDisplaySystemNamespace() #endif