[clangd] Show deprecation diagnostics

[kadircet]

Summary

Adds deprecation warnings to diagnostics.

[kadircet]

clangd/Diagnostics.cpp

299 ↗

(On Diff #164263)

Couldn't find a better way to check for this one. Category types are coming from tablegen file https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td#L128 which does not expose category id, at least I couldn't find even if it does.

[kadircet]

• Formatting.

[ioeric]

clangd/Diagnostics.cpp

299 ↗

(On Diff #164263)

Have you tried the Diagnostic ID (i.e. Info.getID())? It matches the diag kinds defined in DiagnosticSemaKinds.inc (e.g. warn_deprecated).

[ilya-biryukov]

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.

What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?

PS In case I'm missing the context here, please let me know.

[kadircet]

In D51747#1227089, @ilya-biryukov wrote:

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.

What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?

PS In case I'm missing the context here, please let me know.

Agree with you, I think it would be better to provide it as an option. https://reviews.llvm.org/D51724 with this one we added a way to show deprecated symbols on code completion responses and wanted to move forward with showing the ones that are already in existing code.

clangd/Diagnostics.cpp

299 ↗

(On Diff #164263)

Unfortunately these are also internal, https://github.com/llvm-mirror/clang/blob/master/lib/Basic/DiagnosticIDs.cpp#L97

[ioeric]

In D51747#1227089, @ilya-biryukov wrote:

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.

What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?

PS In case I'm missing the context here, please let me know.

Instead of

In D51747#1227719, @kadircet wrote:

In D51747#1227089, @ilya-biryukov wrote:

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.

What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?

PS In case I'm missing the context here, please let me know.

Agree with you, I think it would be better to provide it as an option. https://reviews.llvm.org/D51724 with this one we added a way to show deprecated symbols on code completion responses and wanted to move forward with showing the ones that are already in existing code.

I also agree with you regarding options. A pattern I've observed (in some infamous large codebase ;) is that warnings for deprecated symbols are disabled in the compile command as they can introduce too much noise during build, although it would make sense to show these warnings when user edits the code (most of the time). I think there can be other diagnostics that are more desirable as editor diagnostics than as compiler warnings/errors.

[ilya-biryukov]

I also agree with you regarding options. A pattern I've observed (in some infamous large codebase ;) is that warnings for deprecated symbols are disabled in the compile command as they can introduce too much noise during build, although it would make sense to show these warnings when user edits the code (most of the time). I think there can be other diagnostics that are more desirable as editor diagnostics than as compiler warnings/errors.

What kind of option are we talking about? (1) A flag to clangd or (2) a completely different way to expose deprecated methods (i.e. not diagnostics)?
If (1), users already have a way to enable this by adding this flag when producing compile_commands.json. Not sure if adding a flag to clangd for tampering with one special-cased clang arg carries its weight, however produces compile_commands.json can set the flags they care about.
(2) does seem useful and D51724 is a good example of it. As we have discussed recently, we could expose this as a form of semantic syntax highlighting (not available in clangd or LSP currently).

[sammccall]

In D51747#1227921, @ioeric wrote:

In D51747#1227089, @ilya-biryukov wrote:

! In D51747#1227719, @kadircet wrote:

! In D51747#1227089, @ilya-biryukov wrote:

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.
   
   What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?
   
   PS In case I'm missing the context here, please let me know.

Agree with you, I think it would be better to provide it as an option. https://reviews.llvm.org/D51724 with this one we added a way to show deprecated symbols on code completion responses and wanted to move forward with showing the ones that are already in existing code.

I also agree with you regarding options. A pattern I've observed (in some infamous large codebase ;) is that warnings for deprecated symbols are disabled in the compile command as they can introduce too much noise during build, although it would make sense to show these warnings when user edits the code (most of the time). I think there can be other diagnostics that are more desirable as editor diagnostics than as compiler warnings/errors.

So I'll be (somewhat) the naysayer regarding options.
We still have to choose a default, and almost everyone will use it, including those who don't like it. So we still need the best possible default, and shouldn't let talk of options distract from this.
Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here.

But if we add an option, we need to decide what the *other* option should be too, and this isn't obvious (should it be "suppress deprecation warnings" or "use compilation database as-is"?

My vote for default behavior would be: add -Wdeprecated, and if deprecation diagnostics are errors, downgrade them to warnings. (Or better add -Wdeprecated -Wno-error=deprecated).
But I'm biased by mostly using -Wno-deprecated -Werror codebases, if I didn't I'd probably prefer to just respect the compile-commands.

(I'm not a fan of the downgrade-to-note behavior: notes do not carry the connotation of "bad", and deprecation warnings can have notes attached themselves which is confusing).

[sammccall]

clangd/ClangdServer.cpp

536

can you add a comment that these flags work with both gcc and clang-cl driver modes?
It seems to be true. This is an important invariant (I should add a test for it) that isn't obvious (there's no test today because we didn't know about clang-cl at the time)

538

as noted above I think we should also have -Wno-error=deprecated-declarations

(do you want all of -Wdeprecated, actually?)

clangd/Diagnostics.cpp

299 ↗

(On Diff #164268)

not sure what the concrete benefits are from using Note rather than Warning. It's semantically iffy, so if we do this it should have a comment justifying it.

[ilya-biryukov]

Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here.

Any thoughts on tampering with provided compile args in the first place? Is it fine for clangd to be opinionated about the warnings we choose to always show to the users?
E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't think clangd should force them on everyone.

[sammccall]

In D51747#1228919, @ilya-biryukov wrote:

Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here.

Any thoughts on tampering with provided compile args in the first place? Is it fine for clangd to be opinionated about the warnings we choose to always show to the users?
E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't think clangd should force them on everyone.

A few thoughts here:

• does CDB describe user or project preferences? unclear.
• "show this warning for code I build" is a higher bar than "show this warning for code I edit". So CDB probably enables too few warnings.
• Some warnings play well with -Werror (like uninit warnings), some don't (like deprecated). -Werror projects often disable interesting warnings.

I'm not sure we should strictly follow the CDB, but the bar to override it should probably be high.
Maybe the (default) behavior here should be "add -Wdeprecated -Wno-error=deprecated if -Werror is set"?

[kadircet]

In D51747#1229066, @sammccall wrote:

In D51747#1228919, @ilya-biryukov wrote:

Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here.

Any thoughts on tampering with provided compile args in the first place? Is it fine for clangd to be opinionated about the warnings we choose to always show to the users?
E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't think clangd should force them on everyone.

A few thoughts here:

• does CDB describe user or project preferences? unclear.
• "show this warning for code I build" is a higher bar than "show this warning for code I edit". So CDB probably enables too few warnings.
• Some warnings play well with -Werror (like uninit warnings), some don't (like deprecated). -Werror projects often disable interesting warnings.

I'm not sure we should strictly follow the CDB, but the bar to override it should probably be high.
Maybe the (default) behavior here should be "add -Wdeprecated -Wno-error=deprecated if -Werror is set"?

I agree with checking for -Werror and then providing -Wno-error as well, if -Wdeprecated was not added already.
Then keeping it as warnings shouldn't be too much of a problem except the case you mentioned, if user wants to see all diagnostics as a list suddenly they will get deprecations in that list as well :(.

[sammccall]

In D51747#1230420, @kadircet wrote:

In D51747#1229066, @sammccall wrote:

In D51747#1228919, @ilya-biryukov wrote:

Most of the value of adding an option is: if someone complains, we can tell them to go away :-) One possible corollary is: we shouldn't add the option until someone complains. I'm not 100% sure about that, though - not totally opposed to an option here.

Any thoughts on tampering with provided compile args in the first place? Is it fine for clangd to be opinionated about the warnings we choose to always show to the users?
E.g. I'm a big fan of various uninitialized warnings, but nevertheless don't think clangd should force them on everyone.

A few thoughts here:

• does CDB describe user or project preferences? unclear.
• "show this warning for code I build" is a higher bar than "show this warning for code I edit". So CDB probably enables too few warnings.
• Some warnings play well with -Werror (like uninit warnings), some don't (like deprecated). -Werror projects often disable interesting warnings.

I'm not sure we should strictly follow the CDB, but the bar to override it should probably be high.
Maybe the (default) behavior here should be "add -Wdeprecated -Wno-error=deprecated if -Werror is set"?

I agree with checking for -Werror and then providing -Wno-error as well, if -Wdeprecated was not added already.

I'm nervous enough about trying to ad-hoc parse the flags that I'd be tempted just to ad -Wno-error=deprecated without checking to see if -Werror is set - it's a no-op if not. But up to you.

Ilya's suggestion of just respecting the compilation database also seems OK. It seems a bit sad for the users who won't see deprecation warnings because their project doesn't want to show them at build time. I'd lean towards adding this to see if anyone complains, but happy with whatever you two can agree on.

Then keeping it as warnings shouldn't be too much of a problem except the case you mentioned, if user wants to see all diagnostics as a list suddenly they will get deprecations in that list as well :(.

I don't think that making them notes will avoid this though :-/

[ilya-biryukov]

In D51747#1229066, @sammccall wrote:

A few thoughts here:

• does CDB describe user or project preferences? unclear.

Agree, it's a mix, defaults are from the project but users can add extra flags.

• "show this warning for code I build" is a higher bar than "show this warning for code I edit". So CDB probably enables too few warnings.
• Some warnings play well with -Werror (like uninit warnings), some don't (like deprecated). -Werror projects often disable interesting warnings.

Agreed, editors are different from build.

I'm not sure we should strictly follow the CDB, but the bar to override it should probably be high.

WDYT in the long term about a more general mechanism (to allow users override compiler or warning flags at the clangd level?
So that even if clangd is opinionated about the default warnings it enables, users have an option to override according to their preferences.

In D51747#1230420, @kadircet wrote:

if user wants to see all diagnostics as a list suddenly they will get deprecations in that list as well :(.

Yeah, some level of noise is probably inevitable.

[kadircet]

• Resolve discussions.

[kadircet]

As discussed offline with Ilya, decided to keep the compile flag addition since it would be easier to pass the logic of a command line flag to that point. Also not changing the severity level, since they will show up on diagnostics lists in anyway it doesn't save much.

clangd/ClangdServer.cpp

538

Yes for the second part and no for the first part. As we saw there are some configs out there that treat some types of deprecation warnings as errors, so we don't want to change that behavior.

clangd/Diagnostics.cpp

299 ↗

(On Diff #164268)

Agree on that one, decided on not implementing such a thing until it becomes necessary.

[sammccall]

clangd/ClangdServer.cpp

538

Doesn't this cause problems when the build flags are -Wno-deprecated -Werror then?
We make this -Wno-deprecated -Werror -Wdeprecated, and now all uses of deprecated APIs are errors.

This seems like a common configuration, highly noticeable symptoms, and not what the user wants...

[kadircet]

• Do not show up as errors even on codebases with -Werror.

[kadircet]

clangd/ClangdServer.cpp

538

Well, adding -Wno-error=deprecated to get over this case. Now we might have problems if people wanted to see deprecation warnings as errors, but I believe this shouldn't be much of a problem since they will get those errors at built anytime and we won't be annoying other users(ones that show deprecation as warnings or not showing them at all, hopefully almost everyone?) too much.

WDYT?

[sammccall]

clangd/ClangdServer.cpp

535

nit: the "these flags" is not just for resource dir, can you move that second sentence onto a line above?

538

each of these lines deserves a comment I think.
e.g.
// Deprecations are often hidden for full-project build. They're useful in context.
-Wdeprecated
// Adding -Wdeprecated would trigger errors in projects what set -Werror.
-Wno-error=deprecated

unittests/clangd/ClangdUnitTests.cpp

223 ↗

(On Diff #165266)

I think you should set TestTU.ExtraArgs to "-Wno-deprecated" (with a comment) to verify it's overridden.
Both for clarity, and because I think -Wdeprecated is actually on by default, so I think this test as written would pass without your change.

248 ↗

(On Diff #165266)

I'm not sure exactly what this is testing - I think it's a combination of features that are tested elsewhere. Feel free to drop.

[kadircet]

• Convert unit tests to lit tests and address comments.

[kadircet]

When doing some manual testing saw that it causes a lot of slow down on vim, even when I have just about 200 diagnostics, will wait till a few others check that out. We might wanna limit the number of diagnostics and keep only the ones that are closest to current cursor position.

[sammccall]

Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests?

We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!)

They're mostly used in places where we need to test specifically the protocol (smoke testing features, startup/shutdown, protocol edge cases...)

[kadircet]

In D51747#1233371, @sammccall wrote:

Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests?

We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!)

They're mostly used in places where we need to test specifically the protocol (smoke testing features, startup/shutdown, protocol edge cases...)

Sorry my bad actually forgot to mention. We set additional flags in ClangdServer::getCompileCommand and it is not used on the path of TestTU actually, instead it creates its own command line flags and uses them to invoke the compiler. So I changed to lit tests to make sure it gets invoked through getCompileCommand

[sammccall]

In D51747#1233377, @kadircet wrote:

In D51747#1233371, @sammccall wrote:

Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests?

We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!)

They're mostly used in places where we need to test specifically the protocol (smoke testing features, startup/shutdown, protocol edge cases...)

Sorry my bad actually forgot to mention. We set additional flags in ClangdServer::getCompileCommand and it is not used on the path of TestTU actually, instead it creates its own command line flags and uses them to invoke the compiler. So I changed to lit tests to make sure it gets invoked through getCompileCommand

Ah, that's unfortunate :-(
Sorry to be a pain, but can these be in ClangdTests instead? (Should really be called ClangdServerTests). It's not as nice as TestTU, but...

[kadircet]

• Turn back to unit tests.

[kadircet]

In D51747#1233401, @sammccall wrote:

In D51747#1233377, @kadircet wrote:

In D51747#1233371, @sammccall wrote:

Sorry for all the back and forth on this patch, but I have to ask... what's up with switching to lit tests?

We've mostly started avoiding these as they're hard to maintain and debug (not to mention write... crazy sed tricks!)

They're mostly used in places where we need to test specifically the protocol (smoke testing features, startup/shutdown, protocol edge cases...)

Sorry my bad actually forgot to mention. We set additional flags in ClangdServer::getCompileCommand and it is not used on the path of TestTU actually, instead it creates its own command line flags and uses them to invoke the compiler. So I changed to lit tests to make sure it gets invoked through getCompileCommand

Ah, that's unfortunate :-(
Sorry to be a pain, but can these be in ClangdTests instead? (Should really be called ClangdServerTests). It's not as nice as TestTU, but...

Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway.

[ilya-biryukov]

In D51747#1233499, @kadircet wrote:

Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway.

Just wondering: what's the problem with Vim? Slowness? Bad UI for diagnostics when there are many of those?

[sammccall]

Awesome, thank you.
Just a couple of last nits.

unittests/clangd/ClangdTests.cpp

989

Locking is unneccesary. (Diagnostics will be delivered once, and the SyncAPI calls block until diagnostics are delivered)

1019

nit: the logic is right, but the message could be better.
If we change something and it fails it will print Expected true: DiagConsumer.worksAsExpected(), but was false or so. There are a number of things that could be wrong.
Prefer just to capture a bit more data (all diagnostics) and use a matcher expression:

MATCHER(DeprecationWarning, "") {
  return arg.Category == "Deprecations" && arg.Severity == DiagnosticsEngine::Warning;
}
...
EXPECT_THAT(Diags, ElementsAre(DeprecationWarning()));

That way if there isn't exactly one diagnostic that's a deprecation warning, it'll print the expectation, the full list of diagnostics, and the reason it didn't match.

[kadircet]

• Add matchers to test.

[kadircet]

In D51747#1233530, @ilya-biryukov wrote:

In D51747#1233499, @kadircet wrote:

Np, not planning to land it before we figure out the issue with vim can't handling lots of diagnostics anyway.

Just wondering: what's the problem with Vim? Slowness? Bad UI for diagnostics when there are many of those?

No, it starts to lag after some amount of diagnostics. I can't even move between lines in the file without a huge latency, and it doesn't return to normal after some time.

clangd/ClangdServer.cpp

 }
 
 tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) {
   trace::Span Span("GetCompileCommand");
   llvm::Optional<tooling::CompileCommand> C = CDB.getCompileCommand(File);
   if (!C) // FIXME: Suppress diagnostics? Let the user know?
     C = CDB.getFallbackCommand(File);
 
+  // These flags are working for both gcc and clang-cl driver modes.
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
   C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+  // Deprecations are often hidden for full-project build. They're useful in
+  // context.
+  C->CommandLine.push_back("-Wdeprecated");
+  // Adding -Wdeprecated would trigger errors in projects what set -Werror.
+  C->CommandLine.push_back("-Wno-error=deprecated");
   return std::move(*C);
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 

unittests/clangd/ClangdTests.cpp

 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
 namespace {
 
+MATCHER(DeprecationWarning, "") {
+  return arg.Category == "Deprecations" &&
+         arg.Severity == DiagnosticsEngine::Warning;
+}
+
 bool diagsContainErrors(const std::vector<Diag> &Diagnostics) {
   for (auto D : Diagnostics) {
     if (D.Severity == DiagnosticsEngine::Error ||
         D.Severity == DiagnosticsEngine::Fatal)
       return true;
   }
   return false;
 }
TEST_F(ClangdVFSTest, ChangedHeaderFromISystem) {
   Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
                                          clangd::CodeCompleteOptions()))
                     .Completions;
   // We want to make sure we see the updated version.
   EXPECT_THAT(Completions, ElementsAre(Field(&CodeCompletion::Name, "bar"),
                                        Field(&CodeCompletion::Name, "baz")));
 }
 
+TEST(ClangdCompilecommand, DiagnosticDeprecated) {
+  std::string Code(R"cpp(
+    void foo() __attribute__((deprecated));
+    void bar() {
+      foo();
+    }
+  )cpp");
+  auto SourcePath = testPath("source/foo.cpp");
+
+  MockFSProvider FS;
+  struct DiagConsumer : public DiagnosticsConsumer {
+    void onDiagnosticsReady(PathRef File,
+                            std::vector<Diag> Diagnostics) override {
+      Diags.insert(Diags.end(), Diagnostics.begin(), Diagnostics.end());
+    }
+
+    void reset() {
+      Diags.clear();
+    }
+
+    std::vector<Diag> Diags;
+  } DiagConsumer;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags.push_back("-Wno-deprecated");
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  runAddDocument(Server, SourcePath, Code);
+  EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+  DiagConsumer.reset();
+
+  CDB.ExtraClangFlags.push_back("-Werror");
+  runAddDocument(Server, SourcePath, Code);
+  EXPECT_THAT(DiagConsumer.Diags, ElementsAre(DeprecationWarning()));
+}
+
+
 } // namespace
 } // namespace clangd
 } // namespace clang