From 994bea45c8dfa96cf226b24a63ee9cdad7d7ea06 Mon Sep 17 00:00:00 2001 From: zanaptak <53196138+zanaptak@users.noreply.github.com> Date: Wed, 25 Nov 2020 15:09:43 -0600 Subject: [PATCH] Fix completion with backticks, underscores, numbers (#10500) --- .../Completion/CompletionService.fs | 7 +++ .../Completion/CompletionUtils.fs | 58 ++++++++++++++++++- .../LanguageService/Tokenizer.fs | 19 +++--- .../UnitTests/CompletionProviderTests.fs | 49 ++++++++++++++++ 4 files changed, 123 insertions(+), 10 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs index 8eebae4dbb1..e2c03429d55 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs +++ b/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs @@ -43,6 +43,13 @@ type internal FSharpCompletionService .WithDismissIfLastCharacterDeleted(true) .WithDefaultEnterKeyRule(enterKeyRule) + /// Indicates the text span to be replaced by a committed completion list item. + override _.GetDefaultCompletionListSpan(sourceText, caretIndex) = + 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)>] type internal FSharpCompletionServiceFactory 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