[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D108560#3012755 <https://reviews.llvm.org/D108560#3012755>, @aaron.ballman wrote:

> Sorry for not thinking of this sooner, but there is another diagnostic we might want to consider.
>
>   // NOLINTBEGIN(check)
>   // NOLINTEND(other-check)
>
> where the file does not contain a `NOLINTEND(check)` comment anywhere. In this case, the markers are not actually matched, so it's a `NOLINTBEGIN(check)` comment with no end and a `NOLINTEND(other-check)` comment with no begin. That seems like user confusion we'd also want to diagnose, but it also could be tricky because you really want to add diagnostic arguments for the check name.

See new test for this scenario in `test\clang-tidy\infrastructure\nolintbeginend-mismatched-check-names.cpp`.
Diagnostics are generated for both the `NOLINTBEGIN(check)` with no end and the `NOLINTEND(other-check)` with no begin.

> My concern here is mostly with catching typos where the user types `check` in the first and `chekc` in the second

See new test in `test\clang-tidy\infrastructure\nolintbeginend-typo-in-check-name.cpp`.

> Relatedly, I think this construct is perhaps fine:
>
>   // NOLINTBEGIN(check)
>   // NOLINTEND(*)
>
> because the end "covers" the begin.

That was my initial feeling too. However, after doing a bit more thinking, I feel that this construct causes more problems than it's worth. Consider the following example:

  // NOLINTBEGIN(check-A)
  <code that violates check-A>
  // NOLINTEND(*)

This seems OK, but consider what happens when we add a 4th line:

  // NOLINTBEGIN(check-A)
  <code that violates check-A>
  // NOLINTEND(*)
  <code that violates check-B>

...this generates a `NOLINTEND without a previous NOLINTBEGIN` diagnostic on line 3. From `check-B`'s perspective, it has been `END`ed on line 3 but there was no `BEGIN(check-B)` or `BEGIN(*)` on lines 1-2.
Which `NOLINT(BEGIN/END)` comments are acceptable on a given line depends on which check you're evaluating at a given moment.

This problem goes away if we don't allow the check-specific `NOLINT`s and the "all checks" `NOLINT`s to be mixed and matched:

Example 1:

  // NOLINTBEGIN(check-A)
  <code that violates check-A>
  // NOLINTEND(check-A)
  <code that violates check-B>

Example 2:

  // NOLINTBEGIN
  <code that violates check-A>
  // NOLINTEND
  <code that violates check-B>

Let's look at an example where auto-generated code with its own `NOLINT(BEGIN/END)`s is embedded in another file:

  // NOLINTBEGIN(check-A)
  <code that violates check-A>
  <code that violates check-A>
  
  /*
   *
   Place-holder for auto-generated code
   *
   */
  
  <code that violates check-A>
  // NOLINTEND(check-A)
  
  // ...
  // ...

If the auto-generated code is as follows:

  // NOLINTBEGIN(check-B)
  <code that violates check-B>
  <code that violates check-B>
  // NOLINTEND

Then the complete file is:

  // NOLINTBEGIN(check-A)
  <code that violates check-A>
  <code that violates check-A>
  
  // NOLINTBEGIN(check-B)
  <code that violates check-B>
  <code that violates check-B>
  // NOLINTEND
  
  <code that violates check-A>
  // NOLINTEND(check-A)
  
  // ...
  // ...

The `NOLINTEND` in the autogenerated code ends `check-B` as well as the parent file's `check-A`, against the user's intention.
Clang-Tidy can't offer any helpful advice without mind-reading.

> I'm a bit less clear on this though:
>
>   // NOLINTBEGIN(*)
>   // NOLINTEND(check)
>
> because the begin is not fully covered by the end. However, I'm a bit less clear on what should or should not be diagnosed here, so if we wanted to leave that for follow-up work, that'd be fine.

The same rule as above, //don't mix and match check-specific `NOLINT`s with "all checks" `NOLINT`s//, should apply here.

Also, in your example, you have begun all checks (`check`, `check2`, `check3` ... `checkN`) but only ended one of them. The remaining checks are still awaiting a `NOLINTEND` comment of some sort.

I say all this in the interest of keeping the Clang-Tidy code simple, and reducing the amount of weird behavior that is possible (due to mistakes, lack of knowledge, or "creative" use of the feature). I rather this feature be strict and limited in scope, rather than flexible enough to be used in unforeseen error-prone ways.

Perhaps this feature can be extended in the future (if need be) after it gets some use and user feedback comes in?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560