[clang-tidy] Implement CppCoreGuideline F.18

[ccotter]

Summary

Warn when an rvalue reference function paramter is never moved
from within the function body.

[ccotter]

rebase again

[ccotter]

@PiotrZSL I added an option to allow disabling the strict behavior. It should address many of your examples (e.g., moving subobjects) . Let me know if you have more feedback.

[PiotrZSL]

In D141569#4136493, @ccotter wrote:

@PiotrZSL I added an option to allow disabling the strict behavior. It should address many of your examples (e.g., moving subobjects) . Let me know if you have more feedback.

I will re-test this on Monday/Tuesday. and comeback to you if I find something.

[ccotter]

• Add back -fno-delayed-template-parsing to test

[PiotrZSL]

I don't know why but with latest version I still got false positive with std::function on Clang 15.

using Functor= std::function<... something ...>;

std::unique_ptr Class::createSomething(::Functor&& functor) {
   return std::make_unique<SomeType>(std::move(functor));
}

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

42

duplicated isDefinition and ToParam

[PiotrZSL]

Other false-positive that I still see:

void SomeClass::someVirtualFunction(SomeType&&)
{
BOOST_THROW_EXCEPTION(std::runtime_error("Unexpected call ...."));
}

It shouldnt warn about virtual functions with unnamed parameter, because this can be required by interface.

[PiotrZSL]

And on something like this I still got reproduction in production code:

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp

302

that's not always rvalue, if T would be for example int&, then this going to be lvalue...

[ccotter]

• Remove duplicated matchers

[ccotter]

In

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};

T is not a universal reference in the constructor, it's an rvalue reference to T. There is no deducing context, so universal references are not involved here (and, std::forward would also be incorrect here). The following would be a deducing context with a universal reference:

template <int tagValue, typename T>
struct SomeClass
{
public:
  template <class T2>
  explicit SomeClass(T2&& value) : value(std::forward<T2>(value)) {}
 T value;
};

I thought there was a clang-tidy check that flagged applying std::forward on a non-universal reference, but I don't see one.

[ccotter]

I was split on handling the case where the parameter that was never moved from was not named. For this guideline enforcement to flag all unmoved rvalue reference parameters, code that std::moves into an argument to a virtual method call could be especially confusing or dangerous with regards to whether the object was truly "moved" (move constructor/assignment invoked within the method call). E.g.,

LargeObject obj;
ClassWithVirtualMerthods* v = ...;
v->some_virtual_call(std::move(obj));

If would be confusing if some implementations of the virtual method actually moved the parameter, while others didn't. I'm inclined in this case (regardless of whether the parameter is named in a virtual overridden method) to still flag this code. Looking at the rule enforcement criteria again, the rule states "Don’t conditionally move from objects" which this case falls into.

I could see there being an option for this specific case (defaulted off), although it's very specific of a case, and for something I think the rule should be especially attentive towards. Could I get some additional feedback on this specific case?

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp

302

Looking at the rule enforcement, I might have overlooked one thing:

> Flag all X&& parameters (where X is not a template type parameter name) where the function body uses them without std::move.

I think this means this code should not be flagged, even if T is an object which is cheap to move but expensive to copy (and not a reference to that type). I would like for AClassTemplate<LargeObject> to be flagged, but not AClassTemplate<LargeObject&>, so let me look into this to see if I can limit this to instantiations of the type, rather than looking at the template definition itself.

[PiotrZSL]

In D141569#4139290, @ccotter wrote:

In

template <int tagValue, typename T>
struct SomeClass
{
public:
  explicit SomeClass(T&& value) : value(std::forward<T>(value)) {}
 T value;
};

T is not a universal reference in the constructor, it's an rvalue reference to T. There is no deducing context, so universal references are not involved here (and, std::forward would also be incorrect here). The following would be a deducing context with a universal reference:

Wrong:

#include <utility>

template<typename T>
struct Wrap {

  Wrap(T&& arg) 
    : value(arg)
  {}

  T value;  
};

struct Value {};

int main()
{
    Value value;
    Wrap<Value> v1(std::move(value));
    Wrap<Value> v2(std::move(value));
    Wrap<Value&> v3(value);
    return 0;
}

If you add std::move you will get compilation error, if you add std::forward, everything will work fine. Simply Value& and && will evaluate to Value&, no rvalue reference here.

[PiotrZSL]

In D141569#4139320, @ccotter wrote:

I was split on handling the case where the parameter that was never moved from was not named. For this guideline enforcement to flag all unmoved rvalue reference parameters, code that std::moves into an argument to a virtual method call could be especially confusing or dangerous with regards to whether the object was truly "moved" (move constructor/assignment invoked within the method call). E.g.,

LargeObject obj;
ClassWithVirtualMerthods* v = ...;
v->some_virtual_call(std::move(obj));

If would be confusing if some implementations of the virtual method actually moved the parameter, while others didn't. I'm inclined in this case (regardless of whether the parameter is named in a virtual overridden method) to still flag this code. Looking at the rule enforcement criteria again, the rule states "Don’t conditionally move from objects" which this case falls into.

I could see there being an option for this specific case (defaulted off), although it's very specific of a case, and for something I think the rule should be especially attentive towards. Could I get some additional feedback on this specific case?

I'm not so sure, you may have Base class Car that would have void changeBackRightDoor(Door&&) method, and then you could have class Car5Doors and Car3Doors, in such case its easy to known that Car3Doors wouldn't implement changeBackRightDoor method, and that method wouldnt move anything, using unnamed parameters is a way to go. If you dont exclude this, then at least provide option for that, like IgnoreUnnamedParameters.

[ccotter]

• Ignore params of template type
• Add option for unnamed params

[ccotter]

> If you add std::move you will get compilation error, if you add std::forward, everything will work fine. Simply Value& and && will evaluate to Value&, no rvalue reference here.

I agree that examining the template definition alone is not correct. In your original example with SomeClass, T is not a forwarding reference since T is not deduced in the constructor, so it's misleading to use forward in this context. forward "works" and leads to the behavior you are likely seeking in your SomeClass example, but not in the usual way the most readers would expect (the spirit of the CppCoreGuidelines is to provide certain restricted ideas as recommendations, and using forward in this way violates ES.56: "Flag when std::forward is applied to other than a forwarding reference."). Usually, I'd expect to see a class template on a type to have different specializations for T being a non-reference and another for T being a reference (e.g., std::future), or only allow T to be a reference or non-reference (e.g., std::optional), but not both.

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp

46

@carlosgalvezp Note I've changed the check to not flag never_moves based on more recent discussion. I do think this code should flag, although I'm a little torn. If Foo constrains T to be a reference, then Foo<Obj&>::never_moves is "OK", although I think it's still confusing as never_moves could instead accept T&.

I'll need to rethink this a bit, but I've never the test in place with a FIXME and brief comment explaining the situation.

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp

46

That should read "I've left the test in place"

[PiotrZSL]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

22

and you dont need allOf

23–25

those lines should be equal to just: parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"),
i'm not sure why you need additional equalsBoundNode, thats useful mainly if you would do some tricks with hasParent or something...

26–30

you could try to merge those 3:

unless(hasType(qualType(references(anyOf(isConstQualified(), templateTypeParmType(hasDeclaration(templateTypeParmDecl())), substTemplateTypeParmType())))

41

you already got constructor as second in any of, it shouldnt be matched here, so this line shouldnt be needed

47

you dont need to split things like std::move or lambda detections.

you can add this right to matcher, like:

unless(hasDescendant(lambdaExpr(hasAnyCapture(capturesVar(equalsBoundMode("param"), ...

and here you may need to add new matcher to match ByCopy))
but you dont need to check lambdas in this way, you dont need to check them anyway, they should be check as descendants of functions

104–126

unless(hasDescendant(callExpr(
      anyOf(callee(functionDecl(hasName("::std::move"))),
            callee(unresolvedLookupExpr(hasAnyDeclaration(
                namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),
      argumentCountIs(1), hasArgument(0, moveArgumentOf(StrictMode, Param)))))

any here you could also check lambdas, if you want to ignore copies, but not should why you should do that, but you can always bind this to "move", or implement lambda checking as matcher.... unless(hasAncestor(lambdaExpr(isCapturedByCopy....

Simply as you have some tests already, try pack as much as you can into registerMatchers, because once you split this across multiple submatches in diffrent functions, then it could be hard to maintain.

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h

33–34

use bool here, or it could cause some issues, you wont save anything by using bits. class alignment is already 8.
problem is that you read bools but in storeOptions you store ints, and when dump-config will be used it will show 1/0 not true/false.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst

2

add explicit documentation about options

14

one example

[PiotrZSL]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

27–30

if with this you want to exclude parameters that depend on templates, consider looking into isTemplateInstantiation AST matcher, other option could be to get DeclContext and check isDependentContext(). You would need to check this, maybe look into other checks how its done there. Simply because if this would be for example T* &&, then it could not be detected.

[ccotter]

• Simplify matcher

[ccotter]

Simplified matchers

[ccotter]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h

33–34

Ah, I happened to see ArgumentCommentCheck.h which seems to be lone check that uses bits...

[ccotter]

• Use bool
• Combine into a single matcher

[ccotter]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

104–126

Good call. Single matcher expr is cleaner.

[ccotter]

• Finish combining into a single matcher
• Rename option and add docs

[ccotter]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp

302

Thinking about this more, the enforcement spec for F.18 does apply to the SomeClass example (shown below again), since T is not a deduced parameter in the context of the constructor. Indeed, it has generated a bit of discussion which I think is the specific point of the guideline to avoid confusion around the non-trivial but important concepts of rvalue references, universal references, move/forwards, etc (and, again, I think one would usually expect to see different specializations for T vs T& and/or disallowing one of T/T& to avoid confusion to the reader or even author).

template <class T>
struct SomeClass {
    SomeClass(T&&) { ... } // T is not deduced.
};

I can add an option (although I'm a little worried now as there will be 3 options for this check, which seems like a bit much) for this with the default to adhere to the guideline as stated.

[ccotter]

• Include non-deduced template types

[ccotter]

Ok, all feedback should be addressed (thanks for the comment to consolidate into a single matcher) with appropriate options to relax the enforcement of the guideline.

[PiotrZSL]

Overall not bad, except reported things, I don't have any complains.
Number of options is not an issue.
90% of users wont use them, 10% will be happy to find them instead of dealing with NOLINT.

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

52

i thing you dont need here "hasDescendant(declRefExpr(equalsBoundNode("ref"))),"
simply because hasAncestor alone is doing a trick, if lambda is ancestor, then this call is a descendant for it.

57–58

consider:

hasType(type(rValueReferenceType())),
decl().bind("param"),
optionally(hasType(
                  qualType(references(templateTypeParmType(hasDeclaration(
                      templateTypeParmDecl().bind("template-type"))))))),

no need to duplicate parmVarDecl

62

put this unless before optionally...

65–75

looks like lambda context is visible as both operator() and as body to lambaExpr directly.
this mean that it may match twice, once as functionDecl, and once as lambdaExpr.
You can merge functionDecl with cxxConstructorDecl, just do same trick like with isMoveAssignmentOperator.

I would probably start with functionDecl, and then would try do some trick like forEachParam, but we dont have such matcher....
You missing also isDefinition() in functionDecl even that you got hasBody.

111

consider style:
if (MoveCall) return;

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst

25

maybe AllowPartialMove

[ccotter]

• More matcher simplification

[ccotter]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

65–75

I don't remember why I ever had lambda specific logic here, but it doesn't seem necessary.

[ccotter]

clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp

111

I left this as is as I recall seeing on other phabs a preference towards keeping the curly braces for expressions that (after taking into account line length style rules) span multiple lines. I think it helps my own readability in these situations.

[PiotrZSL]

One more thing I noticed, this check can be in conflict with performance-move-const-arg.

warning: std::move of the variable 'xyz' of the trivially-copyable type 'types::xyz' has no effect; remove std::move() [performance-move-const-arg]

so would be nice to make sure that trivially-copyable types are ignored., or add option to ignore them

[ccotter]

Trivial types should not be passed by rvalue reference, but by value per the diagram under http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing. I feel like adding an option to opt-out of this is missing the point of this check, and rvalue references of trivial type should just be fixed rather than adding an option to permit them.

[PiotrZSL]

In D141569#4147268, @ccotter wrote:

Trivial types should not be passed by rvalue reference, but by value per the diagram under http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing. I feel like adding an option to opt-out of this is missing the point of this check, and rvalue references of trivial type should just be fixed rather than adding an option to permit them.

Imaginate that such trivial type could be for example 200KB in size, copy to argument, and then from argument to for example class member could be costly. Using rvalue allows to pass non const object to some for example function.
I'm asking about option, just because other check also got such option, and if both will have one, then user can decide whatever he want to use std::move or not.
Still I agree that because we don't lose anything there, moving such trivial variable using std::move still would be fine, and at that point checking trivial variables in other check could be disabled, that's what I did in my project what I was fixing these issues.
So all is up to you.

If else, then maybe it would be good to deliver this check, if something happen then before end of release it still could be fixed.

[ccotter]

> Imaginate that such trivial type could be for example 200KB in size

This should be passed by const ref then correct (listed under "Expensive to move (e.g. big BigPOD[]" in the parameter passing guidelines) ? Are there cases where a big pod type needs to be passed by rvalue ref, without std::move where const ref cannot be used instead?

[ccotter]

• More clean option name

[ccotter]

Thanks for the name suggestion

[ccotter]

bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref.

[PiotrZSL]

In D141569#4163607, @ccotter wrote:

bump. I never heard back on the case where using an rvalue reference for a big pod type as opposed to const ref.

I have such use cases in project, they not necessary POD, but more like a way to wrap many arguments into single class.
Something like this:

struct A
{
    void call();  
};

struct Parameters
{
    A subclass;
    // here bunch of other members
};

void function(Parameters&& p)
{
    p.subclass.call();
}

void test()
{
    A a;
    function({a});
}

When you change this into normal lvalue reference, then it wont compile, when you add const then it also wont compile.
In this cases I would probably used //NOLINT, so don't consider this case for this check as blocker.

For POD this issue also may happen but it's related more to the scenario when function modify input data by exploiting fact that input is not const.
In such case you may for example prepare message that would be created by using {}, then call function that would take it as rvalue, function could fill up some additional data like translation id, and send it.
By using rvalue you telling that function that it is an owner of this object now and can do anything with it, including changing it, but not necessarily need to move it.
But still this is also probably use case for NOLINT.

[PiotrZSL]

I was thinking... In theory fix for this check should be one of: use const &, use value, move rvalue.
But const & is not always in an option, in utils we got 2 functions: isOnlyUsedAsConst, isExpensiveToCopy.
So in theory if variable can be used as const, then const & can be used, if var cannot be used as const and is cheap to copy, then in theory value can be used, otherwise we deal with something intended (like partial move, or write to variable), and such cases could be +- ignored in relaxed mode.

Still, this is just something to thing, not necessarily to implement.

[PiotrZSL]

@ccotter Do we commit this, or you plan to still change something here ?

[ccotter]

Yes, I'm ready for this to be committed. I don't have commit access, but if anyone could commit this on my behalf with Chris Cotter <[e-mail]> that'd be great!

[PiotrZSL]

Rebase + Fix order of checks in ReleaseNotes

[ccotter]

Thanks!