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

[Nathan James via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcfa1010c4242: [clangd] Provide suggestions with invalid 
config keys (authored by njames93).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/test/config.test
===
--- clang-tools-extra/clangd/test/config.test
+++ 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,
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -24,6 +24,29 @@
 using llvm::yaml::ScalarNode;
 using llvm::yaml::SequenceNode;
 
+llvm::Optional
+bestGuess(llvm::StringRef Search,
+  llvm::ArrayRef AllowedValues) {
+  unsigned MaxEdit = (Search.size() + 1) / 3;
+  if (!MaxEdit)
+return llvm::None;
+  llvm::Optional 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)
+  r

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

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 311949.
njames93 marked 2 inline comments as done.
njames93 added a comment.

Last nits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/test/config.test
===
--- clang-tools-extra/clangd/test/config.test
+++ 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,
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -24,6 +24,29 @@
 using llvm::yaml::ScalarNode;
 using llvm::yaml::SequenceNode;
 
+llvm::Optional
+bestGuess(llvm::StringRef Search,
+  llvm::ArrayRef AllowedValues) {
+  unsigned MaxEdit = (Search.size() + 1) / 3;
+  if (!MaxEdit)
+return llvm::None;
+  llvm::Optional 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 (

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks!




Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:27
 
+static llvm::Optional
+bestGuess(llvm::StringRef Search,

(nit: no need for static in anon namespace)



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:292
+  // Report a "hard" error, reflecting a config file that can never be valid.
+  void error(const llvm::Twine &Msg, const Node &N) {
+return error(Msg, N.getSourceRange());

optional nit: these node variants are now just convenience wrappers, I'd drop 
the comment from them and group them accordingly with whitespace, up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 311934.
njames93 marked 3 inline comments as done.
njames93 added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/test/config.test
===
--- clang-tools-extra/clangd/test/config.test
+++ 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,
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -24,6 +24,29 @@
 using llvm::yaml::ScalarNode;
 using llvm::yaml::SequenceNode;
 
+static llvm::Optional
+bestGuess(llvm::StringRef Search,
+  llvm::ArrayRef AllowedValues) {
+  unsigned MaxEdit = (Search.size() + 1) / 3;
+  if (!MaxEdit)
+return llvm::None;
+  llvm::Optional 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;
+ 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: hokein.
sammccall added a comment.

Sorry about being slow here.
Just a couple of suggestions to avoid cluttering the config parsing bits too 
much, and IIUC fixits can be removed.




Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:211
+llvm::SmallVector
+unseenKeys(const llvm::SmallSet &SeenKeys) const {
+  llvm::SmallVector Result;

I think inlining unseenKeys into warnUnknownKeys would be more readable by 
isolating the mechanics of typo correction into fewer places.



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:219
+
+static llvm::Optional
+bestGuess(llvm::StringRef Search,

I think we can pull this function out of this class, since it doesn't have much 
to do with config parsing. (Probably just anonymous above - it's not so 
reusable since candidates aren't always just strings)



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:291
+  void error(const llvm::Twine &Msg, llvm::SMRange Range,
+ llvm::ArrayRef Fixes = {}) {
 HadError = true;

please remove the SMFixIt bits, since we don't have any way to use them



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:231
+unsigned EditDistance = Search.edit_distance(Item, true, MaxEdit);
+if (EditDistance == MaxEdit) {
+  if (!Result)

njames93 wrote:
> 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,
Well, there isn't *any* case where we can say for certain that we know what the 
user wanted.

If this were a common library then that shifts the complexity/accuracy tradeoff 
and justifies a bit of extra work. As it is, the fact that it's copy/pasted 
doesn't help much with maintaining/reading it. We can keep this, though I don't 
feel great about it.

(FWIW, these same copy/pasted numbers thresholds are used for clang's typo 
correction, and we're pretty sure they're not very good - @hokein has tuning 
them on his backlog)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In 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 , @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
+getUnseenKeys(const llvm::SmallSet &SeenKeys) const {
+  llvm::SmallVector 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 311067.
njames93 marked 3 inline comments as done.
njames93 added a comment.

Address some comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/test/config.test
===
--- clang-tools-extra/clangd/test/config.test
+++ 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,
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -167,6 +167,7 @@
 return;
   }
   llvm::SmallSet Seen;
+  llvm::SmallVector, 0> UnknownKeys;
   // We *must* consume all items, even on error, or the parser will assert.
   for (auto &KV : llvm::cast(N)) {
 auto *K = KV.getKey();
@@ -198,9 +199,57 @@
 Warn = UnknownHandler(
 Located(**Key, K->getSourceRange()), *Value);
   if (Warn)
-Outer->warning("Unknown " + Description + " key " + **Key, *K);
+UnknownKeys.push_back(std::move(*Key));
 }
   }
+  if (!UnknownKeys.empty())
+warnUnknownKeys(UnknownKeys, unseenKeys(Seen));
+}
+
+  private:
+llvm

[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 , @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
+getUnseenKeys(const llvm::SmallSet &SeenKeys) const {
+  llvm::SmallVector 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 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 310814.
njames93 added a comment.

Fix lit test failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/test/config.test
===
--- clang-tools-extra/clangd/test/config.test
+++ 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,
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -167,6 +167,7 @@
 return;
   }
   llvm::SmallSet Seen;
+  llvm::SmallVector, 0> UnknownKeys;
   // We *must* consume all items, even on error, or the parser will assert.
   for (auto &KV : llvm::cast(N)) {
 auto *K = KV.getKey();
@@ -198,9 +199,62 @@
 Warn = UnknownHandler(
 Located(**Key, K->getSourceRange()), *Value);
   if (Warn)
-Outer->warning("Unknown " + Description + " key " + **Key, *K);
+UnknownKeys.push_back(std::move(*Key));
 }
   }
+  if (!UnknownKeys.empty())
+warnUnknownKeys(UnknownKeys, getUnseenKeys(Seen));
+}
+
+  private:
+llvm::SmallVector
+getUnseenKeys(const ll

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

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: sammccall, kadircet.
Herald added subscribers: usaxena95, arphaman.
njames93 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92990

Files:
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -113,7 +113,7 @@
 
   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"))),
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -79,6 +79,12 @@
   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 @@
   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 @@
   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 @@
 
   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 @@
   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();
 
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -167,6 +167,7 @@
 return;
   }
   llvm::SmallSet Seen;
+  llvm::SmallVector, 0> UnknownKeys;
   // We *must* consume all items, even on error, or the parser will assert.
   for (auto &KV : llvm::cast(N)) {
 auto *K = KV.getKey();
@@ -198,9 +199,61 @@
 Warn = UnknownHandler(
 Located(**Key, K->getSourceRange()), *Value);
   if (Warn)
-Outer->warning("Unknown " + Description + " key " + **Key, *K);
+UnknownKeys.push_back(std::move(*Key));
 }
   }
+  if (!UnknownKeys.empty())
+warnUnknownKeys(UnknownKeys, getUnseenKeys(Seen));
+}
+
+  private:
+llvm::SmallVector
+getUnseenKeys(const llvm::SmallSet &SeenKeys) const {
+  llvm::SmallVector Result;
+  for (const auto &KeyAndHandler : Keys)
+if (!SeenKeys.count(KeyAndHandler.first.str()))
+  Result.push_