| Issue |
184034
|
| Summary |
[clang-tidy] Add `suspicious-nolint` check
|
| Labels |
clang-tidy
|
| Assignees |
|
| Reporter |
unterumarmung
|
Following the discussion here: https://discourse.llvm.org/t/rfc-add-non-blocking-precommit-clang-tidy-analysis, one of the concerns about enabling clang-tidy in pre-commit was that `NOLINT` comments “get out of sync with reality too easily”. This check aims to solve that problem. Even if the RFC above is not accepted, such a check should still be useful to a wider community.
There was an older patch that tried to do something similar: https://reviews.llvm.org/D41326, but it was never accepted. This new check would probably fit best into the `bugprone` or `misc` module.
## What the check should do
### 1. Forbid non-targeted `NOLINT` comments
It should be possible to forbid “catch-all” `NOLINT`s such as:
```cpp
// NOLINT
// NOLINT(*)
```
and the same for `NOLINTNEXTLINE` and `NOLINTBEGIN`/`NOLINTEND`.
The idea is to force targeted suppressions and avoid silently hiding new diagnostics that appear on already `NOLINT`ed lines.
**Fix-it idea:**
If there are diagnostics on that line, the check should suggest adding their check names into the `NOLINT` list. If there are several diagnostics, it should suggest a list of names.
#### 1.1. Optionally forbid wildcard patterns
With a separate option, it should be possible to reject patterns like:
```cpp
// NOLINT(google-*)
```
While this is more targeted than `NOLINT(*)`, it can still hide many issues at once.
### 2. Require an explaining comment
The check should be able to enforce that every `NOLINT` has an explanation, for example:
```cpp
// NOLINT: reason
// NOLINT(xxx): reason
```
Once `NOLINT` is parsed, there should be some non-whitespace text after it.
Open question to the community:
Should the separator format be enforced (e.g. `// NOLINT(xxx): reason`), configurable, or is “any non-whitespace text after the `NOLINT`” good enough?
This is a purely stylistic rule, but some teams want it so they don’t have to ask “why is this `NOLINT` here?” every time.
### 3. Check that `NOLINT` actually suppresses something
If a `NOLINT` does not match any diagnostic on the line, it should be reported.
Rules for matching:
- Non-targeted `NOLINT` (no checks listed):
It should match any diagnostic on that line.
- Patterned `NOLINT` (e.g. `NOLINT(google-*)`):
It should match at least one diagnostic whose check name fits the pattern.
- `NOLINT` with a list of checks/patterns:
Each element in the list should be checked separately.
**Fix-it idea:**
- If none of the listed checks/patterns match anything, suggest removing the whole `NOLINT` comment.
- If only some elements are unused, suggest removing just those elements from the list.
### 4. Warn on unknown check names
If a `NOLINT` refers to a check name that clang-tidy does not know about, the check should warn.
This is useful when updating clang-tidy: a check may have been removed or renamed, but the old name may still linger in the code.
---
Because `NOLINT` comments are also used by other tools (for example `cpplint`), and because of the concerns raised in the older review, it should be possible to enable/disable parts of this check ((1), (3), (4)) based on patterns, and for both checks known and unknown to clang-tidy.
![]()
_______________________________________________
llvm-bugs mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
