[clang-tidy] Fix bugprone-use-after-move when move is in noexcept operator

[njames93]

Summary

Fixes noexcept operator misinterpreted as being evaluated.

[• Quuxplusone]

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

408

What about sizeof, alignof, requires, typeid, and other such unevaluated contexts? Shouldn't there be a common way to spell "this expression is unevaluated"? (I don't know if there is or not.)

[merge_guards_bot]

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[gribozavr2]

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

408

+1 to adding handling and tests for other unevaluated contexts.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

1284

Is the macro a necessary part of this test? If not, can it be removed?

[njames93]

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

408

I think i can write a matcher that will detect that

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

1284

I like to stick to the bug report as much as possible

[gribozavr2]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

1284

This is not generally the policy in LLVM and Clang. We prefer minimal reproducers.

[njames93]

• added more unevaluated context checks

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

1284

I've kept it minimal, the REQUIRE definition is just to suppress other warnings about unused expressions

[njames93]

• Elide braces

[merge_guards_bot]

Unit tests: pass. 62207 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62206 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

• Fix formatting

[merge_guards_bot]

Unit tests: pass. 62207 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[gribozavr2]

LGTM with fixes to the test.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

1284

Adding (void) should do the trick.

1287

I'm not sure why foo and otherUnenvaluated are separate functions. If you want to separate them, I think it would make more sense to put each operator into a separate function then.

[njames93]

• Fix nits in test cases

[merge_guards_bot]

Unit tests: pass. 62213 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

• Remove artifacts of old dependent review

[merge_guards_bot]

Unit tests: pass. 62249 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

 //===--- UseAfterMoveCheck.cpp - clang-tidy -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseAfterMoveCheck.h"
 
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprConcepts.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Lex/Lexer.h"
 
 #include "../utils/ExprSequence.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::utils;
 
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 namespace {
 
+AST_MATCHER(Expr, hasUnevaluatedContext) {
+  if (isa<CXXNoexceptExpr>(Node) || isa<RequiresExpr>(Node))
+    return true;
+  if (const auto *UnaryExpr = dyn_cast<UnaryExprOrTypeTraitExpr>(&Node)) {
+    switch (UnaryExpr->getKind()) {
+    case UETT_SizeOf:
+    case UETT_AlignOf:
+      return true;
+    default:
+      return false;
+    }
+  }
+  if (const auto *TypeIDExpr = dyn_cast<CXXTypeidExpr>(&Node))
+    return !TypeIDExpr->isPotentiallyEvaluated();
+  return false;
+}
+
 /// Contains information about a use-after-move.
 struct UseAfterMove {
   // The DeclRefExpr that constituted the use of the object.
   const DeclRefExpr *DeclRef;
 
   // Is the order in which the move and the use are evaluated undefined?
   bool EvaluationOrderUndefined;
 };

 //   seeing if they are children of a TypeLoc), or
 // - Part of a function template argument (we check this by seeing if they are
 //   children of a DeclRefExpr that references a function template).
 // DeclRefExprs that fulfill these conditions should not be counted as a use or
 // move.
 static StatementMatcher inDecltypeOrTemplateArg() {
   return anyOf(hasAncestor(typeLoc()),
                hasAncestor(declRefExpr(
-                   to(functionDecl(ast_matchers::isTemplateInstantiation())))));
+                   to(functionDecl(ast_matchers::isTemplateInstantiation())))),
+               hasAncestor(expr(hasUnevaluatedContext())));
 }
 
 UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
     : Context(TheContext) {}
 
 bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
                               const ValueDecl *MovedVariable,
                               UseAfterMove *TheUseAfterMove) {
if (!getLangOpts().CPlusPlus11)
     return;
 
   auto CallMoveMatcher =
       callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
                hasArgument(0, declRefExpr().bind("arg")),
                anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
                      hasAncestor(functionDecl().bind("containing-func"))),
                unless(inDecltypeOrTemplateArg()))
           .bind("call-move");
 
   Finder->addMatcher(
       // To find the Stmt that we assume performs the actual move, we look for
       // the direct ancestor of the std::move() that isn't one of the node
       // types ignored by ignoringParenImpCasts().
       stmt(forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
            // Don't allow an InitListExpr to be the moving call. An InitListExpr
            // has both a syntactic and a semantic form, and the parent-child

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

 template <typename T>
 class C : T, B {
   void m_fn1() {
     int a;
     std::move(a);
     C c;
   }
 };
+} // namespace PR33020
+
+namespace UnevalContext {
+struct Foo {};
+void noExcept() {
+  Foo Bar;
+  (void) noexcept(Foo{std::move(Bar)});
+  Foo Other{std::move(Bar)};
+}
+void sizeOf() {
+  Foo Bar;
+  (void)sizeof(Foo{std::move(Bar)});
+  Foo Other{std::move(Bar)};
+}
+void alignOf() {
+  Foo Bar;
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wgnu-alignof-expression"
+  (void)alignof(Foo{std::move(Bar)});
+#pragma clang diagnostic pop
+  Foo Other{std::move(Bar)};
+}
+void typeId() {
+  Foo Bar;
+  // error: you need to include <typeinfo> before using the 'typeid' operator
+  // (void) typeid(Foo{std::move(Bar)}).name();
+  Foo Other{std::move(Bar)};
 }
+} // namespace UnevalContext