llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: None (argothiel) <details> <summary>Changes</summary> Handle new insertReplaceSupport capability (LSP 3.16). Add the new option to the protocol layer and pass it around to the code completion logic. Update CompletionItem::textEdit to become the union type as per the LSP specification. Add a new helper function to the Lexer to find the end of the identifier with full context lexing, to avoid duplicating the logic. Use the helper both in Sema flow, and in the comment completion flow. Use a simpler ASCII-only scan in no-compile mode. Add LIT tests to verify auto-triggered completions, mid-word replacement, unicode, and snippets. Add unit tests to verify insert/replace ranges with and without Sema, including comments and the feature-off case. Update the release notes to document the new capability. Fixes https://github.com/clangd/clangd/issues/2190 --- Patch is 48.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/187623.diff 13 Files Affected: - (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+1) - (modified) clang-tools-extra/clangd/CodeComplete.cpp (+147-36) - (modified) clang-tools-extra/clangd/CodeComplete.h (+13-3) - (modified) clang-tools-extra/clangd/Protocol.cpp (+12-1) - (modified) clang-tools-extra/clangd/Protocol.h (+24-3) - (added) clang-tools-extra/clangd/test/completion-auto-trigger-replace.test (+113) - (added) clang-tools-extra/clangd/test/completion-replace.test (+166) - (added) clang-tools-extra/clangd/test/completion-snippets-replace.test (+68) - (modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+139-15) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang/include/clang/Lex/Lexer.h (+8) - (modified) clang/lib/Lex/Lexer.cpp (+22) - (modified) clang/unittests/Lex/LexerTest.cpp (+34) ``````````diff diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index ebd42abd2dd61..65da685be1278 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -518,6 +518,7 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params, Opts.CodeComplete.EnableSnippets = Params.capabilities.CompletionSnippets; Opts.CodeComplete.IncludeFixIts = Params.capabilities.CompletionFixes; + Opts.CodeComplete.EnableInsertReplace = Params.capabilities.InsertReplace; if (!Opts.CodeComplete.BundleOverloads) Opts.CodeComplete.BundleOverloads = Params.capabilities.HasSignatureHelp; Opts.CodeComplete.DocumentationFormat = diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index 7c390f9c8219d..c58ef139b7437 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -1382,14 +1382,17 @@ void loadMainFilePreambleMacros(const Preprocessor &PP, bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, const clang::CodeCompleteOptions &Options, const SemaCompleteInput &Input, - IncludeStructure *Includes = nullptr) { + IncludeStructure *Includes = nullptr, + std::unique_ptr<CompilerInvocation> CI = nullptr) { trace::Span Tracer("Sema completion"); IgnoreDiagnostics IgnoreDiags; - auto CI = buildCompilerInvocation(Input.ParseInput, IgnoreDiags); if (!CI) { - elog("Couldn't create CompilerInvocation"); - return false; + CI = buildCompilerInvocation(Input.ParseInput, IgnoreDiags); + if (!CI) { + elog("Couldn't create CompilerInvocation"); + return false; + } } auto &FrontendOpts = CI->getFrontendOpts(); FrontendOpts.SkipFunctionBodies = true; @@ -1615,7 +1618,8 @@ class CodeCompleteFlow { bool Incomplete = false; // Would more be available with a higher limit? CompletionPrefix HeuristicPrefix; std::optional<FuzzyMatcher> Filter; // Initialized once Sema runs. - Range ReplacedRange; + Range InsertRange; + std::optional<Range> ReplaceRange; std::vector<std::string> QueryScopes; // Initialized once Sema runs. std::vector<std::string> AccessibleScopes; // Initialized once Sema runs. // Initialized once QueryScopes is initialized, if there are scopes. @@ -1750,8 +1754,19 @@ class CodeCompleteFlow { IsUsingDeclaration = false; Filter = FuzzyMatcher(HeuristicPrefix.Name); auto Pos = offsetToPosition(Content, Offset); - ReplacedRange.start = ReplacedRange.end = Pos; - ReplacedRange.start.character -= HeuristicPrefix.Name.size(); + InsertRange.start = InsertRange.end = Pos; + InsertRange.start.character -= HeuristicPrefix.Name.size(); + + if (Opts.EnableInsertReplace) { + ReplaceRange.emplace(); + ReplaceRange->start = InsertRange.start; + // Scan forward past ASCII identifier characters to find replace end. + size_t ReplaceEnd = Offset; + while (ReplaceEnd < Content.size() && + isAsciiIdentifierContinue(Content[ReplaceEnd])) + ++ReplaceEnd; + ReplaceRange->end = offsetToPosition(Content, ReplaceEnd); + } llvm::StringMap<SourceParams> ProxSources; ProxSources[FileName].Cost = 0; @@ -1832,19 +1847,26 @@ class CodeCompleteFlow { CodeCompleteResult runWithSema() { const auto &CodeCompletionRange = CharSourceRange::getCharRange( Recorder->CCSema->getPreprocessor().getCodeCompletionTokenRange()); + + const SourceManager &SM = Recorder->CCSema->getSourceManager(); + // When we are getting completions with an empty identifier, for example // std::vector<int> asdf; // asdf.^; // Then the range will be invalid and we will be doing insertion, use // current cursor position in such cases as range. if (CodeCompletionRange.isValid()) { - ReplacedRange = halfOpenToRange(Recorder->CCSema->getSourceManager(), - CodeCompletionRange); + InsertRange = halfOpenToRange(SM, CodeCompletionRange); } else { const auto &Pos = sourceLocToPosition( - Recorder->CCSema->getSourceManager(), - Recorder->CCSema->getPreprocessor().getCodeCompletionLoc()); - ReplacedRange.start = ReplacedRange.end = Pos; + SM, Recorder->CCSema->getPreprocessor().getCodeCompletionLoc()); + InsertRange.start = InsertRange.end = Pos; + } + + if (Opts.EnableInsertReplace) { + ReplaceRange.emplace(); + ReplaceRange->start = InsertRange.start; + ReplaceRange->end = getEndOfCodeCompletionReplace(SM); } Filter = FuzzyMatcher( Recorder->CCSema->getPreprocessor().getCodeCompletionFilter()); @@ -1874,6 +1896,26 @@ class CodeCompleteFlow { return toCodeCompleteResult(Top); } + // Returns the LSP position at the end of the identifier suffix after the + // code completion cursor. + Position getEndOfCodeCompletionReplace(const SourceManager &SM) { + const Preprocessor &PP = Recorder->CCSema->getPreprocessor(); + const LangOptions &LangOpts = Recorder->CCSema->getLangOpts(); + + // Skip past the code completion NUL byte and scan forward through + // identifier continuation characters (letters, digits, _, $, UCN, + // unicode). This handles all cases uniformly: with prefix ("vac^1abc"), + // without prefix ("vec.^asdf"), and digit-starting ("vec.^1abc"). + const SourceLocation SuffixBegin = + PP.getCodeCompletionLoc().getLocWithOffset(1); + Position End = sourceLocToPosition( + SM, Lexer::findEndOfIdentifierContinuation(SuffixBegin, SM, LangOpts)); + // Adjust for the NUL byte inserted at the cursor by code completion, + // which inflates the column by 1. + End.character--; + return End; + } + CodeCompleteResult toCodeCompleteResult(const std::vector<ScoredBundle> &Scored) { CodeCompleteResult Output; @@ -1885,7 +1927,8 @@ class CodeCompleteFlow { for (auto &C : Scored) { Output.Completions.push_back(toCodeCompletion(C.first)); Output.Completions.back().Score = C.second; - Output.Completions.back().CompletionTokenRange = ReplacedRange; + Output.Completions.back().CompletionInsertRange = InsertRange; + Output.Completions.back().CompletionReplaceRange = ReplaceRange; if (Opts.Index && !Output.Completions.back().Documentation) { for (auto &Cand : C.first) { if (Cand.SemaResult && @@ -1909,7 +1952,8 @@ class CodeCompleteFlow { } Output.HasMore = Incomplete; Output.Context = CCContextKind; - Output.CompletionRange = ReplacedRange; + Output.InsertRange = InsertRange; + Output.ReplaceRange = ReplaceRange; // Look up documentation from the index. if (Opts.Index) { @@ -2230,16 +2274,54 @@ CompletionPrefix guessCompletionPrefix(llvm::StringRef Content, return Result; } +// If Offset is inside what looks like argument comment (e.g. +// "/*^*/" or "/* foo = ^*/"), returns the offset pointing past the closing "*/". +static std::optional<unsigned> +maybeFunctionArgumentCommentEnd(const PathRef FileName, const unsigned Offset, + const llvm::StringRef Content, + const LangOptions &LangOpts) { + if (Offset > Content.size()) + return std::nullopt; + + SourceManagerForFile FileSM(FileName, Content); + const SourceManager &SM = FileSM.get(); + const SourceLocation Cursor = + SM.getComposedLoc(SM.getMainFileID(), static_cast<unsigned>(Offset)); + const SourceLocation EndOfSuffix = + Lexer::findEndOfIdentifierContinuation(Cursor, SM, LangOpts); + const unsigned EndOfSuffixOffset = SM.getFileOffset(EndOfSuffix); + + const llvm::StringRef Rest = Content.drop_front(EndOfSuffixOffset); + llvm::StringRef RestTrimmed = Rest.ltrim(); + // Comment argument pattern: `/* name = */` — skip past optional `=`. + if (RestTrimmed.starts_with("=")) + RestTrimmed = RestTrimmed.drop_front(1).ltrim(); + if (RestTrimmed.starts_with("*/")) + return EndOfSuffixOffset + (Rest.size() - RestTrimmed.size()) + 2; + return std::nullopt; +} + // Code complete the argument name on "/*" inside function call. -// Offset should be pointing to the start of the comment, i.e.: +// OutsideStartOffset should be pointing before the comment, i.e.: // foo(^/*, rather than foo(/*^) where the cursor probably is. -CodeCompleteResult codeCompleteComment(PathRef FileName, unsigned Offset, - llvm::StringRef Prefix, - const PreambleData *Preamble, - const ParseInputs &ParseInput) { +CodeCompleteResult +codeCompleteComment(PathRef FileName, const unsigned CursorOffset, + unsigned OutsideStartOffset, llvm::StringRef Prefix, + const PreambleData *Preamble, const ParseInputs &ParseInput, + const CodeCompleteOptions &Opts) { if (Preamble == nullptr) // Can't run without Sema. return CodeCompleteResult(); + IgnoreDiagnostics IgnoreDiags; + auto CI = buildCompilerInvocation(ParseInput, IgnoreDiags); + if (!CI) + return CodeCompleteResult(); + + std::optional<unsigned> OutsideEndOffset; + if (Opts.EnableInsertReplace) + OutsideEndOffset = maybeFunctionArgumentCommentEnd( + FileName, CursorOffset, ParseInput.Contents, CI->getLangOpts()); + clang::CodeCompleteOptions Options; Options.IncludeGlobals = false; Options.IncludeMacros = false; @@ -2250,20 +2332,31 @@ CodeCompleteResult codeCompleteComment(PathRef FileName, unsigned Offset, // full patch. semaCodeComplete( std::make_unique<ParamNameCollector>(Options, ParamNames), Options, - {FileName, Offset, *Preamble, + {FileName, OutsideStartOffset, *Preamble, PreamblePatch::createFullPatch(FileName, ParseInput, *Preamble), - ParseInput}); + ParseInput}, + /*Includes=*/nullptr, std::move(CI)); if (ParamNames.empty()) return CodeCompleteResult(); CodeCompleteResult Result; - Range CompletionRange; + Range InsertRange; // Skip /* - Offset += 2; - CompletionRange.start = offsetToPosition(ParseInput.Contents, Offset); - CompletionRange.end = - offsetToPosition(ParseInput.Contents, Offset + Prefix.size()); - Result.CompletionRange = CompletionRange; + const unsigned InsideStartOffset = OutsideStartOffset + 2; + InsertRange.start = offsetToPosition(ParseInput.Contents, InsideStartOffset); + InsertRange.end = + offsetToPosition(ParseInput.Contents, InsideStartOffset + Prefix.size()); + Result.InsertRange = InsertRange; + + if (Opts.EnableInsertReplace) { + Range ReplaceRange; + ReplaceRange.start = InsertRange.start; + ReplaceRange.end = OutsideEndOffset ? offsetToPosition(ParseInput.Contents, + *OutsideEndOffset) + : InsertRange.end; + Result.ReplaceRange = ReplaceRange; + } + Result.Context = CodeCompletionContext::CCC_NaturalLanguage; for (llvm::StringRef Name : ParamNames) { if (!Name.starts_with(Prefix)) @@ -2272,7 +2365,8 @@ CodeCompleteResult codeCompleteComment(PathRef FileName, unsigned Offset, Item.Name = Name.str() + "=*/"; Item.FilterText = Item.Name; Item.Kind = CompletionItemKind::Text; - Item.CompletionTokenRange = CompletionRange; + Item.CompletionInsertRange = InsertRange; + Item.CompletionReplaceRange = Result.ReplaceRange; Item.Origin = SymbolOrigin::AST; Result.Completions.push_back(Item); } @@ -2312,8 +2406,8 @@ CodeCompleteResult codeComplete(PathRef FileName, Position Pos, // parsing, so we must move back the position before running it, extract // information we need and construct completion items ourselves. auto CommentPrefix = Content.substr(*OffsetBeforeComment + 2).trim(); - return codeCompleteComment(FileName, *OffsetBeforeComment, CommentPrefix, - Preamble, ParseInput); + return codeCompleteComment(FileName, *Offset, *OffsetBeforeComment, + CommentPrefix, Preamble, ParseInput, Opts); } auto Flow = CodeCompleteFlow( @@ -2423,7 +2517,9 @@ CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const { } LSP.sortText = sortText(Score.Total, FilterText); LSP.filterText = FilterText; - LSP.textEdit = {CompletionTokenRange, RequiredQualifier + Name, ""}; + TextEdit Edit; + Edit.range = CompletionInsertRange; + Edit.newText = RequiredQualifier + Name; // Merge continuous additionalTextEdits into main edit. The main motivation // behind this is to help LSP clients, it seems most of them are confused when // they are provided with additionalTextEdits that are consecutive to main @@ -2432,19 +2528,34 @@ CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const { // is mainly to help LSP clients again, so that changes do not effect each // other. for (const auto &FixIt : FixIts) { - if (FixIt.range.end == LSP.textEdit->range.start) { - LSP.textEdit->newText = FixIt.newText + LSP.textEdit->newText; - LSP.textEdit->range.start = FixIt.range.start; + if (FixIt.range.end == Edit.range.start) { + Edit.newText = FixIt.newText + Edit.newText; + Edit.range.start = FixIt.range.start; } else { LSP.additionalTextEdits.push_back(FixIt); } } if (Opts.EnableSnippets) - LSP.textEdit->newText += SnippetSuffix; + Edit.newText += SnippetSuffix; // FIXME(kadircet): Do not even fill insertText after making sure textEdit is // compatible with most of the editors. - LSP.insertText = LSP.textEdit->newText; + LSP.insertText = Edit.newText; + if (Opts.EnableInsertReplace) { + assert(CompletionReplaceRange && + "CompletionReplaceRange must be already set before render() " + "when EnableInsertReplace is on"); + InsertReplaceEdit IRE; + IRE.newText = std::move(Edit.newText); + IRE.insert = Edit.range; + IRE.replace = *CompletionReplaceRange; + // FixIt merging may have extended the insert range start; keep replace + // range as a superset per LSP spec. + IRE.replace.start = IRE.insert.start; + LSP.textEdit = std::move(IRE); + } else { + LSP.textEdit = std::move(Edit); + } // Some clients support snippets but work better with plaintext. // So if the snippet is trivial, let the client know. // https://github.com/clangd/clangd/issues/922 diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h index cde22a8212e6a..99ae48c30907c 100644 --- a/clang-tools-extra/clangd/CodeComplete.h +++ b/clang-tools-extra/clangd/CodeComplete.h @@ -71,6 +71,10 @@ struct CodeCompleteOptions { /// Whether to present doc comments as plain-text or markdown. MarkupKind DocumentationFormat = MarkupKind::PlainText; + /// Whether to present the completion as a single textEdit range or as two + /// ranges (insert/replace). + bool EnableInsertReplace = false; + Config::HeaderInsertionPolicy InsertIncludes = Config::HeaderInsertionPolicy::IWYU; @@ -222,7 +226,9 @@ struct CodeCompletion { std::vector<TextEdit> FixIts; /// Holds the range of the token we are going to replace with this completion. - Range CompletionTokenRange; + Range CompletionInsertRange; + /// If set, the range to use when the client's insert mode is "replace". + std::optional<Range> CompletionReplaceRange; // Scores are used to rank completion items. struct Scores { @@ -261,8 +267,12 @@ struct CodeCompleteResult { // The text that is being directly completed. // Example: foo.pb^ -> foo.push_back() // ~~ - // Typically matches the textEdit.range of Completions, but not guaranteed to. - std::optional<Range> CompletionRange; + // Typically matches the textEdit.range (or textEdit.insert range) of + // Completions, but not guaranteed to. + std::optional<Range> InsertRange; + // If not empty, typically matches the textEdit.replace range of Completions, + // but not guaranteed to. + std::optional<Range> ReplaceRange; // Usually the source will be parsed with a real C++ parser. // But heuristics may be used instead if e.g. the preamble is not ready. bool RanParser = true; diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index 793db7b052990..f77b0773d445a 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -202,6 +202,14 @@ llvm::json::Value toJSON(const TextEdit &P) { return Result; } +llvm::json::Value toJSON(const InsertReplaceEdit &P) { + return llvm::json::Object{ + {"newText", P.newText}, + {"insert", P.insert}, + {"replace", P.replace}, + }; +} + bool fromJSON(const llvm::json::Value &Params, ChangeAnnotation &R, llvm::json::Path P) { llvm::json::ObjectMapper O(Params, P); @@ -414,6 +422,8 @@ bool fromJSON(const llvm::json::Value &Params, ClientCapabilities &R, break; } } + if (auto IRSupport = Item->getBoolean("insertReplaceSupport")) + R.InsertReplace = *IRSupport; } if (auto *ItemKind = Completion->getObject("completionItemKind")) { if (auto *ValueSet = ItemKind->get("valueSet")) { @@ -1184,7 +1194,8 @@ llvm::json::Value toJSON(const CompletionItem &CI) { if (CI.insertTextFormat != InsertTextFormat::Missing) Result["insertTextFormat"] = static_cast<int>(CI.insertTextFormat); if (CI.textEdit) - Result["textEdit"] = *CI.textEdit; + Result["textEdit"] = std::visit( + [](const auto &V) { return llvm::json::Value(V); }, *CI.textEdit); if (!CI.additionalTextEdits.empty()) Result["additionalTextEdits"] = llvm::json::Array(CI.additionalTextEdits); if (CI.deprecated) diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 7a99721a1e856..9c1bb9d9bb059 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -34,6 +34,7 @@ #include <memory> #include <optional> #include <string> +#include <variant> #include <vector> // This file is using the LSP syntax for identifier names which is different @@ -261,6 +262,18 @@ bool fromJSON(const llvm::json::Value &, TextEdit &, llvm::json::Path); llvm::json::Value toJSON(const TextEdit &); llvm::raw_ostream &operator<<(llvm::raw_ostream &, const TextEdit &); +struct InsertReplaceEdit { + /// The string to be inserted. + std::string newText; + + /// The range if the insert is requested. + Range insert; + + /// The range if the replace is requested. + Range replace; +}; +llvm::json::Value toJSON(const InsertReplaceEdit &); + struct ChangeAnnotation { /// A human-readable string describing the actual change. The string /// is rendered prominent in the user interface. @@ -510,6 +523,10 @@ struct ClientCapabilities { /// textDocument.completion.completionItem.documentationFormat MarkupKind CompletionDocumentationFormat = MarkupKind::PlainText; + /// Client supports insert replace edit to control different behavior if a + /// completion item is inserted in the text or should replace text. + bool InsertReplace = false; + /// The client has support for completion item label details. /// textDocument.completion.completionItem.labelDetailsSupport. bool CompletionLabelDetail = false; @@ -1372,9 +1389,13 @@ struct CompletionItem { /// An edit which is applied to a document when selecting this completion. /// When an edit is provided `insertText` is ignored. /// - /// Note: The range of the edit must be a single line range and it must - /// contain the position at which completion has been requested. - std::optional<TextEdit> textEdit; + /// Note 1: The text edit's range as well as both ranges from an insert + /// replace edit must be a single line range and must contain the position + /// at which completion has been requested. + /// Note 2: If an `InsertReplaceEdit` is returned, the edit's insert range + /// must be a prefix of the edit's replace range, meaning it must be + /// contained in and starting at the same position. + std:... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/187623 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
