Re: [PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-07 Thread Sam McCall via cfe-commits 
Yep, thanks. This uncovered an existing error, so rather than reverting the
whole thing I've disabled the assert on windows while I fix
it. f1357264b8e3070bef5bb4ff35ececa4d6c76108

On Mon, Dec 7, 2020 at 12:22 PM Nico Weber via Phabricator <
revi...@reviews.llvm.org> wrote:

> thakis added a comment.
>
> Looks like this breaks tests on windows:
> http://45.33.8.238/win/29315/step_9.txt
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D92704/new/
>
> https://reviews.llvm.org/D92704
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-07 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Looks like this breaks tests on windows: http://45.33.8.238/win/29315/step_9.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92704

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

[PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-07 Thread Sam McCall via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.
Closed by commit rGfed9af29c2b5: [clangd] Publish config file errors over LSP 
(authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D92704?vs=309688=309841#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92704

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ConfigProvider.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigTesting.h
  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
@@ -67,6 +67,7 @@
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- clang-tools-extra/clangd/unittests/ConfigTesting.h
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -24,6 +24,12 @@
 struct CapturedDiags {
   std::function callback() {
 return [this](const llvm::SMDiagnostic ) {
+  if (Files.empty() || Files.back() != D.getFilename())
+Files.push_back(D.getFilename().str());
+
+  if (D.getKind() > llvm::SourceMgr::DK_Warning)
+return;
+
   Diagnostics.emplace_back();
   Diag  = Diagnostics.back();
   Out.Message = D.getMessage().str();
@@ -50,7 +56,13 @@
   << D.Message << "@" << llvm::to_string(D.Pos);
 }
   };
-  std::vector Diagnostics;
+  std::vector Diagnostics;  // Warning or higher.
+  std::vector Files; // Filename from diagnostics including notes.
+
+  void clear() {
+Diagnostics.clear();
+Files.clear();
+  }
 };
 
 MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -96,21 +96,26 @@
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files["foo.yaml"] = AddBarBaz;
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+  Diags.clear();
 
   FS.Files.erase("foo.yaml");
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 }
 
@@ -133,21 +138,26 @@
 
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files,
+  ElementsAre(testPath("a/foo.yaml"), testPath("a/b/c/foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(AParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files.erase("a/foo.yaml");
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files,

[PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-07 Thread Haojian Wu via Phabricator via cfe-commits 
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

The pre-merging bot seems to be failed on windows: 
https://reviews.llvm.org/harbormaster/unit/view/215879/.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:833
+  llvm::StringMap> ReportableDiagnostics;
+  auto DiagnosticHandler = [&](const llvm::SMDiagnostic ) {
+// Ensure we create the map entry even for note diagnostics we don't 
report.

I'd encode `Config` into names, `DiagnosticHandler` made me think this is a 
handler for clang diagnostics (without reading the context).



Comment at: clang-tools-extra/clangd/ClangdServer.h:366
   const ThreadsafeFS 
+  Callbacks *ServerCallbacks;
+  mutable std::mutex ConfigDiagnosticsMu;

nit: = nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92704

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

[PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall updated this revision to Diff 309688.
sammccall added a comment.

Handle diagnostics without a filename


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92704

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ConfigProvider.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigTesting.h
  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
@@ -67,6 +67,7 @@
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- clang-tools-extra/clangd/unittests/ConfigTesting.h
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -24,6 +24,12 @@
 struct CapturedDiags {
   std::function callback() {
 return [this](const llvm::SMDiagnostic ) {
+  if (Files.empty() || Files.back() != D.getFilename())
+Files.push_back(D.getFilename().str());
+
+  if (D.getKind() > llvm::SourceMgr::DK_Warning)
+return;
+
   Diagnostics.emplace_back();
   Diag  = Diagnostics.back();
   Out.Message = D.getMessage().str();
@@ -50,7 +56,13 @@
   << D.Message << "@" << llvm::to_string(D.Pos);
 }
   };
-  std::vector Diagnostics;
+  std::vector Diagnostics;  // Warning or higher.
+  std::vector Files; // Filename from diagnostics including notes.
+
+  void clear() {
+Diagnostics.clear();
+Files.clear();
+  }
 };
 
 MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -96,21 +96,26 @@
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files["foo.yaml"] = AddBarBaz;
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+  Diags.clear();
 
   FS.Files.erase("foo.yaml");
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 }
 
@@ -133,21 +138,26 @@
 
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files,
+  ElementsAre(testPath("a/foo.yaml"), testPath("a/b/c/foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(AParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files.erase("a/foo.yaml");
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
 }
 
@@ -169,7 +179,7 @@
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
   EXPECT_THAT(getAddedArgs(Cfg),

[PATCH] D92704: [clangd] Publish config file errors over LSP

2020-12-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

We don't act as a language server for these files (e.g. don't get open/close
notifications for them), but just blindly publish diagnostics for them.

This works reasonably well in coc.nvim and vscode: they show up in the
workspace diagnostic list and when you open the file.
The only update after the file is reparsed, not as you type which is a bit
janky, but seems a lot better than nothing.

Fixes https://github.com/clangd/clangd/issues/614


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92704

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ConfigProvider.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/test/config.test
  clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
  clang-tools-extra/clangd/unittests/ConfigTesting.h
  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
@@ -67,6 +67,7 @@
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- clang-tools-extra/clangd/unittests/ConfigTesting.h
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -24,6 +24,12 @@
 struct CapturedDiags {
   std::function callback() {
 return [this](const llvm::SMDiagnostic ) {
+  if (Files.empty() || Files.back() != D.getFilename())
+Files.push_back(D.getFilename().str());
+
+  if (D.getKind() > llvm::SourceMgr::DK_Warning)
+return;
+
   Diagnostics.emplace_back();
   Diag  = Diagnostics.back();
   Out.Message = D.getMessage().str();
@@ -50,7 +56,13 @@
   << D.Message << "@" << llvm::to_string(D.Pos);
 }
   };
-  std::vector Diagnostics;
+  std::vector Diagnostics;  // Warning or higher.
+  std::vector Files; // Filename from diagnostics including notes.
+
+  void clear() {
+Diagnostics.clear();
+Files.clear();
+  }
 };
 
 MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -96,21 +96,26 @@
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files["foo.yaml"] = AddBarBaz;
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+  EXPECT_THAT(Diags.Files, ElementsAre(testPath("foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+  Diags.clear();
 
   FS.Files.erase("foo.yaml");
   Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 }
 
@@ -133,21 +138,26 @@
 
   auto Cfg = P->getConfig(Params(), Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Files, IsEmpty());
   EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
 
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
   ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(Diags.Files,
+  ElementsAre(testPath("a/foo.yaml"), testPath("a/b/c/foo.yaml")));
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
-  Diags.Diagnostics.clear();
+  Diags.clear();
 
   Cfg = P->getConfig(AParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+