This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by AMS21 on Mar 26 2023, 11:08 AM.

Details

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

Diff Detail

Event Timeline

AMS21 created this revision.Mar 26 2023, 11:08 AM
AMS21 requested review of this revision.Mar 26 2023, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 11:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL requested changes to this revision.Mar 26 2023, 11:28 AM

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

This revision now requires changes to proceed.Mar 26 2023, 11:28 AM

Update ReleaseNotes for this check, and validate documentation.

Eugene.Zelenko added inline comments.
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp
64

Please add.

AMS21 added a comment.Mar 27 2023, 5:07 AM

@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.

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 updated this revision to Diff 509013.Mar 28 2023, 7:50 AM

Implemented suggested fixes from reviews

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 requested changes to this revision.Mar 28 2023, 9:32 AM

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

This revision now requires changes to proceed.Mar 28 2023, 9:32 AM
AMS21 updated this revision to Diff 509405.Mar 29 2023, 9:54 AM
AMS21 marked an inline comment as done.

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

AMS21 added a comment.EditedMar 29 2023, 9:55 AM

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?

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

Should be const auto *.

87

Ditto.

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
267

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 added inline comments.Mar 29 2023, 10:10 AM
clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp
278

add test with 2 base classes (no templates).

AMS21 updated this revision to Diff 509409.Mar 29 2023, 10:11 AM
AMS21 marked 8 inline comments as done.

Sprinkle some more const

PiotrZSL added inline comments.Mar 29 2023, 10:12 AM
clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h
79

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

AMS21 marked 2 inline comments as done.Mar 29 2023, 10:14 AM
AMS21 added inline comments.
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp
21

Well thats embarassing

AMS21 updated this revision to Diff 509417.Mar 29 2023, 10:46 AM

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

AMS21 marked 3 inline comments as done.Mar 29 2023, 10:46 AM
AMS21 edited the summary of this revision. (Show Details)Mar 29 2023, 10:54 AM
AMS21 marked an inline comment as done.Mar 29 2023, 11:08 PM

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–51

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

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

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

37

try_emplace ?

45

pass FunctionProtoType also to this function

51–53

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

77

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

96

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

109–115

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

156

pass FunctionProtoType also to this function, to avoid double checking

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

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

79

this also can be static

Eugene.Zelenko added inline comments.Mar 30 2023, 7:46 AM
clang-tools-extra/docs/ReleaseNotes.rst
260

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

AMS21 updated this revision to Diff 509695.Mar 30 2023, 9:03 AM
AMS21 marked 15 inline comments as done.

Implement review suggestions

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp
45–51

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
96

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.

96

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++.

109–115

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
24

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

PiotrZSL accepted this revision.Mar 30 2023, 9:16 AM

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–51

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.

This revision is now accepted and ready to land.Mar 30 2023, 9:16 AM
AMS21 added inline comments.Mar 30 2023, 9:18 AM
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 added inline comments.Mar 30 2023, 9:20 AM
clang-tools-extra/docs/ReleaseNotes.rst
260

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

AMS21 updated this revision to Diff 509911.Mar 30 2023, 11:13 PM
AMS21 marked 3 inline comments as done.

Fixed docs and cleanup tests a bit

clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp
45–51

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.

AMS21 edited the summary of this revision. (Show Details)Mar 30 2023, 11:14 PM
PiotrZSL accepted this revision.Apr 10 2023, 11:54 PM

Any plans in delivering this ?

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

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.