Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue-63565: community requested small QoL fix for more configurabili… #108005

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang-tools-extra/clangd/ClangdServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ namespace {
// Tracks number of times a tweak has been offered.
static constexpr trace::Metric TweakAvailable(
"tweak_available", trace::Metric::Counter, "tweak_id");

// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
UpdateIndexCallbacks(FileIndex *FIndex,
Expand Down Expand Up @@ -451,6 +451,7 @@ void ClangdServer::codeComplete(PathRef File, Position Pos,

CodeCompleteOpts.MainFileSignals = IP->Signals;
CodeCompleteOpts.AllScopes = Config::current().Completion.AllScopes;
CodeCompleteOpts.ArgumentLists = Config::current().Completion.ArgumentLists;
// FIXME(ibiryukov): even if Preamble is non-null, we may want to check
// both the old and the new version in case only one of them matches.
CodeCompleteResult Result = clangd::codeComplete(
Expand Down
35 changes: 22 additions & 13 deletions clang-tools-extra/clangd/CodeComplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "AST.h"
#include "CodeCompletionStrings.h"
#include "Compiler.h"
#include "Config.h"
#include "ExpectedTypes.h"
#include "Feature.h"
#include "FileDistance.h"
Expand Down Expand Up @@ -350,8 +351,7 @@ struct CodeCompletionBuilder {
CodeCompletionContext::Kind ContextKind,
const CodeCompleteOptions &Opts,
bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
: ASTCtx(ASTCtx),
EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
: ASTCtx(ASTCtx), ArgumentLists(Opts.ArgumentLists),
IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
Completion.Deprecated = true; // cleared by any non-deprecated overload.
add(C, SemaCCS, ContextKind);
Expand Down Expand Up @@ -561,14 +561,23 @@ struct CodeCompletionBuilder {
}

std::string summarizeSnippet() const {
/// localize ArgumentLists tests for better readability
const bool None = ArgumentLists == Config::ArgumentListsPolicy::None;
const bool Open =
ArgumentLists == Config::ArgumentListsPolicy::OpenDelimiter;
const bool Delim = ArgumentLists == Config::ArgumentListsPolicy::Delimiters;
const bool Full =
ArgumentLists == Config::ArgumentListsPolicy::FullPlaceholders ||
(!None && !Open && !Delim); // <-- failsafe: Full is default

if (IsUsingDeclaration)
return "";
auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
if (!Snippet)
// All bundles are function calls.
// FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
// we need to complete 'forward<$1>($0)'.
return "($0)";
return None ? "" : (Open ? "(" : "($0)");

if (Snippet->empty())
return "";
Expand Down Expand Up @@ -607,7 +616,7 @@ struct CodeCompletionBuilder {
return "";
}
}
if (EnableFunctionArgSnippets)
if (Full)
return *Snippet;

// Replace argument snippets with a simplified pattern.
Expand All @@ -622,9 +631,9 @@ struct CodeCompletionBuilder {

bool EmptyArgs = llvm::StringRef(*Snippet).ends_with("()");
if (Snippet->front() == '<')
return EmptyArgs ? "<$1>()$0" : "<$1>($0)";
return None ? "" : (Open ? "<" : (EmptyArgs ? "<$1>()$0" : "<$1>($0)"));
if (Snippet->front() == '(')
return EmptyArgs ? "()" : "($0)";
return None ? "" : (Open ? "(" : (EmptyArgs ? "()" : "($0)"));
return *Snippet; // Not an arg snippet?
}
// 'CompletionItemKind::Interface' matches template type aliases.
Expand All @@ -638,7 +647,7 @@ struct CodeCompletionBuilder {
// e.g. Foo<${1:class}>.
if (llvm::StringRef(*Snippet).ends_with("<>"))
return "<>"; // can happen with defaulted template arguments.
return "<$0>";
return None ? "" : (Open ? "<" : "<$0>");
}
return *Snippet;
}
Expand All @@ -654,7 +663,8 @@ struct CodeCompletionBuilder {
ASTContext *ASTCtx;
CodeCompletion Completion;
llvm::SmallVector<BundledEntry, 1> Bundled;
bool EnableFunctionArgSnippets;
/// the way argument lists are handled.
Config::ArgumentListsPolicy ArgumentLists;
// No snippets will be generated for using declarations and when the function
// arguments are already present.
bool IsUsingDeclaration;
Expand Down Expand Up @@ -797,8 +807,8 @@ SpecifiedScope getQueryScopes(CodeCompletionContext &CCContext,
llvm::StringRef SpelledSpecifier = Lexer::getSourceText(
CharSourceRange::getCharRange(SemaSpecifier->getRange()),
CCSema.SourceMgr, clang::LangOptions());
if (SpelledSpecifier.consume_front("::"))
Scopes.QueryScopes = {""};
if (SpelledSpecifier.consume_front("::"))
Scopes.QueryScopes = {""};
Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
// Sema excludes the trailing "::".
if (!Scopes.UnresolvedQualifier->empty())
Expand Down Expand Up @@ -1591,7 +1601,7 @@ class CodeCompleteFlow {
CompletionPrefix HeuristicPrefix;
std::optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
Range ReplacedRange;
std::vector<std::string> QueryScopes; // Initialized once Sema runs.
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.
std::optional<ScopeDistance> ScopeProximity;
Expand Down Expand Up @@ -2387,8 +2397,7 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const CodeCompletion &C) {
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const CodeCompleteResult &R) {
OS << "CodeCompleteResult: " << R.Completions.size() << (R.HasMore ? "+" : "")
<< " (" << getCompletionKindString(R.Context) << ")"
<< " items:\n";
<< " (" << getCompletionKindString(R.Context) << ")" << " items:\n";
for (const auto &C : R.Completions)
OS << C << "\n";
return OS;
Expand Down
9 changes: 5 additions & 4 deletions clang-tools-extra/clangd/CodeComplete.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "ASTSignals.h"
#include "Compiler.h"
#include "Config.h"
#include "Protocol.h"
#include "Quality.h"
#include "index/Index.h"
Expand Down Expand Up @@ -96,17 +97,17 @@ struct CodeCompleteOptions {
/// '->' on member access etc.
bool IncludeFixIts = false;

/// Whether to generate snippets for function arguments on code-completion.
/// Needs snippets to be enabled as well.
bool EnableFunctionArgSnippets = true;

/// Whether to include index symbols that are not defined in the scopes
/// visible from the code completion point. This applies in contexts without
/// explicit scope qualifiers.
///
/// Such completions can insert scope qualifiers.
bool AllScopes = false;

/// The way argument list on calls '()' and generics '<>' are handled.
Config::ArgumentListsPolicy ArgumentLists =
Config::ArgumentListsPolicy::FullPlaceholders;

/// Whether to use the clang parser, or fallback to text-based completion
/// (using identifiers in the current file and symbol indexes).
enum CodeCompletionParse {
Expand Down
15 changes: 15 additions & 0 deletions clang-tools-extra/clangd/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,25 @@ struct Config {
std::vector<std::string> FullyQualifiedNamespaces;
} Style;

/// controls the completion options for argument lists.
enum class ArgumentListsPolicy {
/// nothing, no argument list and also NO Delimiters "()" or "<>".
None,
/// open, only opening delimiter "(" or "<".
OpenDelimiter,
/// empty pair of delimiters "()" or "<>".
Delimiters,
/// full name of both type and variable.
FullPlaceholders,
};

/// Configures code completion feature.
struct {
/// Whether code completion includes results that are not visible in current
/// scopes.
bool AllScopes = true;
/// controls the completion options for argument lists.
ArgumentListsPolicy ArgumentLists = ArgumentListsPolicy::FullPlaceholders;
} Completion;

/// Configures hover feature.
Expand All @@ -150,6 +164,7 @@ struct Config {
bool BlockEnd = false;
// Limit the length of type names in inlay hints. (0 means no limit)
uint32_t TypeNameLimit = 32;

} InlayHints;

struct {
Expand Down
15 changes: 15 additions & 0 deletions clang-tools-extra/clangd/ConfigCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,21 @@ struct FragmentCompiler {
C.Completion.AllScopes = AllScopes;
});
}
if (F.ArgumentLists) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a compileEnum helper to do this more easily, see the handling of BackgroundPolicy as an example

if (auto Val =
compileEnum<Config::ArgumentListsPolicy>("ArgumentLists",
*F.ArgumentLists)
.map("None", Config::ArgumentListsPolicy::None)
.map("OpenDelimiter",
Config::ArgumentListsPolicy::OpenDelimiter)
.map("Delimiters", Config::ArgumentListsPolicy::Delimiters)
.map("FullPlaceholders",
Config::ArgumentListsPolicy::FullPlaceholders)
.value())
Out.Apply.push_back([Val](const Params &, Config &C) {
C.Completion.ArgumentLists = *Val;
});
}
}

void compile(Fragment::HoverBlock &&F) {
Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/clangd/ConfigFragment.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H

#include "Config.h"
#include "ConfigProvider.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
Expand Down Expand Up @@ -308,6 +309,13 @@ struct Fragment {
/// Whether code completion should include suggestions from scopes that are
/// not visible. The required scope prefix will be inserted.
std::optional<Located<bool>> AllScopes;
/// How to present the argument list between '()' and '<>':
/// valid values are enum Config::ArgumentListsPolicy values:
/// None: Nothing at all
/// OpenDelimiter: only opening delimiter "(" or "<"
/// Delimiters: empty pair of delimiters "()" or "<>"
/// FullPlaceholders: full name of both type and parameter
std::optional<Located<std::string>> ArgumentLists;
};
CompletionBlock Completion;

Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/ConfigYAML.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ class Parser {
if (auto AllScopes = boolValue(N, "AllScopes"))
F.AllScopes = *AllScopes;
});
Dict.handle("ArgumentLists", [&](Node &N) {
if (auto ArgumentLists = scalarValue(N, "ArgumentLists"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since F.ArgumentLists is itself an optional, this can be simplified to:

F.ArgumentLists = scalarValue(N, "ArgumentLists");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this comment remains to be addressed)

F.ArgumentLists = *ArgumentLists;
});
Dict.parse(N);
}

Expand Down
10 changes: 8 additions & 2 deletions clang-tools-extra/clangd/tool/ClangdMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ opt<bool> EnableFunctionArgSnippets{
desc("When disabled, completions contain only parentheses for "
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
init(true),
};

opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
Expand Down Expand Up @@ -650,6 +650,7 @@ class FlagsConfigProvider : public config::Provider {
std::optional<Config::CDBSearchSpec> CDBSearch;
std::optional<Config::ExternalIndexSpec> IndexSpec;
std::optional<Config::BackgroundPolicy> BGPolicy;
std::optional<Config::ArgumentListsPolicy> ArgumentLists;

// If --compile-commands-dir arg was invoked, check value and override
// default path.
Expand Down Expand Up @@ -694,13 +695,19 @@ class FlagsConfigProvider : public config::Provider {
BGPolicy = Config::BackgroundPolicy::Skip;
}

if (!EnableFunctionArgSnippets) {
ArgumentLists = Config::ArgumentListsPolicy::Delimiters;
}

Frag = [=](const config::Params &, Config &C) {
if (CDBSearch)
C.CompileFlags.CDBSearch = *CDBSearch;
if (IndexSpec)
C.Index.External = *IndexSpec;
if (BGPolicy)
C.Index.Background = *BGPolicy;
if (ArgumentLists)
C.Completion.ArgumentLists = *ArgumentLists;
if (AllScopesCompletion.getNumOccurrences())
C.Completion.AllScopes = AllScopesCompletion;

Expand Down Expand Up @@ -916,7 +923,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
Opts.CodeComplete.IncludeIndicator.Insert.clear();
Opts.CodeComplete.IncludeIndicator.NoInsert.clear();
}
Opts.CodeComplete.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
Opts.CodeComplete.RunParser = CodeCompletionParse;
Opts.CodeComplete.RankingModel = RankingModel;

Expand Down
45 changes: 33 additions & 12 deletions clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "ClangdServer.h"
#include "CodeComplete.h"
#include "Compiler.h"
#include "Config.h"
#include "Feature.h"
#include "Matchers.h"
#include "Protocol.h"
Expand Down Expand Up @@ -1137,8 +1138,8 @@ TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
}

TEST(CompletionTests, EmptySnippetDoesNotCrash) {
// See https://github.com/clangd/clangd/issues/1216
auto Results = completions(R"cpp(
// See https://github.com/clangd/clangd/issues/1216
auto Results = completions(R"cpp(
int main() {
auto w = [&](auto &&f) { return f(f); };
auto f = w([&](auto &&f) {
Expand All @@ -1154,18 +1155,18 @@ TEST(CompletionTests, EmptySnippetDoesNotCrash) {
}

TEST(CompletionTest, Issue1427Crash) {
// Need to provide main file signals to ensure that the branch in
// SymbolRelevanceSignals::computeASTSignals() that tries to
// compute a symbol ID is taken.
ASTSignals MainFileSignals;
CodeCompleteOptions Opts;
Opts.MainFileSignals = &MainFileSignals;
completions(R"cpp(
// Need to provide main file signals to ensure that the branch in
// SymbolRelevanceSignals::computeASTSignals() that tries to
// compute a symbol ID is taken.
ASTSignals MainFileSignals;
CodeCompleteOptions Opts;
Opts.MainFileSignals = &MainFileSignals;
completions(R"cpp(
auto f = []() {
1.0_^
};
)cpp",
{}, Opts);
{}, Opts);
}

TEST(CompletionTest, BacktrackCrashes) {
Expand Down Expand Up @@ -2595,10 +2596,10 @@ TEST(SignatureHelpTest, DynamicIndexDocumentation) {
ElementsAre(AllOf(sig("foo() -> int"), sigDoc("Member doc"))));
}

TEST(CompletionTest, CompletionFunctionArgsDisabled) {
TEST(CompletionTest, ArgumentListsPolicy) {
CodeCompleteOptions Opts;
Opts.EnableSnippets = true;
Opts.EnableFunctionArgSnippets = false;
Opts.ArgumentLists = Config::ArgumentListsPolicy::Delimiters;

{
auto Results = completions(
Expand Down Expand Up @@ -2670,6 +2671,26 @@ TEST(CompletionTest, CompletionFunctionArgsDisabled) {
EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
named("FOO"), snippetSuffix("($0)"))));
}
{
Opts.ArgumentLists = Config::ArgumentListsPolicy::None;
auto Results = completions(
R"cpp(
void xfoo(int x, int y);
void f() { xfo^ })cpp",
{}, Opts);
EXPECT_THAT(Results.Completions,
UnorderedElementsAre(AllOf(named("xfoo"), snippetSuffix(""))));
}
{
Opts.ArgumentLists = Config::ArgumentListsPolicy::OpenDelimiter;
auto Results = completions(
R"cpp(
void xfoo(int x, int y);
void f() { xfo^ })cpp",
{}, Opts);
EXPECT_THAT(Results.Completions,
UnorderedElementsAre(AllOf(named("xfoo"), snippetSuffix("("))));
}
}

TEST(CompletionTest, SuggestOverrides) {
Expand Down
Loading