Clang-tidy suppression comments and the WarningsAsErrors setting are ignored

[Quuxplusone]

Bugzilla Link	PR41218
Status	REOPENED
Importance	P enhancement
Reported by	Nathan Ridge (zeratul976@hotmail.com)
Reported on	2019-03-24 19:16:31 -0700
Last modified on	2020-08-31 01:36:04 -0700
Version	unspecified
Hardware	PC Linux
CC	hokein@google.com, ibiryukov@google.com, kadircetinkaya.06.tr@gmail.com, lizan.j@gmail.com, llvm-bugs@lists.llvm.org, sammccall@google.com, svenpanne@gmail.com
Fixed by commit(s)
Attachments	stderr.txt (36655 bytes, text/plain) clangd-stderr.txt (14731 bytes, text/plain) lsp-log-clangd.txt (9627 bytes, text/plain) test.lsp (447 bytes, text/plain) out.txt (4704 bytes, text/plain) out2.txt (2776 bytes, text/plain) test.lsp (457 bytes, text/plain) out.txt (4786 bytes, text/plain)
Blocks
Blocked by
See also

When using clangd's clang-tidy integration, some customization mechanisms that
work with standalone clang-tidy, do not work.

This includes:

  * Suppressing specific diagnostics using suppression
    comments as documented here [1].

  * Using the WarningsAsErrors option in the .clang-tidy
    config file to upgrade some checks from warnings to
    errors.

The reason these don't work is that they are handled in
ClangTidyDiagnosticConsumer, but the clangd integration does not use this
component.

[1] https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

[Quuxplusone]

Haojian, Sam, maybe we could extract this code from ClangTidyDiagnosticConsumer or use ClangTidyDiagnosticConsumer in our code.

Other options?

[Quuxplusone]

We don't aim to fully support all clang-tidy configurations in clangd (some
clang-tidy options are not sensible for clangd), currently we only support the
options (Checks, CheckOptions) that control which checks are enabled.

We may support some widely-used clang-tidy configs in clangd (NOLINT is a
reasonable one). But for WarningsAsErrors, this option is rarely used IMO, I
don't think it is worth to support it.

> maybe we could extract this code from ClangTidyDiagnosticConsumer or use
ClangTidyDiagnosticConsumer in our code.

We could reuse some part of ClangTidyDiagnosticConsumer code, but
ClangTidyDiagnosticConsumer is coupled with clang-tidy, it has clang-tidy-
specific things which are not sensible in clangd.

[Quuxplusone]

I posted a patch for supporting suppression comments:

https://reviews.llvm.org/D60953

[Quuxplusone]

And a patch for suppporting warnings-as-errors:

https://reviews.llvm.org/D61841

[Quuxplusone]

The patches have merged, so this is now fixed.

[Quuxplusone]

I still see NOLINT marked lint error appears as problem on VSCode with clangd 9 and 10, is the patch merged? It doesn't seem 9.0.0 or 10.0.0 contains the patch.

[Quuxplusone]

Same problem here with clangd 10: It still ignores NOLINT comments in the source code.

I think this issue should be re-opened, as it is clearly not fixed.

[Quuxplusone]

I'm not sure about the policy when/how to re-open bugs, but I simply try to do it... :-) If there is another way to do it, a hint/link would be helpful.

[Quuxplusone]

Please include a testcase.

These work for me at trunk (with "Checks: bugprone-lambda-function-name" in
clang tidy)

  auto x = []{
    return __FUNCTION__; // warning emitted
  };

  auto x = []{
    return __FUNCTION__; // NOLINT
  };

  auto x = []{
    // NOLINTNEXTLINE
    return __FUNCTION__;
  };

Note that NOLINT is only for clang-tidy, not warnings in general.

[Quuxplusone]

These work for me at trunk (with "Checks: bugprone-lambda-function-name" in clang tidy)

Rather, in .clang-tidy

[Quuxplusone]

Sure, here is a tiny example:

void foo(char **bar) {
    free(bar);  // NOLINT
}

I get "Do not manage memory manually; use RAII [cppcoreguidelines-no-malloc]"
within Spacemacs and clangd-10 as the LSP backend.

[Quuxplusone]

I don't see the warning with clangd 10.0.0 on that code (I do when I delete //
NOLINT).

Can you paste a full verbose log?
(Add -log=verbose to clangd flags and provide the stderr log stream, which most
plugins expose somehow)

[Quuxplusone]

I don't have the slightest idea how to do that with Spacemacs. The relevant
snippet of my .spacemacs configuration:

...
dotspacemacs-configuration-layers
   '(
     ...
     (c-c++ :variables
            c-c++-backend 'lsp-clangd
            c-c++-default-mode-for-headers 'c++-mode
            lsp-clients-clangd-executable "clangd-10"
            company-clang-executable "clang-10"
            clang-format-executable "clang-format-10")
     ...
    )

So this is basically the cookie-cutter configuration to use clangd in c++-mode,
nothing surprising here. To reproduce:

    * git clone -b develop https://github.com/syl20bnr/spacemacs ~/.emacs.d
    * Fire up Emacs
    * Edit .spacemacs corresponding to the above snippet
    * Restart Emacs
    * See the bug in a project with cppcoreguidelines-no-malloc enabled.

I fully understand that this is not really a "minimal" way to see the bug, but
it's doable in a few minutes. Is there a way to test this without an editor?
How do *you* test this?

[Quuxplusone]

I think you can pass extra args to clangd in spacemacs via "c-c++-lsp-args"

[Quuxplusone]

Attached stderr.txt (36655 bytes, text/plain): stderr log from clangd 10.0.0 working as expected

[Quuxplusone]

Sorry, I did spend some time trying to set up spacemacs, but the instructions
provided don't work for me, I get errors such as:

  Symbol's value as variable is void: helm-buffer-list-reorder-fn
  Error running timer lsp--on-idle (error "Keyword argument :end-line not one of (...)")

So I tested with plain emacs instead.

  (require 'lsp-mode)
  (add-hook 'c-mode-hook #'lsp)
  (setq lsp-log-io t)
  (setq lsp-clients-clangd-args '("-log=verbose" "-pretty"))
  (setq lsp-clients-clangd-executable "/path/to/clangd_10.0.0/bin/clangd")

The example you gave worked for me in clangd 10.0.0 (warning emitted, goes away
when NOLINT is present). The log is attached (from the "*clangd::stderr*"
buffer).

> Is there a way to test this without an editor

No, but we have plans for this.

> How do *you* test this?

I tend to use vim + coc because it's what I'm familiar with, but any editor is
fine as long as you can work out how to get logs out of it. VSCode is a nice
neutral choice as it has good LSP support and is pretty easy for newcomers (in
a way that vim and emacs aren't).

[Quuxplusone]

OK, I've figure out the right .spacemacs configuration, see the markers below:

   ...
   dotspacemacs-configuration-layers
   '(
     ...
     (c-c++ :variables
            c-c++-backend 'lsp-clangd
            c-c++-default-mode-for-headers 'c++-mode
            lsp-clients-clangd-executable "clangd-10"
            lsp-log-io t                                         ;; <========
            lsp-clients-clangd-args '("-log=verbose" "-pretty")  ;; <========
            company-clang-executable "clang-10"
            clang-format-executable "clang-format-10")
     ...
    )

I will attach the contents of the buffers *clangd::stderr* and *lsp-log:
cland:....*.

[Quuxplusone]

Attached clangd-stderr.txt (14731 bytes, text/plain): ``` clangd::stderr buffer

[Quuxplusone]

Attached lsp-log-clangd.txt (9627 bytes, text/plain): ``` lsp-log: clangd:... buffer

[Quuxplusone]

I'm attaching the output of

   /usr/bin/clangd-10 --input-style=delimited -sync -clang-tidy-checks=cppcoreguidelines-no-malloc -pretty < test.lsp > out2.txt

If you compare our output with mine you see that you are missing the "-clang-
tidy-checks=cppcoreguidelines-no-malloc" option and have "-log=verbose"
instead. If I use that, I get no diagnostics, either.

[Quuxplusone]

Attached out2.txt (2776 bytes, text/plain): Output of minimized example

[Quuxplusone]

Attached test.lsp (457 bytes, text/plain): Minimized LSP example (with NOLINT)

[Quuxplusone]

Attached test.lsp (447 bytes, text/plain): Minimized LSP example

[Quuxplusone]

Attached out.txt (4704 bytes, text/plain): Full stderr (with -log=verbose) for minimized LSP example

[Quuxplusone]

Attached out.txt (4786 bytes, text/plain): Full stderr log (with -log=verbose and -clang-tidy-checks=...) for minimized LSP example

[Quuxplusone]

Sorry, I screwed up two things when uploading (I'd tried a few settings and
uploaded the wrong version).
With the // NOLINT comment present and the -clang-tidy-checks flag, I see no
diagnostics.

Not to be overly paranoid, but does the binary I'm using match what you have?
$ md5sum usr/bin/clangd-10
0ea0f07b9cf465e65245b70821a2318d  usr/bin/clangd-10

[Quuxplusone]

To be sure, I removed all clang-related stuff from my box and did a fresh
install of clang-{10,11,12} packages from https://apt.llvm.org/. Currently I
have:

---------------------------------------------------------------------
2fd475650a4b2ace2f45fe5821f34af4  /usr/bin/clangd-10
Ubuntu clangd version 10.0.1-++20200708122807+ef32c611aa2-
1~exp1~20200707223407.61
07dc544bdf1922f2b39723d95fd90655  /usr/bin/clangd-11
Ubuntu clangd version 11.0.0-++20200829062559+2c6a593b5e1-
1~exp1~20200829163219.75
d7682f007bc166c8042ed618b7750a3c  /usr/bin/clangd-12
Ubuntu clangd version 12.0.0-++20200830052612+716e35a0cf5-
1~exp1~20200830153301.149
---------------------------------------------------------------------

Running

----------------------------------------------------------------------
/usr/bin/clangd-10 --input-style=delimited -sync -clang-tidy-
checks=cppcoreguidelines-no-malloc -log=verbose -pretty < test.lsp
---------------------------------------------------------------------

yields no diagnostics (as it should be), running with a modified test.lsp
(without "// NOLINT") yields "Do not manage memory manually; use RAII" (again
as it should be). clangd-11 and clangd-12 behave the same.

Nevertheless, using clangd-10 via the LSP backend in Spacemacs ignores NOLINT.
:-/ I think I have to cut down the example, it is highly unclear to me where
the problem is: clangd or the LSP code in Spacemacs.

BTW: Using clangd-11 and clangd-12 via the LSP backend doesn't work at all, I
gets tons of errors like "'foobar' must resolve to a function declared within
the '__llvm_libc' namespace (lsp)" and "System include blah not allowed (fix
available) (lsp)".