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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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.

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.



================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:202
           if (Warn)
-            Outer->warning("Unknown " + Description + " key " + **Key, *K);
+            UnknownKeys.push_back(std::move(*Key));
         }
----------------
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.


================
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;
----------------
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)


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:221
+    getBestGuess(llvm::StringRef Search, llvm::ArrayRef<llvm::StringRef> Items,
+                 unsigned MaxEdit = 0) {
+
----------------
nit: drop unused default param


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:223
+
+      // If Search is sufficiently short (size() < 2), just return nothing.
+      if (Search.size() < 2)
----------------
Comment here just echoes the code - remove?


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:231
+        unsigned EditDistance = Search.edit_distance(Item, true, MaxEdit);
+        if (EditDistance == MaxEdit) {
+          if (!Result)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990