issue-63565: community requested small QoL fix for more configurabili… #108005
Conversation
…ty on --function-arg-placeholders
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: None (MK-Alias) ChangesThis is a small Quality of Life patch. It adds more configurability to The current issue for this is: 63565. I say current, because more issues where made on this topic. My original issue was: 390. Some of my motivations can be read in that thread: here and here You might want to checkout the outputted help text, which is set in I'll be awaiting your reply or merger. Thanks! Full diff: https://github.com/llvm/llvm-project/pull/108005.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 89eee392837af4..9fb264ef9160b3 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -350,8 +350,7 @@ struct CodeCompletionBuilder {
CodeCompletionContext::Kind ContextKind,
const CodeCompleteOptions &Opts,
bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
- : ASTCtx(ASTCtx),
- EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
+ : ASTCtx(ASTCtx), PlaceholderType(Opts.PlaceholderType),
IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
Completion.Deprecated = true; // cleared by any non-deprecated overload.
add(C, SemaCCS, ContextKind);
@@ -561,6 +560,14 @@ struct CodeCompletionBuilder {
}
std::string summarizeSnippet() const {
+ /// localize PlaceholderType for better readability
+ const bool None = PlaceholderType == CodeCompleteOptions::None;
+ const bool Open = PlaceholderType == CodeCompleteOptions::OpenDelimiter;
+ const bool Delim = PlaceholderType == CodeCompleteOptions::Delimiters;
+ const bool Full =
+ PlaceholderType == CodeCompleteOptions::FullPlaceholders ||
+ (!None && !Open && !Delim); // <-- failsafe
+
if (IsUsingDeclaration)
return "";
auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
@@ -568,7 +575,7 @@ struct CodeCompletionBuilder {
// 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 "";
@@ -607,7 +614,7 @@ struct CodeCompletionBuilder {
return "";
}
}
- if (EnableFunctionArgSnippets)
+ if (Full)
return *Snippet;
// Replace argument snippets with a simplified pattern.
@@ -622,9 +629,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" : "($0)"));
return *Snippet; // Not an arg snippet?
}
// 'CompletionItemKind::Interface' matches template type aliases.
@@ -638,7 +645,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;
}
@@ -654,7 +661,7 @@ struct CodeCompletionBuilder {
ASTContext *ASTCtx;
CodeCompletion Completion;
llvm::SmallVector<BundledEntry, 1> Bundled;
- bool EnableFunctionArgSnippets;
+ CodeCompleteOptions::PlaceholderOption PlaceholderType;
// No snippets will be generated for using declarations and when the function
// arguments are already present.
bool IsUsingDeclaration;
@@ -797,8 +804,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())
@@ -1591,7 +1598,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;
@@ -2387,8 +2394,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;
diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h
index a7c1ae95dcbf49..fc27a1c7eefc94 100644
--- a/clang-tools-extra/clangd/CodeComplete.h
+++ b/clang-tools-extra/clangd/CodeComplete.h
@@ -96,9 +96,21 @@ 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;
+ // requested by community in favour of 'EnableFunctionArgSnippets',
+ // see here for more info:
+ // https://github.com/llvm/llvm-project/issues/63565#issuecomment-1975065771
+ /// Controls how the delimter/argument-list for callables: "()"
+ /// and for generics: "<>" are handled
+ enum PlaceholderOption {
+ /// nothing, no argument list and also NO Delimiters "()" or "<>"
+ None = 0,
+ /// open, only opening delimiter "(" or "<"
+ OpenDelimiter,
+ /// empty pair of delimiters "()" or "<>" (or [legacy] alias 0)
+ Delimiters,
+ /// full name of both type and variable (or [legacy] alias 1)
+ FullPlaceholders,
+ } PlaceholderType = PlaceholderOption::FullPlaceholders;
/// Whether to include index symbols that are not defined in the scopes
/// visible from the code completion point. This applies in contexts without
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 3a5449ac8c7994..a186f314819664 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -242,13 +242,31 @@ opt<std::string> FallbackStyle{
init(clang::format::DefaultFallbackStyle),
};
-opt<bool> EnableFunctionArgSnippets{
- "function-arg-placeholders",
- cat(Features),
- desc("When disabled, completions contain only parentheses for "
- "function calls. When enabled, completions also contain "
- "placeholders for method parameters"),
- init(CodeCompleteOptions().EnableFunctionArgSnippets),
+opt<CodeCompleteOptions::PlaceholderOption> PlaceholderOption{
+ "function-arg-placeholders", cat(Features),
+ desc("Set the way placeholders and delimiters are implemented."),
+ init(CodeCompleteOptions().PlaceholderType),
+ values(
+ clEnumValN(CodeCompleteOptions::PlaceholderOption::None, "None",
+ "insert nothing, not even the delimiters \"()\" or "
+ "\"<>\": void foo"),
+ clEnumValN(CodeCompleteOptions::PlaceholderOption::OpenDelimiter,
+ "OpenDelimiter",
+ "Only insert opening delimiter \"(\" or \"<\", no "
+ "placeholders: void foo("),
+ clEnumValN(CodeCompleteOptions::PlaceholderOption::Delimiters,
+ "Delimiters",
+ "Only insert delimiters \"()\" and \"<>\", no placeholders: "
+ "void foo()"),
+ clEnumValN(
+ CodeCompleteOptions::PlaceholderOption::FullPlaceholders,
+ "FullPlaceholders",
+ "[default] Use full type names and value names: void foo(int x)"),
+ clEnumValN(CodeCompleteOptions::PlaceholderOption::Delimiters, "0",
+ "[deprecated] use: Delimiters instead"),
+ clEnumValN(CodeCompleteOptions::PlaceholderOption::FullPlaceholders,
+ "1", "[deprecated] use: FullPlaceholders instead"))
+
};
opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
@@ -916,7 +934,7 @@ 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.PlaceholderType = PlaceholderOption;
Opts.CodeComplete.RunParser = CodeCompletionParse;
Opts.CodeComplete.RankingModel = RankingModel;
|
There was a problem hiding this comment.
Thanks for putting together this patch!
Two high-level pieces of feedback:
- The current patch implements the options as additional values of the
--function-arg-placeholderscommand-line flag. However, the proposal in this comment was to make these values of a new config file key calledArgumentLists, underCompletion. (This involves a bit of boilerplate; it helps to look at a previous patch which adds a new config key, such as this one.) - It would be great to have some unit tests exercising the options. We have an existing unit test exercising the current boolean flag here which can be extended as appropriate.
|
Thanks for the feedback. Will update the patch soon! (I've got lots of stuff to do, but it's high on my todo-list!) Just to be clear, do you want to keep the new |
The latter. The only change related to the command-line flag that's needed, is to translate its (still boolean) value into the new (enum-valued) config option; we have a class named FlagsConfigProvider which helps with that (so that the rest of the code only needs to care about the config option). |
|
Thanks for the input, a followup question is: Which one takes precedence? Does the |
If you are handling the flag in |
|
Because the Completion:
AllScopes: None
ArgumentLists: NoneIt seems like the most logical choice, because it's an auto-completion feature! Is this okay!? Or must it go in an other block!? |
Yep. By the way, this was specified in the original proposal:
|
|
Updated the source. It should now confirm to everything. To recap: UnitTest have also been updated. Other then the tests already in place, it will also check the As mentions in this post: these to issues remain within vsocde/vscode-clangd:
The OpenDelimiter issue seems like a small fix and would generally improve the UX on this. Ill be awaiting your response. Edit: the builds are failing because of a case type. Will update it soon... |
clang-tools-extra/clangd/Config.h
Outdated
| @@ -126,11 +126,29 @@ struct Config { | |||
| std::vector<std::string> FullyQualifiedNamespaces; | |||
| } Style; | |||
|
|
|||
| /// controls the completion options for argument lists. | |||
| enum class ArgumentListsOption { | |||
There was a problem hiding this comment.
naming suggestion: ArgumentListsPolicy, for consistency with other enums defined in this file
clang-tools-extra/clangd/Config.h
Outdated
| /// the default value. This will imply FullPlaceholders unless overridden by | ||
| /// --function-arg-placeholders=0, in which case Delimiters is used. | ||
| /// any other use-case of --function-arg-placeholders is ignored | ||
| UnsetDefault = 0, |
There was a problem hiding this comment.
This value isn't necessary.
The way config providers interact, using BackgroundPolicy (an existing config option which also exists as a command-line flag) as an example:
- A
Configobject is created FlagsConfigProviderruns first. If the relevant command-line flag (--background-index) was specified, it assigns the provided value to theIndex.Backgroundfield of theConfigobject created at step (1). Note, if the flag wasn't provided, this line doesn't run at all.- Then the config provider for the config file runs. The data structure representing data parsed from the file stores everything using
std::optional, which is only populated if the key is present in the config file. That then overwrites theIndex.Backgroundfield of the sameConfigobject, but once again this line only runs at all if theoptionalwas populated.
As a result, no "unset" value is needed; std::optional takes care of that. The default value of Completion::ArgumentLists below -- used if neither a command-line flag nor a config file key was specified -- should be FullPlaceholders.
| "Latency of clang-tidy check '{0}' is not known. " | ||
| "It will only run if ClangTidy.FastCheckFilter is Loose or None", | ||
| Str) | ||
| llvm::formatv("Latency of clang-tidy check '{0}' is not known. " |
There was a problem hiding this comment.
nit: I'm seeing some diff hunks containing unrelated formatting changes. If would be great to remove this as they pollute the blame
There was a problem hiding this comment.
I use clang-format with the .clang-format of the code base. Even if I don't change anything and just do a clang-format this change will happen. It seems that it's not correctly formatted. I will fix this and my recommendation would be to use clang-format on this file - or perhaps the whole code base - post patching in its own commit. this will fix this issue(s) in the future.
| @@ -622,6 +622,31 @@ struct FragmentCompiler { | |||
| C.Completion.AllScopes = AllScopes; | |||
| }); | |||
| } | |||
| if (F.ArgumentLists) { | |||
There was a problem hiding this comment.
We have a compileEnum helper to do this more easily, see the handling of BackgroundPolicy as an example
| /// None: Nothing at all | ||
| /// OpenDelimiter: only opening delimiter "(" or "<" | ||
| /// Delimiters: empty pair of delimiters "()" or "<>" | ||
| /// FullPlaceholders: full name of both type and variable |
There was a problem hiding this comment.
nit: variable --> parameter
| "function calls. When enabled, completions also contain " | ||
| "placeholders for method parameters"), | ||
| init(CodeCompleteOptions().EnableFunctionArgSnippets), | ||
| opt<Config::ArgumentListsOption> PlaceholderOption{ |
There was a problem hiding this comment.
For simplicity and consistency with --background-index, let's keep the name and type of this unchanged
| 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 == |
There was a problem hiding this comment.
For reasons mentioned above, no need for the second condition here (at this point, the config provider for the config file has not yet assigned anything to C; when it does run later, it will overwrite the field if needed)
|
|
||
| /// localize a ArgList depending on the config. If the config is unset we | ||
| /// will use our local (this) version, else use the one of the config | ||
| const Config::ArgumentListsOption ArgList = |
There was a problem hiding this comment.
For consistency with our existing Completion option (AllScopes), let's do the Config lookup here and propagate it into CodeCompleteOpts.
That then gets propagated to CodeCompletionBuilder::ArgumentLists in the CodeCompletionBuilder constructor, and here we can just use the ArgumentLists member without doing anything further.
| @@ -6,6 +6,7 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "../Config.h" | |||
There was a problem hiding this comment.
This can be included without the ../. (Please alphabetize it among the existing includes.)
There was a problem hiding this comment.
This can be included without the ../
Yes, but it of-course shouldn't! Confirm the standard it should be either: #include "../Config.h" or #include <Config.h> (and also it should be Config.hpp)
But because this is used in the whole codebase, if changed it to #include "Config.h"
| @@ -2595,10 +2596,10 @@ TEST(SignatureHelpTest, DynamicIndexDocumentation) { | |||
| ElementsAre(AllOf(sig("foo() -> int"), sigDoc("Member doc")))); | |||
| } | |||
|
|
|||
| TEST(CompletionTest, CompletionFunctionArgsDisabled) { | |||
| TEST(CompletionTest, ArgumentListsNotFullPlaceholders) { | |||
There was a problem hiding this comment.
nit: suggestion for slightly more general test name: ArgumentListsPolicy
clang-tools-extra/clangd/Config.h
Outdated
| None, | ||
| /// open, only opening delimiter "(" or "<" | ||
| OpenDelimiter, | ||
| /// empty pair of delimiters "()" or "<>" (or [legacy] alias 0) |
There was a problem hiding this comment.
nit: these mentions of legacy aliases can be removed (it's just confusing since that applies to the command line flag which is defined elsewhere)
Could you file a new issue for each of these please? |
|
Here a quick summery of the most important things. The reason I've implemented a
I now have a version which I belief is fully compliant to all your critique., however this means that the This would be confirm the overlaying logic of the program. So I would recommend just accept this.! If someone ever wants to change the parsing order, our new I will push the new version in a couple of minutes for you to evaluate. |
Sure! Will do so when the our changes have been merged! |
|
Note: because the command line argument is reverted back to its original boolean: |
Thanks for catching that; I was under the mistaken impression that it was the other way around! I think it would be a nicer user experience if the config file took precedence over compile flags... but since that's not the case today for existing flags (like In other words, I think what the current version of the patch is doing is fine. |
You can test this locally with the following command:git-clang-format --diff 925b220ee424f8489bc8d7b1a247f2c5f3edde5d 49ca6ddee1003aaf58f076da5c2ee6767acbf939 --extensions h,cpp -- clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clangd/CodeComplete.h clang-tools-extra/clangd/Config.h clang-tools-extra/clangd/ConfigCompile.cpp clang-tools-extra/clangd/ConfigFragment.h clang-tools-extra/clangd/ConfigYAML.cpp clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 547f988b87..9b38be04e7 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -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,
|
There was a problem hiding this comment.
Thanks, this is looking pretty good. My only remaining request is to please split the formatting (whitespace) changes out. I know it's the fault of the code not being clang-format clean, and I'm happy to merge them in a separate PR, but it confuses things when "git blame" says that the last commit that touched some unrelated test / code line is this one about ArgumentLists.
| @@ -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")) | |||
There was a problem hiding this comment.
(this comment remains to be addressed)
The whole idea of command-line arguments is to override defaults at execution time. Config-files should be persistent part of the config, and to override the current config, you can use - non persistent - command-line arguments... That's the whole idea behind it is so you can run a command with a certain intent, The other way around will render the command line useless, or might have unexpected behavior. (say: supplying a command line argument and it then not doing anything)
I really don't know what you mean here!? can you elaborate? what do you want me to do? I'm pretty happy with the patch, as-is. Only the boolean on the command-line
And it's not confusing if "git blame" says that this same part of code is there in a "clang-format commit". Instead of it's original? The world is simply not perfect. The git blame is going to change these lines anyways. A choice have been made to use clang-format. The source currently is not properly formatted because the previous commits where not properly formatted, or a newer version of clang-format has a different interpretations of how to format the code. Me now having to first format the code to comply to the clang-format requirement, and then having to unformat other parts of the source is a absolute annoyance! And also the reason why the clang-format check is now failing. The irony is that people choose to use a code-formatter to not have to discuss or deal with code formatting, and then we now have to discuss the code formatting. Code-formatters have their pros & cons, and this one of the cons, and personally I would argue that this issue is just "collateral damage" of using a code-formatter. Things like these can and will happen from time to time, if the code isn't properly formatted, or the formatter [clang-format] gets updated. It's simply inherit of the choice of using a formatter. I will commit the newest version of the patch, which will have the mentioned int value on A: Merge it as is (recommended: this follows the guide) Effected files in this PR are: So if you which to first clang-format before merging, you technically only have to do these. |
I would say that's less confusing, because you know you can just skip that sort of commit in the blame. I'm not trying to be difficult here; "please remove unrelated formatting changes" is something I've been told by other reviewers in response to my own patches. |
|
This is too bad. I was just looking for this feature. |
|
I've resubmitted this as #111322. |
This is a small Quality of Life patch. It adds more configurability to
--function-arg-placeholders.The current issue for this is: 63565. I say current, because more issues where made on this topic. My original issue was: 390. Some of my motivations can be read in that thread: here and here
You might want to checkout the outputted help text, which is set in
ClangdMain.cpp. It is not bad, but perhaps give it a last critical look!?I'll be awaiting your reply or merger.
Thanks!