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

[cor3ntin]

Summary

This adds a diagnostic when an unqualified call is resolved
to std::move or std::forward.

This follows some C++ committee discussions where some
people where concerns that this might be an usual anti pattern
particularly britle worth warning about - both because move
is a common name and because these functions accept any values.

This warns inconditionnally of whether the current context is in
std:: or not, as implementations probably want to always qualify
these calls too, to avoid triggering adl accidentally.

[cor3ntin]

There may be some things worth considering here:

• libc++ own tests and build is affected, I have not addressed these yet.
• other standard libs are affected but because they are considered system headers, no warning is emitted

I think this is a worth while first approach. We might want to consider a "warn on adl calls" attribute, or something of that nature, but it would be added to probably a lot of functions, so maybe we want the opposite but that might also be very noisy.
I'm also itching to warn of using namespace at file scope, but maybe that's better left in clang tidy?

Anyway, the current proposed solution should have no false positive or false negative, and we can expand from there as needed if people find the warning useful.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

4812

This should probably be named -Wunqualified-std-move resp. -Wunqualified-std-forward, so that we preserve our ability to add a more generalized -Wunqualified-std-call diagnostic (probably not enabled by default) in the future.

I'd actually like to go ahead and add the generalized diagnostic right now, since it would be about 10 extra lines of code, and at least I would use it as soon as it landed in trunk! But I'm prepared for that to be controversial and wouldn't necessarily pick that hill to die on. :) (Ah, and I'm reminded that the generalized diagnostic would need a bigger list of hard-coded functions to ignore, such as swap and begin and end and so on.)

clang/test/SemaCXX/unqualified-std-call.cpp

34

I'd like to see a test where the template finds move via ADL, rather than being directly in the scope of a using namespace std directive. This is the most important case AIUI, because we can train people not to using namespace but we can't so easily train them not to accidentally forget a std::.
I'd like to see a couple of tests where the (function, resp. template) is itself located inside namespace std. (You mention you deliberately want to warn on this, and I think that's good, but I don't see any tests for it.)
I'd like to see a test where the std::move that is found by lookup is actually std::__1::move with an inline namespace, because that's what libc++ actually does in practice. (This should still warn.)

[cjdb]

Thanks for working on this! I'm overall very happy, but I don't see a test case for a unary move originating from another namespace.

clang/lib/Sema/SemaExpr.cpp

6422

6442

There's a range-based llvm::find in llvm/STLExtras.h.

clang/test/SemaCXX/warn-self-move.cpp

19

Perhaps this should warn if the algorithm std::move isn't seen by the compiler (that isn't a request for this patch).

20–21

Rationale: there's less mental gymnastics when backslashes aren't involved.

30–31

[erichkeane]

I'm not sure this is a 'good' enough warning (by design?) to allow in -Wall either, Wall seems to be only "this is definitely a mistake" type issues, and I don't think this issue rises to that requirement (though others are likely more qualified <get it!> than I to judge).

Implementation generally looks right to me otherwise, with 1 question/comment.

clang/lib/Sema/SemaExpr.cpp

6437

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?

[• Quuxplusone]

Tangential data point: I hacked up this check to find all unqualified std functions and ran it on libc++'s tests and also on some of my own codebase (including re2, asio, protobuf). I'm fixing the issues it turned up in libc++'s tests. In the other codebases, there was a lot of min and max in multiple codebases; several uses of back_inserter and getline across different codebases; scattered probably-accidental use of algorithms in protobuf (sort, stable_sort, binary_search); one file contained using std::setw etc.; and asio makes very heavy use of unqualified declval.
This is irrelevant to Corentin's interests with this PR; I'm just dumping the finding here in case anyone's ever interested in generalizing this check and wants to know what it would find.

clang/lib/Sema/SemaExpr.cpp

6437

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.

[erichkeane]

clang/lib/Sema/SemaExpr.cpp

6437

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.

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

6437

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.

[• Quuxplusone]

clang/lib/Sema/SemaExpr.cpp

6437

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 :))

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

6437

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

[cor3ntin]

• Adds more tests for calls inside std, inline namespaces and adl lookup
• use llvm::find
• rename the warning flag to unqualified-std-cast-call to leave room for future extensions

[cor3ntin]

Fix comment to address Christopher's feedback

[cor3ntin]

clang/test/SemaCXX/warn-self-move.cpp

20–21

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

[erichkeane]

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

clang/lib/Sema/SemaExpr.cpp

6420

6422

6437

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.

6441

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.

clang/test/SemaCXX/warn-self-move.cpp

20–21

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.

[cor3ntin]

Fix comments punctuation.

clang/lib/Sema/SemaExpr.cpp

6441

To be honest, this is something I've considered, and I'll go in that direction if you ask me to.
My thinking is that we do not have the need to do that right now, we can expand on it when we want to, and in the meantime, bailing out early on arity avoid wasting cpu cycles.

[erichkeane]

I understand you still don't have commit-rights. If you'd like, I can push this in a little bit. Just let me know the name/email you'd like me to use.

clang/lib/Sema/SemaExpr.cpp

6441

Right, its the inconsistency between the two (being extra future-proof here, and being much-less future-proof above) that bugs me.

Not enough to make you do anything here though.

[cor3ntin]

In D119670#3343076, @erichkeane wrote:

I understand you still don't have commit-rights. If you'd like, I can push this in a little bit. Just let me know the name/email you'd like me to use.

I guess i should ask for those at some point, I feed bad asking you or Aaron every time...

"Corentin Jabot <[e-mail]>"

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

6441

Oh, i see.. I guess we could not have an array but that seemed cleaner. Or we could find a less generic name for the function, if you want.

[rnk]

This broke our libc++ -Werror build:

llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: unqualified call to std::move [-Werror,-Wunqualified-std-cast-call]
                                                __root_(move(other.__root_)),
                                                        ^
                                                        std::

We can disable the warning, but we look forward to an upstream resolution.

[erichkeane]

In D119670#3343718, @rnk wrote:

This broke our libc++ -Werror build:

llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: unqualified call to std::move [-Werror,-Wunqualified-std-cast-call]
                                                __root_(move(other.__root_)),
                                                        ^
                                                        std::

We can disable the warning, but we look forward to an upstream resolution.

This is exactly the kind of case that this warning is designed to catch. Unqualified calls std::move is what we're trying to catch.

[cor3ntin]

In D119670#3343726, @erichkeane wrote:

In D119670#3343718, @rnk wrote:

This broke our libc++ -Werror build:

llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: unqualified call to std::move [-Werror,-Wunqualified-std-cast-call]
                                                __root_(move(other.__root_)),
                                                        ^
                                                        std::

We can disable the warning, but we look forward to an upstream resolution.

This is exactly the kind of case that this warning is designed to catch. Unqualified calls std::move is what we're trying to catch.

The issue is that for its own tests, libc++ headers are not considered system headers. we should fix libc++. I can do that later today. I though @Quuxplusone did.

[• arthur.j.odwyer]

I thought I caught all of them, too! I encourage someone with commit rights
to land the missing five characters "std::" right now, and/or I'll be able
to take my own look in about 6 hours from now.

Arthur

[cor3ntin]

The issue is that for its own tests, libc++ headers are not considered system headers. we should fix libc++. I can do that later today. I though @Quuxplusone did.

Patch here https://reviews.llvm.org/D120509 - I'm still running the test locally to double check

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_cxx17_compat_adl_only_template_id : Warning<
   "use of function template name with no prior function template "
   "declaration in function call with explicit template arguments "
   "is incompatible with C++ standards before C++20">,
   InGroup<CXXPre20Compat>, DefaultIgnore;
 def ext_adl_only_template_id : ExtWarn<
   "use of function template name with no prior declaration in function call "
   "with explicit template arguments is a C++20 extension">, InGroup<CXX20>;
 
+def warn_unqualified_call_to_std_function : Warning<
+  "unqualified call to %0">, InGroup<DiagGroup<"unqualified-std-call">>;
+
+
+
 // C++ Template Argument Lists
 def err_template_missing_args : Error<
   "use of "
   "%select{class template|function template|variable template|alias template|"
   "template template parameter|concept|template}0 %1 requires template "
   "arguments">;
 def err_template_arg_list_different_arity : Error<
   "%select{too few|too many}0 template arguments for "

clang/lib/Sema/SemaExpr.cpp

if (!EnclosingFunctionCtx->isDependentContext()) {
     // ... since this is an implicit member reference, that might potentially
     // involve a 'this' capture, mark 'this' for potential capture in
     // enclosing lambdas.
     if (CurLSI->ImpCaptureStyle != CurLSI->ImpCap_None)
       CurLSI->addPotentialThisCapture(CallLoc);
   }
 }
 
+// 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) {
+  // We are only checking move and forward so exit early here
+  if (Call->getNumArgs() != 1)
+    return;
+
+  Expr *E = Call->getCallee()->IgnoreParenImpCasts();
+  if (!E || isa<UnresolvedLookupExpr>(E))
+    return;
+  DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(E);
+  if (!DRE || !DRE->getLocation().isValid())
+    return;
+
+  if (DRE->getQualifier())
+    return;
+
+  NamedDecl *D = dyn_cast_or_null<NamedDecl>(Call->getCalleeDecl());
+  if (!D || !D->isInStdNamespace())
+    return;
+
+  // Only warn for some functions deemed more frequent or problematic
+  static constexpr llvm::StringRef SpecialFunctions[] = {"move", "forward"};
+  auto it = std::find(std::begin(SpecialFunctions), std::end(SpecialFunctions),
+                      D->getName());
+  if (it == std::end(SpecialFunctions))
+    return;
+
+  S.Diag(DRE->getLocation(), diag::warn_unqualified_call_to_std_function)
+      << D->getQualifiedNameAsString()
+      << FixItHint::CreateInsertion(DRE->getLocation(), "std::");
+}
+
 ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
                                MultiExprArg ArgExprs, SourceLocation RParenLoc,
                                Expr *ExecConfig) {
   ExprResult Call =
       BuildCallExpr(Scope, Fn, LParenLoc, ArgExprs, RParenLoc, ExecConfig,
                     /*IsExecConfig=*/false, /*AllowRecovery=*/true);
   if (Call.isInvalid())
     return Call;
 
   // Diagnose uses of the C++20 "ADL-only template-id call" feature in earlier
   // language modes.
   if (auto *ULE = dyn_cast<UnresolvedLookupExpr>(Fn)) {
     if (ULE->hasExplicitTemplateArgs() &&
         ULE->decls_begin() == ULE->decls_end()) {
       Diag(Fn->getExprLoc(), getLangOpts().CPlusPlus20
                                  ? diag::warn_cxx17_compat_adl_only_template_id
                                  : diag::ext_adl_only_template_id)
           << ULE->getName();
     }
   }
 
   if (LangOpts.OpenMP)
     Call = ActOnOpenMPCall(Call, Scope, LParenLoc, ArgExprs, RParenLoc,
                            ExecConfig);
-
+  if (LangOpts.CPlusPlus) {
+    CallExpr *CE = dyn_cast<CallExpr>(Call.get());
+    if (CE)
+      DiagnosedUnqualifiedCallsToStdFunctions(*this, CE);
+  }
   return Call;
 }
 
 /// BuildCallExpr - Handle a call to Fn with the specified array of arguments.
 /// This provides the location of the left/right parens and a list of comma
 /// locations.
 ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
                                MultiExprArg ArgExprs, SourceLocation RParenLoc,

clang/test/SemaCXX/unqualified-std-call-fixits.cpp

+// RUN: %clang_cc1 -verify -std=c++20 -Wall %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -std=c++20 -fixit %t
+// RUN: %clang_cc1 -Wall -Werror -x c++ -std=c++20 %t
+// RUN: cat %t | FileCheck %s
+
+namespace std {
+
+void move(auto &&a) {}
+
+void forward(auto &a) {}
+
+} // namespace std
+
+using namespace std;
+
+void f() {
+  int i = 0;
+  move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  std::
+  forward(i); // expected-warning {{unqualified call to std::forward}}
+              // CHECK: {{^}}  std::
+}

clang/test/SemaCXX/unqualified-std-call.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+
+namespace std {
+
+template <typename T>
+void dummy(T &&) {}
+template <typename T>
+void move(T &&) {}
+template <typename T, typename U>
+void move(T &&, U &&) {}
+template <typename T>
+void forward(T &) {}
+
+} // namespace std
+
+namespace global {
+
+using namespace std;
+
+void f() {
+  int i = 0;
+  std::move(i);
+  move(i);   // expected-warning{{unqualified call to std::move}}
+  (move)(i); // expected-warning{{unqualified call to std::move}}
+  std::dummy(1);
+  dummy(1);
+  std::move(1, 2);
+  move(1, 2);
+}
+
+template <typename T>
+void g(T &&foo) {
+  std::move(foo);
+  move(foo); // expected-warning{{unqualified call to std::move}}
+
+  std::forward<decltype(foo)>(foo);
+  forward<decltype(foo)>(foo); // expected-warning{{unqualified call to std::forward}}
+  move(1, 2);
+  dummy(foo);
+}
+
+void call() {
+  g(0); //expected-note {{here}}
+}
+
+} // namespace global
+
+namespace named {
+
+using std::forward;
+using std::move;
+
+void f() {
+  int i = 0;
+  move(i); // expected-warning{{unqualified call to std::move}}
+  move(1, 2);
+}
+
+template <typename T>
+void g(T &&foo) {
+  move(foo);                     // expected-warning{{unqualified call to std::move}}
+  forward<decltype(foo)>(foo);   // expected-warning{{unqualified call to std::forward}}
+  (forward<decltype(foo)>)(foo); // expected-warning{{unqualified call to std::forward}}
+  move(1, 2);
+}
+
+void call() {
+  g(0); //expected-note {{here}}
+}
+
+} // namespace named
+
+namespace overload {
+using namespace std;
+template <typename T>
+int move(T &&);
+void f() {
+  int i = 0;
+  move(i);
+}
+} // namespace overload
+
+namespace test_alias {
+namespace alias = std;
+using namespace alias;
+void f() {
+  int i = 0;
+  move(i); // expected-warning{{unqualified call to std::move}}
+  move(1, 2);
+}
+
+} // namespace test_alias

clang/test/SemaCXX/warn-self-move.cpp

 }
 }
 
 void int_test() {
   int x = 5;
   x = std::move(x);  // expected-warning{{explicitly moving}}
   (x) = std::move(x);  // expected-warning{{explicitly moving}}
 
   using std::move;
-  x = move(x);  // expected-warning{{explicitly moving}}
+  x = move(x); // expected-warning{{explicitly moving}} \
+                   expected-warning {{unqualified call to std::move}}
 }
 
 int global;
 void global_int_test() {
   global = std::move(global);  // expected-warning{{explicitly moving}}
   (global) = std::move(global);  // expected-warning{{explicitly moving}}
 
   using std::move;
-  global = move(global);  // expected-warning{{explicitly moving}}
+  global = move(global); // expected-warning{{explicitly moving}} \
+                             expected-warning {{unqualified call to std::move}}
 }
 
 class field_test {
   int x;
   field_test(field_test&& other) {
     x = std::move(x);  // expected-warning{{explicitly moving}}
     x = std::move(other.x);
     other.x = std::move(x);