[clang-tools-extra] [clang-tidy] Extend `bugprone-use-after-move` to allow custom invalidation functions (PR #170346)

[via cfe-commits]

martinboehme wrote:

> @martinboehme Could you explain how that is any different from this?
> 
> ```
> #include <string>
> #include <vector>
> 
> bool TryInsert(std::vector<std::string>& v, std::string&& s) {
> 	if (some_condition()) { return false; }
> 	v.push_back(std::move(s));
> 	return true;
> }
> 
> std::string foo(std::vector<std::string>& v, std::string&& s) {
> 	if (!TryInsert(v, std::move(s))) {
> 		return std::move(s);  // warning: 's' used after it was moved [bugprone-use-after-move]
> 	}
> 	return {};
> }
> ```

You mean how is this different from the example I posted earlier? Quoting it again:

```cxx
SomeMoveableType obj;
void foo(SomeMoveableType &&, SomeOtherType);
// Obviously a contrived example, but you get the point
foo(std::move(obj), obj.operation());
```

One difference is that `TryInsert` in your example has "maybe move" semantics (it might move, or it might not, similar to `try_emplace()`). But that's not the decisive difference here.

The key line in my example is this one:

```cxx
foo(std::move(obj), obj.operation());
```

Note how this uses `obj` in two arguments to the same function.

If the check assumed that `std::move(obj)` already leaves `obj` in a moved-from state, then it would need to flag `obj.operation()` as a use-after-move. But the check knows that the move only happens once `foo()` starts executing, so `obj.operation()` is guaranteed to be evaluated before the move, and hence it is not a use-after-move[^1].

Now let's look at a similar example but with a user-defined invalidation function. We'll assume [`close(2)`](https://pubs.opengroup.org/onlinepubs/009604499/functions/close.html) has been marked as an invalidation function.

```cxx
int fd = open(...);
void foo(int err, ssize_t bytes_written);
foo(close(fd), write(fd, ...));
```

Observations:

- This is a synthetic example -- I'm not sure what purpose this would serve. But I assume this kind of pattern could come up in real code with other invalidation functions.
- This is an "use after invalidation". The order in which function arguments are evaluated is unspecified, so we have to assume pessimistically that `close(fd)` (the invalidation) is evaluated before `write(fd, ...)` (the use).
- The check does not currently flag this use after invalidation because it uses the same logic as for `std::move` and hence concludes, erroneously, that the invalidation only happens once `foo()` starts executing.

> It seems to me the crux of your argument...
> 
> > sooner or later someone is going to ~configure an invalidation function~ _compose an outer function_ that does return a value, will use that value in an expression, and be mightily confused about why the check doesn't understand when the invalidation happens.
> 
> ...applies ~verbatim to situations with `std::move` and `std::forward` too, no?

I'm not sure exactly what you mean here (particularly with respect to the "compose an outer function"), but I hope I've made it clear above how `std::move` and `std::forward` are different from user-defined invalidation functions.

> > I realize this is often not an issue in practice
> 
> This is important, and has been our guiding principle here. The reality is this check (and the typical tidy in general) has always had rough edges and been inherently limited in what it can do. 

Definitely. (I'm the original author of bugprone-use-after-move and am painfully aware of its limitations.)

However, we accept these limitations because they are hard to overcome. The issue I point out above is not hard to overcome at all: `std::move` and `std::forward` should continue to determine the `moving-call` using [this matcher](https://github.com/llvm/llvm-project/blob/a95a9465bfd261f7b7a464d79336faa74af780c0/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp#L508), whereas user-defined invalidation functions should simply consider the `moving-call` to be identical to the `call-move`[^2]. (IOW, we should be doing _less_ work for user-defined invalidation functions!)

My wider point is that this illustrates how the semantics of `std::move` and `std::forward` are different from user-defined invalidation functions and that these should therefore be handled by a different check (which would internally reuse almost all of the implementation).

> All that said, though, I'm not really opposing you: if you'd like to make the check more precise, or rename it more accurately, and others are on board -- I have no particular opposition. I've done what I can to improve it so far, and I'll be happy to see contributors improve the check further.

I unfortunately currently can't devote the time to contributing to the check. I realize it's on me that I didn't notice this PR while it was open and voice my concerns at the time. I do believe that the status quo is unsatisfactory, and I hope that I've explained why.

[^1]: C++ doesn't define the order in which function arguments are evaluated; if `obj.operation()` is evaluated before `std::move(obj)`, there's no question of this being a use-after-move of course. But even in the case where we get "unlucky" and the `std::move(obj)` is evaluated first, this isn't a use-after-move.

[^2]: Note also how these nodes are now misleadingly named. They should really be `invalidating-call` and `call-invalidator` or similar.

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