[clangd] Provide suggestions with invalid config keys

Update the config file warning when an unknown key is detected which is likely a typo by suggesting the likely key.
This won't suggest a key that has already been seen in the block.

Appends the fix to the diag, however right now there is no support for presenting that fix to the user.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D92990

diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 022d890..20cdc0b 100644 (file)
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -24,6 +24,29 @@ using llvm::yaml::Node;
 using llvm::yaml::ScalarNode;
 using llvm::yaml::SequenceNode;
 
+llvm::Optional<llvm::StringRef>
+bestGuess(llvm::StringRef Search,
+          llvm::ArrayRef<llvm::StringRef> AllowedValues) {
+  unsigned MaxEdit = (Search.size() + 1) / 3;
+  if (!MaxEdit)
+    return llvm::None;
+  llvm::Optional<llvm::StringRef> Result;
+  for (const auto &AllowedValue : AllowedValues) {
+    unsigned EditDistance = Search.edit_distance(AllowedValue, true, MaxEdit);
+    // We can't do better than an edit distance of 1, so just return this and
+    // save computing other values.
+    if (EditDistance == 1U)
+      return AllowedValue;
+    if (EditDistance == MaxEdit && !Result) {
+      Result = AllowedValue;
+    } else if (EditDistance < MaxEdit) {
+      Result = AllowedValue;
+      MaxEdit = EditDistance;
+    }
+  }
+  return Result;
+}
+
 class Parser {
   llvm::SourceMgr &SM;
   bool HadError = false;
@@ -167,6 +190,7 @@ private:
         return;
       }
       llvm::SmallSet<std::string, 8> Seen;
+      llvm::SmallVector<Located<std::string>, 0> UnknownKeys;
       // We *must* consume all items, even on error, or the parser will assert.
       for (auto &KV : llvm::cast<MappingNode>(N)) {
         auto *K = KV.getKey();
@@ -198,9 +222,30 @@ private:
             Warn = UnknownHandler(
                 Located<std::string>(**Key, K->getSourceRange()), *Value);
           if (Warn)
-            Outer->warning("Unknown " + Description + " key " + **Key, *K);
+            UnknownKeys.push_back(std::move(*Key));
         }
       }
+      if (!UnknownKeys.empty())
+        warnUnknownKeys(UnknownKeys, Seen);
+    }
+
+  private:
+    void warnUnknownKeys(llvm::ArrayRef<Located<std::string>> UnknownKeys,
+                         const llvm::SmallSet<std::string, 8> &SeenKeys) const {
+      llvm::SmallVector<llvm::StringRef> UnseenKeys;
+      for (const auto &KeyAndHandler : Keys)
+        if (!SeenKeys.count(KeyAndHandler.first.str()))
+          UnseenKeys.push_back(KeyAndHandler.first);
+
+      for (const Located<std::string> &UnknownKey : UnknownKeys)
+        if (auto BestGuess = bestGuess(*UnknownKey, UnseenKeys))
+          Outer->warning("Unknown " + Description + " key '" + *UnknownKey +
+                             "'; did you mean '" + *BestGuess + "'?",
+                         UnknownKey.Range);
+        else
+          Outer->warning("Unknown " + Description + " key '" + *UnknownKey +
+                             "'",
+                         UnknownKey.Range);
     }
   };
 
@@ -238,16 +283,20 @@ private:
   }
 
   // Report a "hard" error, reflecting a config file that can never be valid.
-  void error(const llvm::Twine &Msg, const Node &N) {
+  void error(const llvm::Twine &Msg, llvm::SMRange Range) {
     HadError = true;
-    SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Error, Msg,
-                    N.getSourceRange());
+    SM.PrintMessage(Range.Start, llvm::SourceMgr::DK_Error, Msg, Range);
+  }
+  void error(const llvm::Twine &Msg, const Node &N) {
+    return error(Msg, N.getSourceRange());
   }
 
   // Report a "soft" error that could be caused by e.g. version skew.
+  void warning(const llvm::Twine &Msg, llvm::SMRange Range) {
+    SM.PrintMessage(Range.Start, llvm::SourceMgr::DK_Warning, Msg, Range);
+  }
   void warning(const llvm::Twine &Msg, const Node &N) {
-    SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Warning, Msg,
-                    N.getSourceRange());
+    return warning(Msg, N.getSourceRange());
   }
 };
 

diff --git a/clang-tools-extra/clangd/test/config.test b/clang-tools-extra/clangd/test/config.test
index eb556fe..3d5e2bb 100644 (file)
--- a/clang-tools-extra/clangd/test/config.test
+++ b/clang-tools-extra/clangd/test/config.test
@@ -19,7 +19,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "Unknown Config key Foo",
+# CHECK-NEXT:        "message": "Unknown Config key 'Foo'",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 3,

diff --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
index 9c45266..899896d 100644 (file)
--- a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@ CompileFlags:
   Unknown: 42
 )yaml";
 
+const char *AddFooWithTypoErr = R"yaml(
+CompileFlags:
+  Add: foo
+  Removr: 42
+)yaml";
+
 const char *AddBarBaz = R"yaml(
 CompileFlags:
   Add: bar
@@ -95,7 +101,7 @@ TEST(ProviderTest, FromYAMLFile) {
   auto P = Provider::fromYAMLFile(testPath("foo.yaml"), /*Directory=*/"", FS);
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
-              ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+              ElementsAre(DiagMessage("Unknown CompileFlags key 'Unknown'")));
   EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
   Diags.clear();
@@ -105,6 +111,16 @@ TEST(ProviderTest, FromYAMLFile) {
   EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
+  FS.Files["foo.yaml"] = AddFooWithTypoErr;
+  Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(
+      Diags.Diagnostics,
+      ElementsAre(DiagMessage(
+          "Unknown CompileFlags key 'Removr'; did you mean 'Remove'?")));
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+  Diags.clear();
+
   FS.Files["foo.yaml"] = AddBarBaz;
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
@@ -143,7 +159,7 @@ TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
 
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
-              ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+              ElementsAre(DiagMessage("Unknown CompileFlags key 'Unknown'")));
   // FIXME: fails on windows: paths have mixed slashes like C:\a/b\c.yaml
   EXPECT_THAT(Diags.Files,
               ElementsAre(testPath("a/foo.yaml"), testPath("a/b/c/foo.yaml")));
@@ -178,7 +194,7 @@ TEST(ProviderTest, Staleness) {
   FS.Files["foo.yaml"] = AddFooWithErr;
   auto Cfg = P->getConfig(StaleOK, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
-              ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+              ElementsAre(DiagMessage("Unknown CompileFlags key 'Unknown'")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
   Diags.clear();
 

diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index 54ef7d1..4039a9b 100644 (file)
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@ CompileFlags: {$unexpected^
 
   ASSERT_THAT(
       Diags.Diagnostics,
-      ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"),
+      ElementsAre(AllOf(DiagMessage("Unknown If key 'UnknownCondition'"),
                         DiagKind(llvm::SourceMgr::DK_Warning),
                         DiagPos(YAML.range("unknown").start),
                         DiagRange(YAML.range("unknown"))),