[clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[AMS21]

Summary

Previously a struct like this:

template <typename>
struct A { A(A&&) = default; };

Would trigger a false positive, since even though it is not marked as
noexcept it still is due to the = default.
Now we only give a warning if the defaulted move constructor is
actually declared as throwing and correctly resolve it if they are
defaulted.

This fixes llvm#56026, llvm#41414, llvm#38081

[PiotrZSL]

Current solution is invalid, defaulted move constructors can throw by default.
There are 2 more issues for this case:
https://github.com/llvm/llvm-project/issues/38081
https://github.com/llvm/llvm-project/issues/41414

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

40

that's not true...

#include <memory>

struct Field
{
    Field() = default;
    Field(Field&&) {

    }
};

struct Test
{
    Test(Test&&) = default;
    Field field;
};

static_assert(std::is_nothrow_move_constructible<Test>::value);

Is default, not noexcept...

Try something like this:

bool cannotThrow(const FunctionDecl *Func) {
  const auto *FunProto = Func->getType()->getAs<FunctionProtoType>();
  if (!FunProto)
    return false;

  switch (FunProto->canThrow()) {
  case CT_Cannot:
    return true;
  case CT_Dependent: {
    const Expr *NoexceptExpr = FunProto->getNoexceptExpr();
    bool Result;
    return NoexceptExpr && !NoexceptExpr->isValueDependent() &&
           NoexceptExpr->EvaluateAsBooleanCondition(
               Result, Func->getASTContext(), true) &&
           Result;
  }
  default:
    return false;
  };
}

// Source: https://reviews.llvm.org/D145865

so you could do something like this:

if (cannotThrow(Decl))
    return;

I don't see reason to limit this check only to default'ed. This could be improved, to simply validate also things like `noexcept(std::is_nothrow_move_constructible<TemplateType>::value)`

[PiotrZSL]

Update ReleaseNotes for this check, and validate documentation.

[Eugene.Zelenko]

clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp

64

Please add.

[AMS21]

@PiotrZSL thanks for the quick response.

I've tried to work on the Field test case but seem to have hit a roadblock. From what I can see the FunctionProtoType exception specificaion is EST_Unevaluated which causes it to give up because of the check on Line 41.
I'm not very familiar with the codebase but looked around and found the ResolveExceptionSpec() function inside Sema.h which seems to do exactly what I need: tell me if the implicit definition is throwing or not.
But I can't quite seem to figure out how to get a Sema object or construct one to use said function.
Or maybe there is another more clever way to go about it which I'm not aware of?

Any pointers into the right direction would be kindly appreciated.

[PiotrZSL]

Don't waste time of Sema, no way to access it, as far as I know, it's already destroyed when checks are executed.

[AMS21]

Implemented suggested fixes from reviews

[PiotrZSL]

Try to move some code to separate file, some utilty/...
Add tests with tyepdefs.
Add tests with class derive from type that it gets from template.

Probably this code will be used in few more checks in future.
try implement some virtual base validation,
add some cache so we could avoid validating same class/method again.

Maybe change this into separate class.
Anyway good work..

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

21

Constrcutor -> Constructor

21–22

use isMoveConstructor() here, instead of using Ctor->isMoveConstructor() later.

27

consider moving this all from this file to some utils/something, and don't restrict this only to move operator/constructor but also support destructor/other constructors and so on.
Simply if is not default, ten use evaluateFunctionEST, if is default, then try analyse as you doing now.

31

i think that you may still need to use isUnresolvedExceptionSpec here, and return Unknown if that is true

61

getType()->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();

81

but we shoudn't, we should test now fields of that record.... because it may not have move constructor... (in theory). Probably we should also check if it has constructor, but is's implicit one, then we should also check fields and bases of this class instead of trying to check constructor.

95

isInvalidDecl looks redudant.

98–100

this looks redudant..

[PiotrZSL]

Try refactoring code a lite bit, check suggestions, add tests for typedefs, and inheritance from template type.

[AMS21]

Move functionality to a seperate file
Also the noexcept move constrcutor check now only runs with exceptions enabled like other checks.

[AMS21]

In D146922#4227832, @PiotrZSL wrote:

Try refactoring code a lite bit, check suggestions, add tests for typedefs, and inheritance from template type.

I'm not quite sure what you mean by tests for typedefs. Maybe you could give me an example?

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp

84

Should be const auto *.

88

Ditto.

[PiotrZSL]

In D146922#4231317, @AMS21 wrote:

In D146922#4227832, @PiotrZSL wrote:

Try refactoring code a lite bit, check suggestions, add tests for typedefs, and inheritance from template type.

I'm not quite sure what you mean by tests for typedefs. Maybe you could give me an example?

struct SomeStruct { ... };
using TSomeStruct = SomeStruct;

struct OK {
  OK(OK &&) = default;
  OK &operator=(OK&&) = default;

  TSomeStruct member;
};

struct OK : TSomeStruct {
  OK(OK &&) = default;
  OK &operator=(OK&&) = default;
};

Anyway, looks promising... I still need to check this more deeply.

clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp

288

for those 2 templates do test with and without instantiation

// like this:
void testTemplates() {
  OK21<SomeStruct> value(OK21<SomeStruct>());
  value = OK21<SomeStruct>();
}

,
add test with template as member.
like:

template <typename T>
struct OK22 
{
  OK22(OK22 &&) = default;
  OK22 &operator=(OK22 &&) = default;

  T member;
};

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp

299

add test with 2 base classes (no templates).

[AMS21]

Sprinkle some more const

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h

80

std::unordered_map, or try to check if llvm doesn't have something better with some pre-allocation.

[AMS21]

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

21

Well thats embarassing

[AMS21]

Use llvm::DenseMap instead of std::map
Add some more test cases

[PiotrZSL]

Looks ok, would be good to run this on some code base.

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

22

use isMoveAssignmentOperator instead of hasOverloadedOperatorName

23

move those 2 to begining...

30

invert this if to reduce nesting.

const auto *Decl = Result.Nodes.getNodeAs<CXXMethodDecl>("decl")
if (!Decl)
    return;

32–36

IsConstructor = CXXConstructorDecl::classof(Decl);

39

remove {}, leave just

if (SpecAnalyzer.analyze(Decl) !=     utils::ExceptionSpecAnalyzer::State::Throwing) return;

45–49

woudn't getExceptionSpecInfo() != EST_NoexceptFalse do a trick here ?

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp

32

use FunctionCache.find to avoid double search in line 32 and 38 (and reuse result)

36

try_emplace ?

44

pass FunctionProtoType also to this function

50–52

technically you could change this class to operator on CXXMethodDecl since begining

76

check if this is trivial type, if its trivial type then cannot throw...
FDecl->getType()->isTrivialType(FDecl->getAstContext());

95

Can we hit some endless recursion here ?
Maybe so protection against checking Record that we currently checking.

108–114

I'm not sure if we need to be so picky...
In short we could check all bases.
Virtual, Abstract or not...

155

pass FunctionProtoType also to this function, to avoid double checking

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h

23

No need for std::int8_t, and if you really want then use std::uint8_t

78

this also can be static

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

260

Please keep alphabetical order (by check name) in this section.

[AMS21]

Implement review suggestions

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

45–49

I've tested it and it seem that a struct like this:

struct B {
  static constexpr bool kFalse = false;
  B(B &&) noexcept(kFalse);
};

Also has the the ExceptionSpecType set to EST_NoexceptFalse. So changing this would change the previous behavior.

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp

95

Yes we can hit an infinite recursion here.
Take this class:

struct A {
    A(A &&) = default;
};

Since the move constructor is defaulted, we need to call analyzeUnresolvedOrDefaulted. There we determine the kind of defaulted member function which in this case is a move constructor.
Then we call analyzeRecord. Without setting SkipMethods::Yes we would try to check the move constructor of the class which in this case is A. We would call analyze on the move constructor of A which is exactly where we started and we'd have an infinite loop.

That is why I've added the SkipMethods parameter. While analyzing the defaulted move constructor of A we only want to look at its base classes and it's fields and determine if they are declared as throwing or not. While for their the base classes and fields of A we also want to check their move constructor (if they have any).

I hope my explanation was at least a bit helpful.

There might be a better solution to this, Before this I had essentially the same code twice for checking the bases and field and wanted to combine them.

95

Another way to defiantly have an infinite recursion is if to resolve a struct A; we needed to resolve struct B; and to resolve that we needed to resolve struct A. You would need something like A inheriting from B and B inheriting from A or having the other type as a member variable. Same goes for a struct which contains itself.

But I'm currently unaware on how to create such a scenario with legal C++.

108–114

Honestly I not sure about it. I just tried to copy what Sema does when resolving noexcept.
Removing it definitely makes the code easier to read.

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h

23

I prefer to not use it, but saw ExceptionAnalyzer use it. So I just copied it from there.

[PiotrZSL]

Overall looks good to me. Leave it open for few days, maybe someone else want to comment.

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

45–49

And thats why I pointing this, we should treat this function in same way as it would have noexcept(false)

but functions that use templates like:
noexcept(T::value) should not fall into this

clang-tools-extra/docs/ReleaseNotes.rst

261–262

Maybe: Fixed an issue in the performance-noexcept-move-constructor checker that was causing false-positives when the move constructor or move assign operator were defaulted.

[AMS21]

clang-tools-extra/docs/ReleaseNotes.rst

260

Sure but I'm a bit confused. I assuming the check name is without the prefix so in this case noexcept-move-constructor. So it should be after readability-avoid-underscore-in-googletest-name. But why then is use-after-move sorted before identifier-naming.
I'm probably missing something very obvious.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

260

Full check name (i.e. performance-noexcept-move-constructor) should be accounted.

[AMS21]

Fixed docs and cleanup tests a bit

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

45–49

Ah okay. But I'm not so sure about this change. It would effectively make us report on less cases where we actually know that the move constructor is declared as throwing.

It might be that the user made some logic mistake in their noexcept expression which leads to it always evaluating to false and I think we should still report in that case.

If you don't care about the performance implication of a throwing move constructor or have other reasons to want them, you can always simply disable this check.

[PiotrZSL]

Any plans in delivering this ?

[AMS21]

I'm fine with pushing this change, but since I don't have commit access someone would need to commit this for me

[PiotrZSL]

I'm pushing this as it looks fine, and I don't see any reason to block this.
@carlosgalvezp Fell free to look into this in some free time if you see any bugs/issues.

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.h

 //===--- NoexceptMoveConstructorCheck.h - clang-tidy-------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOEXCEPTMOVECONSTRUCTORCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOEXCEPTMOVECONSTRUCTORCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/ExceptionSpecAnalyzer.h"
 
 namespace clang::tidy::performance {
 
 /// The check flags user-defined move constructors and assignment operators not
 /// marked with `noexcept` or marked with `noexcept(expr)` where `expr`
 /// evaluates to `false` (but is not a `false` literal itself).
 ///
 /// Move constructors of all the types used with STL containers, for example,
 /// need to be declared `noexcept`. Otherwise STL will choose copy constructors
 /// instead. The same is valid for move assignment operations.
 class NoexceptMoveConstructorCheck : public ClangTidyCheck {
 public:
   NoexceptMoveConstructorCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
-    return LangOpts.CPlusPlus11;
+    return LangOpts.CPlusPlus11 && LangOpts.CXXExceptions;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  utils::ExceptionSpecAnalyzer SpecAnalyzer;
 };
 
 } // namespace clang::tidy::performance
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOEXCEPTMOVECONSTRUCTORCHECK_H

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp

 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::performance {
 
 void NoexceptMoveConstructorCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      cxxMethodDecl(anyOf(cxxConstructorDecl(), hasOverloadedOperatorName("=")),
+      cxxMethodDecl(anyOf(cxxConstructorDecl(isMoveConstructor()),
+                          hasOverloadedOperatorName("=")),
                     unless(isImplicit()), unless(isDeleted()))
           .bind("decl"),
       this);
 }
 
 void NoexceptMoveConstructorCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<CXXMethodDecl>("decl")) {
     bool IsConstructor = false;
     if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Decl)) {
-      if (!Ctor->isMoveConstructor())
-        return;
       IsConstructor = true;
     } else if (!Decl->isMoveAssignmentOperator()) {
       return;
     }
 
-    const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>();
+    if (SpecAnalyzer.analyze(Decl) !=
+        utils::ExceptionSpecAnalyzer::State::Throwing) {
+      return;
+    }
 
-    if (isUnresolvedExceptionSpec(ProtoType->getExceptionSpecType()))
+    // Don't complain about nothrow(false), but complain on nothrow(expr)
+    // where expr evaluates to false.
+    const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>();
+    const Expr *NoexceptExpr = ProtoType->getNoexceptExpr();
+    if (NoexceptExpr) {
+      NoexceptExpr = NoexceptExpr->IgnoreImplicit();
+      if (!isa<CXXBoolLiteralExpr>(NoexceptExpr)) {
+        diag(NoexceptExpr->getExprLoc(),
+             "noexcept specifier on the move %select{assignment "
+             "operator|constructor}0 evaluates to 'false'")
+            << IsConstructor;
+      }
       return;
+    }
 
-    if (!isNoexceptExceptionSpec(ProtoType->getExceptionSpecType())) {
-      auto Diag = diag(Decl->getLocation(),
-                       "move %select{assignment operator|constructor}0s should "
-                       "be marked noexcept")
-                  << IsConstructor;
-      // Add FixIt hints.
-      SourceManager &SM = *Result.SourceManager;
-      assert(Decl->getNumParams() > 0);
-      SourceLocation NoexceptLoc = Decl->getParamDecl(Decl->getNumParams() - 1)
-                                       ->getSourceRange()
-                                       .getEnd();
-      if (NoexceptLoc.isValid())
-        NoexceptLoc = Lexer::findLocationAfterToken(
-            NoexceptLoc, tok::r_paren, SM, Result.Context->getLangOpts(), true);
-      if (NoexceptLoc.isValid())
-        Diag << FixItHint::CreateInsertion(NoexceptLoc, " noexcept ");
-      return;
-    }
-
-    // Don't complain about nothrow(false), but complain on nothrow(expr)
-    // where expr evaluates to false.
-    if (ProtoType->canThrow() == CT_Can) {
-      Expr *E = ProtoType->getNoexceptExpr();
-      E = E->IgnoreImplicit();
-      if (!isa<CXXBoolLiteralExpr>(E)) {
-        diag(E->getExprLoc(),
-             "noexcept specifier on the move %select{assignment "
-             "operator|constructor}0 evaluates to 'false'")
-            << IsConstructor;
-      }
-    }
-  }
+    SourceLocation NoexceptLoc =
+        Decl->getParamDecl(Decl->getNumParams() - 1)->getSourceRange().getEnd();
 }
 
 } // namespace clang::tidy::performance

clang-tools-extra/clang-tidy/utils/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyUtils
   Aliasing.cpp
   ASTUtils.cpp
   DeclRefExprUtils.cpp
   ExceptionAnalyzer.cpp
+  ExceptionSpecAnalyzer.cpp
   ExprSequence.cpp
   FileExtensionsUtils.cpp
   FixItHintUtils.cpp
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
   NamespaceAliaser.cpp

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h

+//===--- ExceptionSpecAnalyzer.h - clang-tidy -------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_SPEC_ANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_SPEC_ANALYZER_H
+
+#include "clang/AST/DeclCXX.h"
+#include "llvm/ADT/DenseMap.h"
+
+namespace clang::tidy::utils {
+
+/// This class analysis if a `FunctionDecl` has been declared implicitly through
+/// defaulting or explicitly as throwing or not and evaluates noexcept
+/// expressions if needed. Unlike the `ExceptionAnalyzer` however it can't tell
+/// you if the function will actually throw an exception or not.
+class ExceptionSpecAnalyzer {
+public:
+  enum class State : std::int8_t {
+    Throwing,    ///< This function has been declared as possibly throwing.
+    NotThrowing, ///< This function has been declared as not throwing.
+    Unknown, ///< We're unable to tell if this function is declared as throwing
+             ///< or not.
+  };
+
+  ExceptionSpecAnalyzer() = default;
+
+  State analyze(const FunctionDecl *FuncDecl);
+
+private:
+  enum class DefaultableMemberKind : std::int8_t {
+    DefaultConstructor,
+    CopyConstructor,
+    MoveConstructor,
+    CopyAssignment,
+    MoveAssignment,
+    Destructor,
+
+    CompareEqual,
+    CompareNotEqual,
+    CompareThreeWay,
+    CompareRelational,
+
+    None,
+  };
+
+  State analyzeImpl(const FunctionDecl *FuncDecl);
+
+  State analyzeUnresolvedOrDefaulted(const FunctionDecl *FuncDecl);
+
+  State analyzeFieldDecl(const FieldDecl *FDecl, DefaultableMemberKind Kind);
+
+  State analyzeBase(const CXXBaseSpecifier &Base, DefaultableMemberKind Kind);
+
+  enum class SkipMethods : bool {
+    Yes = true,
+    No = false,
+  };
+
+  State analyzeRecord(const CXXRecordDecl *RecDecl, DefaultableMemberKind Kind,
+                      SkipMethods SkipMethods = SkipMethods::No);
+
+  static State analyzeFunctionEST(const FunctionDecl *FuncDecl);
+
+  static bool hasTrivialMemberKind(const CXXRecordDecl *RecDecl,
+                                   DefaultableMemberKind Kind);
+
+  static bool isConstructor(DefaultableMemberKind Kind);
+
+  static bool isSpecialMember(DefaultableMemberKind Kind);
+
+  static bool isComparison(DefaultableMemberKind Kind);
+
+  DefaultableMemberKind getDefaultableMemberKind(const FunctionDecl *FuncDecl);
+
+  llvm::DenseMap<const FunctionDecl *, State> FunctionCache{32u};
+};
+
+} // namespace clang::tidy::utils
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_SPEC_ANALYZER_H

clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp

+//===--- ExceptionSpecAnalyzer.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 "ExceptionSpecAnalyzer.h"
+
+#include "clang/AST/Expr.h"
+
+namespace clang::tidy::utils {
+
+enum class BasesToVisit : std::int8_t {
+  /// Visit all non-virtual (direct) bases.
+  VisitNonVirtualBases,
+  /// Visit all direct bases, virtual or not.
+  VisitDirectBases,
+  /// Visit all non-virtual bases, and all virtual bases if the class
+  /// is not abstract.
+  VisitPotentiallyConstructedBases,
+  /// Visit all direct or virtual bases.
+  VisitAllBases,
+};
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyze(const FunctionDecl *FuncDecl) {
+  ExceptionSpecAnalyzer::State State;
+
+  // Check if the function has already been analyzed and reuse that result.
+  if (FunctionCache.count(FuncDecl) == 0) {
+    State = analyzeImpl(FuncDecl);
+
+    // Cache the result of the analysis.
+    FunctionCache.insert(std::make_pair(FuncDecl, State));
+  } else
+    State = FunctionCache[FuncDecl];
+
+  return State;
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeUnresolvedOrDefaulted(
+    const FunctionDecl *FuncDecl) {
+  const auto *FunProto = FuncDecl->getType()->getAs<FunctionProtoType>();
+  if (!FunProto)
+    return State::Unknown;
+
+  const auto *MethodDecl = cast<CXXMethodDecl>(FuncDecl);
+  if (!MethodDecl)
+    return State::Unknown;
+
+  const CXXRecordDecl *RecordDecl = MethodDecl->getParent();
+  if (!RecordDecl)
+    return State::Unknown;
+
+  const DefaultableMemberKind Kind = getDefaultableMemberKind(FuncDecl);
+
+  if (Kind == DefaultableMemberKind::None)
+    return State::Unknown;
+
+  return analyzeRecord(RecordDecl, Kind, SkipMethods::Yes);
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeFieldDecl(const FieldDecl *FDecl,
+                                        DefaultableMemberKind Kind) {
+  if (!FDecl)
+    return State::Unknown;
+
+  if (const CXXRecordDecl *RecDecl =
+          FDecl->getType()->getUnqualifiedDesugaredType()->getAsCXXRecordDecl())
+    return analyzeRecord(RecDecl, Kind);
+
+  // FIXME: There may still be a way to figure out if the field can throw or not
+  return State::Unknown;
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeBase(const CXXBaseSpecifier &Base,
+                                   DefaultableMemberKind Kind) {
+  const auto *RecType = Base.getType()->getAs<RecordType>();
+  if (!RecType)
+    return State::Unknown;
+
+  const auto *BaseClass = cast<CXXRecordDecl>(RecType->getDecl());
+
+  return analyzeRecord(BaseClass, Kind);
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeRecord(const CXXRecordDecl *RecordDecl,
+                                     DefaultableMemberKind Kind,
+                                     SkipMethods SkipMethods) {
+  if (!RecordDecl)
+    return State::Unknown;
+
+  // Trivial implies noexcept
+  if (hasTrivialMemberKind(RecordDecl, Kind))
+    return State::NotThrowing;
+
+  if (SkipMethods == SkipMethods::No)
+    for (const auto *MethodDecl : RecordDecl->methods())
+      if (getDefaultableMemberKind(MethodDecl) == Kind)
+        return analyze(MethodDecl);
+
+  BasesToVisit Bases = isConstructor(Kind)
+                           ? BasesToVisit::VisitPotentiallyConstructedBases
+                           : BasesToVisit::VisitAllBases;
+
+  if (Bases == BasesToVisit::VisitPotentiallyConstructedBases)
+    Bases = RecordDecl->isAbstract() ? BasesToVisit::VisitNonVirtualBases
+                                     : BasesToVisit::VisitAllBases;
+
+  for (const auto &BaseSpec : RecordDecl->bases())
+    if (Bases == BasesToVisit::VisitDirectBases || !BaseSpec.isVirtual()) {
+      State Result = analyzeBase(BaseSpec, Kind);
+      if (Result == State::Throwing || Result == State::Unknown)
+        return Result;
+    }
+
+  if (Bases == BasesToVisit::VisitAllBases)
+    for (const auto &BaseSpec : RecordDecl->vbases()) {
+      State Result = analyzeBase(BaseSpec, Kind);
+      if (Result == State::Throwing || Result == State::Unknown)
+        return Result;
+    }
+
+  for (const auto *FDecl : RecordDecl->fields())
+    if (!FDecl->isInvalidDecl() && !FDecl->isUnnamedBitfield()) {
+      State Result = analyzeFieldDecl(FDecl, Kind);
+      if (Result == State::Throwing || Result == State::Unknown)
+        return Result;
+    }
+
+  return State::NotThrowing;
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeImpl(const FunctionDecl *FuncDecl) {
+  const auto *FuncProto = FuncDecl->getType()->getAs<FunctionProtoType>();
+  if (!FuncProto)
+    return State::Unknown;
+
+  const ExceptionSpecificationType EST = FuncProto->getExceptionSpecType();
+
+  if (EST == EST_Unevaluated || (EST == EST_None && FuncDecl->isDefaulted()))
+    return analyzeUnresolvedOrDefaulted(FuncDecl);
+
+  return analyzeFunctionEST(FuncDecl);
+}
+
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeFunctionEST(const FunctionDecl *FuncDecl) {
+  const auto *FuncProto = FuncDecl->getType()->getAs<FunctionProtoType>();
+  if (!FuncProto)
+    return State::Unknown;
+
+  if (isUnresolvedExceptionSpec(FuncProto->getExceptionSpecType()))
+    return State::Unknown;
+
+  switch (FuncProto->canThrow()) {
+  case CT_Cannot:
+    return State::NotThrowing;
+  case CT_Dependent: {
+    const Expr *NoexceptExpr = FuncProto->getNoexceptExpr();
+    bool Result;
+    return (NoexceptExpr && !NoexceptExpr->isValueDependent() &&
+            NoexceptExpr->EvaluateAsBooleanCondition(
+                Result, FuncDecl->getASTContext(), true) &&
+            Result)
+               ? State::NotThrowing
+               : State::Throwing;
+  }
+  default:
+    return State::Throwing;
+  };
+}
+
+bool ExceptionSpecAnalyzer::hasTrivialMemberKind(const CXXRecordDecl *RecDecl,
+                                                 DefaultableMemberKind Kind) {
+  if (!RecDecl)
+    return false;
+
+  switch (Kind) {
+  case DefaultableMemberKind::DefaultConstructor:
+    return RecDecl->hasTrivialDefaultConstructor();
+  case DefaultableMemberKind::CopyConstructor:
+    return RecDecl->hasTrivialCopyConstructor();
+  case DefaultableMemberKind::MoveConstructor:
+    return RecDecl->hasTrivialMoveConstructor();
+  case DefaultableMemberKind::CopyAssignment:
+    return RecDecl->hasTrivialCopyAssignment();
+  case DefaultableMemberKind::MoveAssignment:
+    return RecDecl->hasTrivialMoveAssignment();
+  case DefaultableMemberKind::Destructor:
+    return RecDecl->hasTrivialDestructor();
+
+  default:
+    return false;
+  }
+}
+
+bool ExceptionSpecAnalyzer::isConstructor(DefaultableMemberKind Kind) {
+  switch (Kind) {
+  case DefaultableMemberKind::DefaultConstructor:
+  case DefaultableMemberKind::CopyConstructor:
+  case DefaultableMemberKind::MoveConstructor:
+    return true;
+
+  default:
+    return false;
+  }
+}
+
+bool ExceptionSpecAnalyzer::isSpecialMember(DefaultableMemberKind Kind) {
+  switch (Kind) {
+  case DefaultableMemberKind::DefaultConstructor:
+  case DefaultableMemberKind::CopyConstructor:
+  case DefaultableMemberKind::MoveConstructor:
+  case DefaultableMemberKind::CopyAssignment:
+  case DefaultableMemberKind::MoveAssignment:
+  case DefaultableMemberKind::Destructor:
+    return true;
+  default:
+    return false;
+  }
+}
+
+bool ExceptionSpecAnalyzer::isComparison(DefaultableMemberKind Kind) {
+  switch (Kind) {
+  case DefaultableMemberKind::CompareEqual:
+  case DefaultableMemberKind::CompareNotEqual:
+  case DefaultableMemberKind::CompareRelational:
+  case DefaultableMemberKind::CompareThreeWay:
+    return true;
+  default:
+    return false;
+  }
+}
+
+ExceptionSpecAnalyzer::DefaultableMemberKind
+ExceptionSpecAnalyzer::getDefaultableMemberKind(const FunctionDecl *FuncDecl) {
+  if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl)) {
+    if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(FuncDecl)) {
+      if (Ctor->isDefaultConstructor())
+        return DefaultableMemberKind::DefaultConstructor;
+
+      if (Ctor->isCopyConstructor())
+        return DefaultableMemberKind::CopyConstructor;
+
+      if (Ctor->isMoveConstructor())
+        return DefaultableMemberKind::MoveConstructor;
+    }
+
+    if (MethodDecl->isCopyAssignmentOperator())
+      return DefaultableMemberKind::CopyAssignment;
+
+    if (MethodDecl->isMoveAssignmentOperator())
+      return DefaultableMemberKind::MoveAssignment;
+
+    if (isa<CXXDestructorDecl>(FuncDecl))
+      return DefaultableMemberKind::Destructor;
+  }
+
+  const LangOptions &LangOpts = FuncDecl->getLangOpts();
+
+  switch (FuncDecl->getDeclName().getCXXOverloadedOperator()) {
+  case OO_EqualEqual:
+    return DefaultableMemberKind::CompareEqual;
+
+  case OO_ExclaimEqual:
+    return DefaultableMemberKind::CompareNotEqual;
+
+  case OO_Spaceship:
+    // No point allowing this if <=> doesn't exist in the current language mode.
+    if (!LangOpts.CPlusPlus20)
+      break;
+    return DefaultableMemberKind::CompareThreeWay;
+
+  case OO_Less:
+  case OO_LessEqual:
+  case OO_Greater:
+  case OO_GreaterEqual:
+    // No point allowing this if <=> doesn't exist in the current language mode.
+    if (!LangOpts.CPlusPlus20)
+      break;
+    return DefaultableMemberKind::CompareRelational;
+
+  default:
+    break;
+  }
+
+  // Not a defaultable member kind
+  return DefaultableMemberKind::None;
+}
+
+} // namespace clang::tidy::utils

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
+- Fixed an issue in :doc:`performance-noexcept-move-constructor
+  <clang-tidy/checks/performance/noexcept-move-constructor>` which resulted in false
+  positives if the constructor was defaulted.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp

-// RUN: %check_clang_tidy %s performance-noexcept-move-constructor %t
+// RUN: %check_clang_tidy %s performance-noexcept-move-constructor %t -- -- -fexceptions
 
 struct C_1 {
  ~C_1() {}
  C_1(int a) {}
  C_1(C_1&& a) :C_1(5) {}
  // CHECK-FIXES: ){{.*}}noexcept{{.*}}:
  C_1& operator=(C_1&&) { return *this; }
  // CHECK-FIXES: ){{.*}}noexcept{{.*}} {

  ~C_3() {}
  C_3(C_3&& a);
 // CHECK-FIXES: ){{.*}}noexcept{{.*}};
  C_3& operator=(C_3&& a);
 // CHECK-FIXES: ){{.*}}noexcept{{.*}};
 };
 
 C_3::C_3(C_3&& a) = default;
-// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default;
 C_3& C_3::operator=(C_3&& a) = default;
-// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default;
 
 template <class T>
 struct C_4 {
  C_4(C_4<T>&&) {}
 // CHECK-FIXES: ){{.*}}noexcept{{.*}} {}
  ~C_4() {}
  C_4& operator=(C_4&& a) = default;
-// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default;
 };
 
 template <class T>
 struct C_5 {
  C_5(C_5<T>&&) {}
 // CHECK-FIXES:){{.*}}noexcept{{.*}} {}
  ~C_5() {}
  auto operator=(C_5&& a)->C_5<T> = default;
-// CHECK-FIXES:){{.*}}noexcept{{.*}} = default;
 };
 
 template <class T>
 struct C_6 {
  C_6(C_6<T>&&) {}
 // CHECK-FIXES:){{.*}}noexcept{{.*}} {}
  ~C_6() {}
  auto operator=(C_6&& a)->C_6<T>;
 // CHECK-FIXES:){{.*}}noexcept{{.*}};
 };
 
 template <class T>
 auto C_6<T>::operator=(C_6<T>&& a) -> C_6<T> {}
-// CHECK-FIXES: ){{.*}}noexcept{{.*}} {}
+// CHECK-FIXES: ){{.*}}noexcept{{.*}} {}
 

clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp

-// RUN: %check_clang_tidy %s performance-noexcept-move-constructor %t
+// RUN: %check_clang_tidy %s performance-noexcept-move-constructor %t -- -- -fexceptions
+
+struct Empty
+{};
+
+struct IntWrapper {
+  int value;
+};
 
 class A {
   A(A &&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
   A &operator=(A &&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
 };
 
 struct B {
   static constexpr bool kFalse = false;
   B(B &&) noexcept(kFalse);
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
 };
 
+template <typename>
+struct C
+{
+  C(C &&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  C& operator=(C &&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+};
+
+struct D
+{
+  static constexpr bool kFalse = false;
+  D(D &&) noexcept(kFalse) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  D& operator=(D &&) noexcept(kFalse) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false'
+};
+
+template <typename>
+struct E
+{
+  static constexpr bool kFalse = false;
+  E(E &&) noexcept(kFalse);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  E& operator=(E &&) noexcept(kFalse);
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false'
+};
+
+template <typename>
+struct F
+{
+  static constexpr bool kFalse = false;
+  F(F &&) noexcept(kFalse) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  F& operator=(F &&) noexcept(kFalse) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false'
+};
+
+struct Field
+{
+  Field() = default;
+  Field(Field&&) noexcept(false) {
+  }
+  Field& operator=(Field &&) noexcept(false) {
+    return *this;
+  }
+};
+
+struct G {
+  G(G &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  G& operator=(G &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+
+  Field field;
+};
+
+void throwing_function() noexcept(false) {}
+
+struct H {
+  H(H &&) noexcept(noexcept(throwing_function()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  H &operator=(H &&) noexcept(noexcept(throwing_function()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false'
+};
+
+template <typename>
+struct I {
+  I(I &&) noexcept(noexcept(throwing_function()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  I &operator=(I &&) noexcept(noexcept(throwing_function()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false'
+};
+
+template <typename TemplateType> struct TemplatedType {
+  static void f() {}
+};
+
+template <> struct TemplatedType<int> {
+  static void f() noexcept {}
+};
+
+struct J {
+  J(J &&) noexcept(noexcept(TemplatedType<double>::f()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]
+  J &operator=(J &&) noexcept(noexcept(TemplatedType<double>::f()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor]
+};
+
+struct K : public Field {
+  K(K &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  K &operator=(K &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+};
+
+struct InheritFromField : public Field
+{};
+
+struct L {
+  L(L &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  L &operator=(L &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+
+  InheritFromField IFF;
+};
+
+struct M : public InheritFromField {
+  M(M &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  M &operator=(M &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+};
+
+struct N : public IntWrapper, Field {
+  N(N &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  N &operator=(N &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+};
+
+struct O : virtual IntWrapper, Field {
+  O(O &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor]
+  O &operator=(O &&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]
+};
+
 class OK {};
 
 void f() {
   OK a;
   a = OK();
 }
 
 class OK1 {
struct OK4 {
   OK4(OK4 &&) noexcept = default;
   OK4 &operator=(OK4 &&) noexcept = default;
 };
 
 struct OK5 {
   OK5(OK5 &&) noexcept(true) = default;
   OK5 &operator=(OK5 &&) noexcept(true) = default;
 };
+
+struct OK6 {
+  OK6(OK6 &&) = default;
+  OK6& operator=(OK6 &&) = default;
+};
+
+template <typename>
+struct OK7 {
+  OK7(OK7 &&) = default;
+  OK7& operator=(OK7 &&) = default;
+};
+
+template <typename>
+struct OK8 {
+  OK8(OK8 &&) noexcept = default;
+  OK8& operator=(OK8 &&) noexcept = default;
+};
+
+template <typename>
+struct OK9 {
+  OK9(OK9 &&) noexcept(true) = default;
+  OK9& operator=(OK9 &&) noexcept(true) = default;
+};
+
+template <typename>
+struct OK10 {
+  OK10(OK10 &&) noexcept(false) = default;
+  OK10& operator=(OK10 &&) noexcept(false) = default;
+};
+
+template <typename>
+struct OK11 {
+  OK11(OK11 &&) = delete;
+  OK11& operator=(OK11 &&) = delete;
+};
+
+void nothrowing_function() noexcept {}
+
+struct OK12 {
+  OK12(OK12 &&) noexcept(noexcept(nothrowing_function()));
+  OK12 &operator=(OK12 &&) noexcept(noexcept(nothrowing_function));
+};
+
+struct OK13 {
+  OK13(OK13 &&) noexcept(noexcept(nothrowing_function)) = default;
+  OK13 &operator=(OK13 &&) noexcept(noexcept(nothrowing_function)) = default;
+};
+
+template <typename> struct OK14 {
+  OK14(OK14 &&) noexcept(noexcept(TemplatedType<int>::f()));
+  OK14 &operator=(OK14 &&) noexcept(noexcept(TemplatedType<int>::f()));
+};
+
+struct OK15 {
+  OK15(OK15 &&) = default;
+  OK15 &operator=(OK15 &&) = default;
+
+  int member;
+};
+
+template <typename>
+struct OK16 {
+  OK16(OK16 &&) = default;
+  OK16 &operator=(OK16 &&) = default;
+
+  int member;
+};
+
+struct OK17
+{
+  OK17(OK17 &&) = default;
+  OK17 &operator=(OK17 &&) = default;
+  OK empty_field;
+};
+
+template <typename>
+struct OK18
+{
+  OK18(OK18 &&) = default;
+  OK18 &operator=(OK18 &&) = default;
+
+  OK empty_field;
+};
+
+struct OK19 : public OK
+{
+  OK19(OK19 &&) = default;
+  OK19 &operator=(OK19 &&) = default;
+};
+
+struct OK20 : virtual OK
+{
+  OK20(OK20 &&) = default;
+  OK20 &operator=(OK20 &&) = default;
+};
+
+template <typename T>
+struct OK21 : public T
+{
+  OK21() = default;
+  OK21(OK21 &&) = default;
+  OK21 &operator=(OK21 &&) = default;
+};
+
+template <typename T>
+struct OK22 : virtual T
+{
+  OK22() = default;
+  OK22(OK22 &&) = default;
+  OK22 &operator=(OK22 &&) = default;
+};
+
+template <typename T>
+struct OK23 {
+  OK23()= default;
+  OK23(OK23 &&) = default;
+  OK23 &operator=(OK23 &&) = default;
+
+  T member;
+};
+
+void testTemplates() {
+  OK21<Empty> value(OK21<Empty>{});
+  value = OK21<Empty>{};
+
+  OK22<Empty> value2{OK22<Empty>{}};
+  value2 = OK22<Empty>{};
+
+  OK23<Empty> value3{OK23<Empty>{}};
+  value3 =OK23<Empty>{};
+}
+
+struct OK24 : public Empty, OK1 {
+  OK24(OK24 &&) = default;
+  OK24 &operator=(OK24 &&) = default;
+};
+
+struct OK25 : virtual Empty, OK1 {
+  OK25(OK25 &&) = default;
+  OK25 &operator=(OK25 &&) = default;
+};
+
+struct OK26 : public Empty, IntWrapper {
+  OK26(OK26 &&) = default;
+  OK26 &operator=(OK26 &&) = default;
+};
+
+template <typename T>
+struct OK27 : public T
+{
+  OK27(OK27 &&) = default;
+  OK27 &operator=(OK27 &&) = default;
+};
+
+template <typename T>
+struct OK28 : virtual T
+{
+  OK28(OK28 &&) = default;
+  OK28 &operator=(OK28 &&) = default;
+};
+
+template <typename T>
+struct OK29 {
+  OK29(OK29 &&) = default;
+  OK29 &operator=(OK29 &&) = default;
+
+  T member;
+};