[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

See the suggestions, our comments are supposed to be full sentences terminated with a full-stop. Otherwise nothing actionable.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:6420
+// Once a call is fully resolved, warn for unqualified calls to specific
+// C++ standard functions, like move and forward
+static void DiagnosedUnqualifiedCallsToStdFunctions(Sema &S, CallExpr *Call) {
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:6422
+static void DiagnosedUnqualifiedCallsToStdFunctions(Sema &S, CallExpr *Call) {
+  // We are only checking unary move and forward so exit early here
+  if (Call->getNumArgs() != 1)
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:6441
+  // Only warn for some functions deemed more frequent or problematic
+  static constexpr llvm::StringRef SpecialFunctions[] = {"move", "forward"};
+  auto it = llvm::find(SpecialFunctions, D->getName());
----------------
I find myself a touch grumpy that this is used to keep this expandable to other std functions, but still sticks itself to unary functions (particularly when the function name `DiagnosedUnqualifiedCallsToStdFunctions` isn't unary-specific).

I'm not sure how actionable this comment is, but I felt the need to share.  I guess I could suggest moving this to the top and making this hold a 'pair' of args + name (plus presumably a "don't check arg count" for some sort of variadic function check).

But, 'eh', think about it perhaps?  Maybe this comment can just exist to help out the next person who wants to add something to this function.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6437
+  NamedDecl *D = dyn_cast_or_null<NamedDecl>(Call->getCalleeDecl());
+  if (!D || !D->isInStdNamespace())
+    return;
----------------
cor3ntin wrote:
> Quuxplusone wrote:
> > cor3ntin wrote:
> > > erichkeane wrote:
> > > > Quuxplusone wrote:
> > > > > erichkeane wrote:
> > > > > > Do we really want this?  I guess I would think doing:
> > > > > > 
> > > > > > 
> > > > > > ```
> > > > > > void MyFunc(auto whatever) {
> > > > > >   auto X = move(whatever);
> > > > > > ```
> > > > > > 
> > > > > > when I MEAN std::move, just for it to pick up a non-std::move the 1st time is likely the same problem?  Or should it get a separate warning?
> > > > > That's a good point (IMHO). Perhaps instead of making this a specific case of "warn for unqualified call to things in `std` (namely `move` and `forward`)," we should make it a specific case of "warn for any unqualified use of //this identifier// (namely `move` and `forward`)." That's closer in spirit to Nico Josuttis's comment that `move` is almost like a keyword in modern C++, and therefore shouldn't be thrown around willy-nilly. Either you mean `std::move` (in which case qualify it), or you mean some other `my::move` (in which case qualify it), but using the bare word `move` is inappropriate in modern C++ no matter whether it //currently// finds something out of `std` or not.
> > > > > I'm ambivalent between these two ways of looking at the issue. Maybe someone can think up a reason to prefer one or the other?
> > > > > 
> > > > > libc++'s tests do include several recently-added instances of `move` as a //variable name//, e.g. `auto copy(x); auto move(std::move(x));`. This confuses grep but would not confuse Clang, for better and worse. I don't expect that real code would ever do this, either.
> > > > > 
> > > > > @erichkeane's specific example is a //template//, which means it's going to be picked up by D72282 `clang-tidy bugprone-unintended-adl` also. Using ADL inside templates triggers multiple red flags simultaneously. Whereas this D119670 is the only thing that's going to catch unqualified `move` in //non-template// code.
> > > > > That's a good point (IMHO). Perhaps instead of making this a specific case of "warn for unqualified call to things in `std` (namely `move` and `forward`)," we should make it a specific case of "warn for any unqualified use of //this identifier// (namely `move` and `forward`)." That's closer in spirit to Nico Josuttis's comment that `move` is almost like a keyword in modern C++, and therefore shouldn't be thrown around willy-nilly. Either you mean `std::move` (in which case qualify it), or you mean some other `my::move` (in which case qualify it), but using the bare word `move` is inappropriate in modern C++ no matter whether it //currently// finds something out of `std` or not.
> > > > 
> > > > Ah! I guess that was just my interpretation of how this patch worked: Point out troublesome 'keyword-like-library-functions' used unqualified.  I think the alternative (warn for unqualified call to things in std) is so incredibly noisy as to be worthless, particularly in light of 'using' statements.
> > > > 
> > > > > @erichkeane's specific example is a //template//, which means it's going to be picked up by D72282 `clang-tidy bugprone-unintended-adl` also. Using ADL inside templates triggers multiple red flags simultaneously. Whereas this D119670 is the only thing that's going to catch unqualified `move` in //non-template// code.
> > > > 
> > > > This was a template for convenience sake (so y'all couldn't "well actually" me on the type I chose), but good to know we have a warning like that!
> > > > 
> > > > What I was TRYING to point out a case where the person is using `move` or `forward` intending to have the `std` version, but accidentially ending up with thier own version.
> > > It is *likely* the same problem. The problem is the "likely" does a lot of heavy lifting here.
> > > I think there is general agreement that std::move should be called qualified, not that `move` is somehow a special identifier that users should not use. These are almost contradictory statements - aka if we wanted to discourage people to name their function move, we wouldn't also need or want to force them to qualify their call.
> > > 
> > > It is very possible that `std::move` cannot be used at all in the TU because it is simply not declared, in which case the code would be perfectly fine.
> > > A slightly more involved approach would be to try to detect whether std::move is declared by doing a lookup and warn in that case, but I'm not sure that brings much.
> > > I certainly disagree that calling a function `move` is cause for warning.
> > > I certainly disagree that calling a function `move` is cause for warning.
> > 
> > Right, agreed. For example, `boost::move` exists, and that's fine. (I wonder if any codebase contains the line `using boost::move;`!)
> > 
> > > These are almost contradictory statements - aka if we wanted to discourage people to name their function move, we wouldn't also need or want to force them to qualify their call.
> > 
> > Well, the original raison d'etre for D119670 was the observation that `using namespace std::views; auto y = move(x);` will change behavior in C++23, if C++23 adds `std::views::move` as a view adaptor. There's nothing in that story that involves a //user-defined// function named `move`; the reason we want to warn people off of unqualified `move` (or unqualified //anything//-in-std) is for SD-8-adjacent reasons: we want the library to be free to add new entities (such as `std::views::move` specifically) without breaking old code. (Note: the above code involves a second red flag — `using namespace` — so we don't expect it to be //common//.)
> > 
> > > It is very possible that `std::move` cannot be used at all in the TU because it is simply not declared, in which case the code would be perfectly fine.
> > 
> > Strong disagree. Maybe `std::move` isn't visible //today//, but then tomorrow you `#include <utility>` or some random header that transitively includes the relevant piece of `<utility>`, and boom, //now// your `auto y = move(x)` does something different. Unqualified single-arg `move` is a warning-worthy time bomb no matter what, IMO. ...Which I guess means I'm coming around to favor Erich's suggestion (at least so far this morning :))
> The raison d'etre for D119670 is that we don't want people to make unqualified calls to `std::move`. 
> 
> That `views::move` would otherwise conflict, is certainly one of the reason for that, and SD-8 supports the theory that we should also warn on using namespace and on standard functions that are found by ADL that should not be (which i decided not to do here because I don't know that we have a good grip on that).
> 
> I certainly do not want to warn on calls to arbitrary functions that happen to share a name with a standard function, and `move` is no exception.
> 
>  * We _know_ that an unqualified call to `std::move` is something we can warn about as there is a strong recommendation that these calls should be qualified (from WG21)
>  * There _might_ be an issue with unqualified call to an arbitrary function which share a name with the standard library function (which may be `move` or some other name)
> 
> These are 2 very different categories of warnings, and one is a lot less subjective than the other. In the first scenario the user is doing something bad, in the other case they *may* be - but their code may be perfectly valid.
> 
> Now, I'm not saying the later warning would not be useful, but, it is certainly not something we want to be on by default, nor in `-Wall`.
> 
> The PR as it is has no false positive, and I think anything that could have false positives should either be a clang-tidy check or a not-on-by-default warning
> 
>  
> 
> 
> 
>I certainly do not want to warn on calls to arbitrary functions that happen to share a name with a standard function, and move is no exception.

Haven't been paying attention to this in a while, but yeah, you're right.  I've come around.  There _IS_ a warning that would have some use (where we treat unqualified 'move' as a potential mistake), but I don't see it as being frontend-worthy.  Perhaps it has clang-tidy potential.


================
Comment at: clang/test/SemaCXX/warn-self-move.cpp:20-21
   using std::move;
-  x = move(x);  // expected-warning{{explicitly moving}}
+  x = move(x); // expected-warning{{explicitly moving}} \
+                   expected-warning {{unqualified call to std::move}}
 }
----------------
cor3ntin wrote:
> cjdb wrote:
> > Rationale: there's less mental gymnastics when backslashes aren't involved.
> I think this is unusual so I'd like @aaron.ballman 's opinion - I think to recall he asked me to make the exact opposite change
Spoke to Aaron, he prefers it as-is.  _I_ prefer the other way like @cjdb, but not enough to have a discussion on it.  Either way is fine for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119670