[clang-tools-extra] [clang-tidy] Change AllowCastToVoid default to true in bugprone-unused-return-value (PR #200173)

[Daniil Dudkin via cfe-commits]

Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/200173 at github.com>


unterumarmung wrote:

> I don't think that we can say that it is definitely *wrong* without any knowledge about the context.

For some functions I think we can. I cannot come up with a case where discarding `std::unique_ptr::release()` is not harmful, or where discarding `std::vector::empty()` is not useless and misleading. Can you?

And I think this is the key point here: `(void)` only answers "was this discard intentional?". This check asks a different question: "should this return value be discarded at all?"

For many functions in the default `CheckedFunctions`, an intentional discard is exactly the suspicious behavior we want to diagnose. Writing `(void)` does not make the operation less suspicious. It only tells us that the programmer did it on purpose.

> The default behavior of Clang Tidy *must* respect the judgement of the users, because they are professionals who know *much more* about their own code than our shallow AST-based automated checks.

I don't agree with this premise. The purpose of static analysis and linting is precisely to question the programmer's judgement when the code looks suspicious. Humans write bugs, including intentional ones. If the tool always stopped once the programmer expressed intent, a lot of useful diagnostics would disappear.

The user still has the final say. They decide which checks to enable, how to configure them, and which diagnostics to suppress. One should not blindly enable all clang-tidy checks and expect every default to fit every codebase.

> The checkers may have a paranoid analysis mode that spams the user with "I see you wanted this, but did you *really* want it?", but it is a terrible experience, so it must not be the default one.

I don't see this as a paranoid mode. For this check, "yes, I intended to discard it" does not answer the question the check is asking.

If the user considers that behavior too noisy for their codebase, they can set `AllowCastToVoid=true`. That is exactly what the option is for.

> But `// NOLINT(bugprone-unused-return-value)` has a *HUGE* drawback that it is only understood by clang-tidy, while `(void)` is the well-established standard notation for "I'm intentionally discarding this value" which is widely used in the industry and also recognized by many other code analysis tools.

I agree that `(void)` is the standard way to say "I intentionally discard this value". But that is orthogonal to this diagnostic.

If both facts matter, both can be written:

```cpp
(void)foo(); // NOLINT(bugprone-unused-return-value)
```

`(void)` says the discard is intentional. `NOLINT` says this particular suspicious discard was reviewed and accepted despite this check.

For user-configured functions where discarding the result is sometimes perfectly reasonable, `AllowCastToVoid=true` makes sense. But for the default functions, I would even question whether `(void)` suppression should be allowed at all, perhaps with a few exceptions. Otherwise it can completely defeat the purpose of the check.

> Also if somebody thinks that `(void)` is easy to overlook, they can easily search for it in the repository and review each location where it appears...

They can, but that requires a separate proactive audit that somebody has to decide to do.

A `NOLINT` attached to the suspicious call is visible during normal code review. `(void)` is also used for unrelated legitimate reasons, for example suppressing unused-variable warnings from structured bindings, so suspicious discarded calls can blend in with ordinary casts.

That is exactly why I think a check-specific suppression is more useful here: it makes the exceptional case explicit at the point where it matters.

https://github.com/llvm/llvm-project/pull/200173