This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Show deprecation diagnostics
AbandonedPublic

Authored by kadircet on Sep 6 2018, 12:04 PM.

Details

Summary

Adds deprecation warnings to diagnostics.

Event Timeline

kadircet created this revision.Sep 6 2018, 12:04 PM
kadircet added inline comments.Sep 6 2018, 12:08 PM
clangd/Diagnostics.cpp
299

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 updated this revision to Diff 164268.Sep 6 2018, 12:25 PM
  • Formatting.
ioeric added inline comments.Sep 7 2018, 1:51 AM
clangd/Diagnostics.cpp
299

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 added a comment.EditedSep 7 2018, 3:37 AM

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.

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
ioeric added a comment.Sep 7 2018, 2:17 PM

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

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.

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).

! 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 added inline comments.Sep 10 2018, 7:02 AM
clangd/ClangdServer.cpp
535

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)

537

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

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

clangd/Diagnostics.cpp
299

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.

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.

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"?

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 :(.

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 :-/

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.

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 updated this revision to Diff 165252.Sep 13 2018, 4:53 AM
kadircet marked 5 inline comments as done.
  • Resolve discussions.

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
537

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

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

sammccall added inline comments.Sep 13 2018, 5:18 AM
clangd/ClangdServer.cpp
537

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 updated this revision to Diff 165266.Sep 13 2018, 5:59 AM
kadircet marked an inline comment as done.
  • Do not show up as errors even on codebases with -Werror.
kadircet added inline comments.Sep 13 2018, 5:59 AM
clangd/ClangdServer.cpp
537

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?

kadircet retitled this revision from [clangd] Implement deprecation diagnostics with lower severity. to [clangd] Show deprecation diagnostics.Sep 13 2018, 6:02 AM
kadircet edited the summary of this revision. (Show Details)
sammccall accepted this revision.Sep 13 2018, 6:06 AM
sammccall added inline comments.
clangd/ClangdServer.cpp
534

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

537

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
230

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.

255

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.

This revision is now accepted and ready to land.Sep 13 2018, 6:06 AM
kadircet updated this revision to Diff 165275.Sep 13 2018, 6:57 AM
kadircet marked 2 inline comments as done.
  • Convert unit tests to lit tests and address comments.
kadircet planned changes to this revision.Sep 13 2018, 6:57 AM
kadircet marked 2 inline comments as done.

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.

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 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

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 updated this revision to Diff 165302.Sep 13 2018, 8:20 AM
  • Turn back to unit tests.
This revision is now accepted and ready to land.Sep 13 2018, 8:20 AM

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.

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 accepted this revision.Sep 13 2018, 12:42 PM

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

unittests/clangd/ClangdTests.cpp
984 ↗(On Diff #165302)

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

1014 ↗(On Diff #165302)

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 updated this revision to Diff 165462.Sep 14 2018, 3:25 AM
kadircet marked 2 inline comments as done.
  • Add matchers to test.

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.

kadircet abandoned this revision.Oct 26 2018, 2:50 AM