[PATCH] D92990: [clangd] Provide suggestions with invalid config keys

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D92990#2446097 <https://reviews.llvm.org/D92990#2446097>, @sammccall wrote:

> This is cool! Detecting basic typos could save a fair bit of time.
> I do want to be a bit wary of spending too much complexity on small enhancements to rarely used code - if it's more than a simple typo we're correcting, then I think it's ok to make the user look it up.

I always like to think users don't spend much time reading the instructions, so little pointers like this do add value and result in a more polished experience.

> In D92990#2445283 <https://reviews.llvm.org/D92990#2445283>, @njames93 wrote:
>
>> Adding in support for fix-its(at least in vscode) didn't seem to work. The language server there doesn't seem to send the `textDocument/codeAction` message to the server for the `.clangd` file.
>
> Right, we can publish to any file, but we only expect to get requests for files we're active on, and users will expect us to track draft changes etc. I don't think we want to go down the path of being a full language server for config files, so I wouldn't pursue fixit support.

Ahh I don't know the full ins and outs of LSP. As nice as a fix-it would be, It appears that's a little too much work so should best be avoided.



================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:202
           if (Warn)
-            Outer->warning("Unknown " + Description + " key " + **Key, *K);
+            UnknownKeys.push_back(std::move(*Key));
         }
----------------
sammccall wrote:
> i'm not sure it's actually worth the complexity to delay this, track the seen keys, and exclude them from the analysis.
> Offering a suggestion already seems pretty good, but I don't think we should try too hard to make it perfect.
Perfection isn't needed, but suggesting a fix that will result in another error is arguably counter-productive.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:211
+    llvm::SmallVector<llvm::StringRef, 4>
+    getUnseenKeys(const llvm::SmallSet<std::string, 8> &SeenKeys) const {
+      llvm::SmallVector<llvm::StringRef, 4> Result;
----------------
sammccall wrote:
> nit: drop explicit '4' size here
> also drop `get` prefix here and for best-guess (matching the local style for functions used for value rather than for side-effect)
Forgot about the new default SmallVector storage feature.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:231
+        unsigned EditDistance = Search.edit_distance(Item, true, MaxEdit);
+        if (EditDistance == MaxEdit) {
+          if (!Result)
----------------
sammccall wrote:
> This branch is confusing.
> It seems to be saying if we have a tie for best, don't return either result. Why is that important/worth any complexity?
If we have 2 (or more) possible solutions, we can't say for certainty what the user wanted, in which case its best to not suggest anything, in case its incorrect and instead force the user to take an informed look. It doesn't really add much complexity to do this check, And most of this code is copied from somewhere else in llvm as its a pretty common routine. Maybe they could be coalesced into 1 class to reduce duplication,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92990/new/

https://reviews.llvm.org/D92990