Page MenuHomePhabricator

[clang-tidy] Add check 'misc-move-forwarding-reference'

Authored by mboehme on Jul 11 2016, 7:46 AM.



The check emits a warning if std::move() is applied to a forwarding reference, i.e. an rvalue reference of a function template argument type.

If a developer is unaware of the special rules for template argument deduction on forwarding references, it will seem reasonable to apply std::move() to the forwarding reference, in the same way that this would be done for a "normal" rvalue reference.

This has a consequence that is usually unwanted and possibly surprising: If the function that takes the forwarding reference as its parameter is called with an lvalue, that lvalue will be moved from (and hence placed into an indeterminate state) even though no std::move() was applied to the lvalue at the callsite.

As a fix, the check will suggest replacing the std::move() with a std::forward().

This patch requires D23004 to be submitted before it.

Diff Detail


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Prazek added inline comments.Jul 11 2016, 11:41 AM
71 ↗(On Diff #63513)

Use CPlusPlus11 here

19 ↗(On Diff #63513)

Please add doc comment.

29 ↗(On Diff #63513)

Very nice section! good idea.

So I have a thoughts about multiple sections in documentation (which is not a issue for you).
I think the check lists doc should not include sections - it doesn't look good and it also prevents people from using sections in docs.

Prazek added a project: Restricted Project.Jul 11 2016, 3:22 PM
mboehme updated this revision to Diff 63663.Jul 12 2016, 3:32 AM

Addressed Prazek's review comments.

mboehme marked 5 inline comments as done.Jul 12 2016, 3:35 AM

Nice check. This should be mentioned in docs/ReleaseNotes.rst


34 ↗(On Diff #63513)

I've talked to Alex -- using a Twine to avoid multiple allocations is the best I can do here.

29 ↗(On Diff #63513)

Do I understand correctly that you're not expecting me to change anything here?

Prazek added inline comments.Jul 16 2016, 9:31 AM
30 ↗(On Diff #63663)

Nothing from me. It's just general comment

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
20 ↗(On Diff #63663)

Might as well pass Context by const reference rather than pointer.

43–53 ↗(On Diff #63663)

I'm not certain I understand the benefit to this. We know it's a call to std::move() from the standard namespace already, so why do we care about the original text? I think it's reasonable to replace any call to move() from the standard namespace with ::std::forward(), so we should be able to remove the if statements and not have to go read the original source text from the source manager (which could involve, for instance, a query for that text over a slow network).

92 ↗(On Diff #63663)

It might be a bit more clear if you made isStdMove() into a local AST matcher and called it from here.

119 ↗(On Diff #63663)

Is there a reason why you can't just use CallMove->getExprLoc() instead of toying with character ranges?

alexfh added a subscriber: alexfh.Jul 25 2016, 6:49 AM
alexfh added inline comments.
36 ↗(On Diff #63663)

nit: The second llvm::Twine is not necessary.

43–53 ↗(On Diff #63663)

I think, the value of this is to maintain consistency with the existing code in terms of whether the std namespace should be globally qualified or not. Changing std::move to ::std::forward would sometimes be unwelcome, if the code doesn't use ::std otherwise.

60 ↗(On Diff #63663)

The variable name is rather uncommon for LLVM/Clang code (in particular, the The prefix). Maybe FuncTemplateDecl? Equally clear and somewhat shorter.

119 ↗(On Diff #63663)

I think, makeFileCharRange is needed. It helps finding the correct macro expansion level to insert the replacement on. In case the range is not on the same macro expansion level, the condition on the line 116 will stop the check from introducing changes to the code that are more likely to break it.

The only change I would suggest is to only disable the fix-it and not the whole diagnostic in case unsupported macro usage is detected.

thx for the check

25 ↗(On Diff #63663)

nit: reference?

43–53 ↗(On Diff #63663)

I agree with alex, it's better to keep code consistency (programmer intend).
But, at the same time, the check should be bomb proof for ugly cases like:

"std:: move"
 ":: std    ::    move",

For the moment, the code seems to propose a fix only when it's a known case,
and a warning is emitted in every cases.

92 ↗(On Diff #63663)

+1 I agree with Aaron here.

The match will also be more precise.

sbenza added inline comments.Jul 25 2016, 2:17 PM
20 ↗(On Diff #63663)

Function names start in lower case.

30 ↗(On Diff #63663)

Isn't this just CharSourceRange::getTokenRange(Callee->getLocStart(),Callee->getLocEnd()) ?

83 ↗(On Diff #63663)

Will this trigger on code like this:

template <typename T>
void F(T);

void G() { F<string&&>(std::move(str)); }
mboehme updated this revision to Diff 65484.Jul 26 2016, 3:13 AM
mboehme marked an inline comment as done.

Changes in response to reviewer comments

mboehme marked 13 inline comments as done.Jul 26 2016, 3:25 AM
mboehme added inline comments.
20 ↗(On Diff #63663)

Sorry -- still getting used to LLVM naming conventions.

30 ↗(On Diff #63663)

Thanks -- done.

43–53 ↗(On Diff #63663)

why do we care about the original text?

See alexfh's comment -- my intent is to perserve the existing style in terms of whether "std" is globally qualified or not.

83 ↗(On Diff #63663)

No, it won't -- I've added a test for this.

The reason it doesn't trigger is because the check only looks at the template itself, but not any instantiations (this is because the main matcher below matches for UnresolvedLookupExpr, which only exists in the template).

92 ↗(On Diff #63663)

That was my original plan, but there doesn't seem to be a way to do this with an AST matcher, and I'm not sure whether it's desired that this should change or what would be the best way to change it.

The main issue is that the decls() contained in a UnresolvedLookupExpr aren't children of the UnresolvedLookupExpr, and RecursiveASTVisitor doesn't contain any special-case logic to visit them either. (A consequence of this is that they don't show up when dumping the AST.) This means that it isn't possible to match for them using unresolvedLookupExpr(has(...)), for example.

This is the reason I went with an explicit test inside the check(). But if you have a suggestion on how this could be changed into an AST matcher in a way that's consistent with the overall design of AST matchers, I'd be open to that!

119 ↗(On Diff #63663)

Multiple changes here:

  • Following alexfh's suggestion, I only disable the fix-it and not the whole diagnostic for the macro constructs in question (see corresponding change in the test).
  • It turns out that once I do this, the isValid() check in replaceMoveWithForward() already does the job of avoiding making invalid replacements on macros, so I can do without it here and indeed simply use CallMove->getExprLoc(). (I've also added another macro test case for additional coverage.)
18 ↗(On Diff #65484)

Added numbering for consistency.

aaron.ballman added inline comments.
44–54 ↗(On Diff #65484)

I kind of thought that was going to be the case, but wanted to double-check. I agree with the notion, but disagree with the approach -- as @etienneb mentions, this code is not resilient enough to actually do the right thing. I don't think we should be going back to the original source text, but should instead be using the information the AST already tracks. This will perform better and do the right thing in all cases.

93 ↗(On Diff #65484)

I wonder if there's a reason for this behavior, or if it's simple a matter of not being needed previously and so it was never implemented. @sbenza or @klimek may know. I think we should be fixing the RecursiveASTVisitor, unless there is a valid reason not to (which there may be), though that would be a separate patch (and can happen after we land this one).

sbenza added inline comments.Jul 28 2016, 1:10 PM
93 ↗(On Diff #65484)

Even if the nodes are not visited through the recursive visitor, you can always have a matcher for it.
Eg: hasAnyConstructorInitializer / cxxCtorInitializer.

But what node are you trying to visit here?
The only node I see is NamingClass, which is not really a child node.
Like the referred Decl in a DeclRefExpr is not a child either. You can't use has() there, you have to use to().

mboehme updated this revision to Diff 66284.Aug 1 2016, 1:03 AM
mboehme marked 3 inline comments as done.
mboehme marked 7 inline comments as done.Aug 1 2016, 1:15 AM
mboehme added inline comments.
93 ↗(On Diff #65484)

I've now reworked this to use new matchers (in review in D23004). Thanks for the comments, which finally got me on the right track!

But what node are you trying to visit here?

I'm trying to visit the decls() in an OverloadExpr (this happened in isStdMove(), which is now deleted).

As you note, these intentionally aren't children of the OverloadExpr (so it doesn't make sense for the RecursiveASTMatcher to visit them), but it still makes sense to have a matchers for them -- which I've now added.

45–55 ↗(On Diff #66284)

I've changed the code to look at the NestedNameSpecifiers instead of looking at the original source text.

But, at the same time, the check should be bomb proof for ugly cases like [snip]

Apologies, I misunderstood -- I thought you were saying it should be bomb proof in the sense that it shouldn't suggest any replacement (and that's why I marked your comment done). Having read aaron.ballman's comment, I now realize what you meant is that the check should be bomb proof in the sense that it should create a correct fix even for such "ugly" cases.

mboehme updated this object.Aug 1 2016, 1:15 AM
mboehme updated this revision to Diff 66914.Aug 5 2016, 12:55 AM
mboehme marked 2 inline comments as done.

Renamed canReferToDecl to hasAnyDeclaration.

This is for consistency with the corresponding change in D23004.

Now that D23004 is submitted, can you take another look?

sbenza added inline comments.Aug 10 2016, 7:19 AM
64 ↗(On Diff #66914)

I'm guessing this is checking Lang==C++11, but we actually want Lang>=C++11, right?
I'm not sure if there is a way to check that.

49 ↗(On Diff #66914)

I don't understand what this is trying to test.
What is the overload resolution operator?
Also, these two cases look exactly the same as the previous two.

117 ↗(On Diff #66914)

This is missing one case for C++14's generic lambdas.

[&] (auto&& x) { y = std::move(x); }

It might already detect correctly, but it will need a special case for generating the std::forward
It might need to be something like:

[&] (auto&& x) { y = std::forward<decltype(x)>(x); }

We can also just leave a TODO for later.

aaron.ballman added inline comments.Aug 10 2016, 7:33 AM
64 ↗(On Diff #66914)

CPlusPlus11 will be true for C++ >= 11.

mboehme updated this revision to Diff 67828.Aug 12 2016, 6:31 AM

Handle case where the forwarding reference is a parameter of a generic lambda.

mboehme updated this revision to Diff 67829.Aug 12 2016, 6:34 AM
mboehme marked 2 inline comments as done.

Fix typo in comment

mboehme updated this revision to Diff 67830.Aug 12 2016, 6:38 AM

Restore spaces around scope resolution operator in test case

mboehme added inline comments.Aug 12 2016, 6:44 AM
50 ↗(On Diff #67830)

Sorry, that should have been "scope resolution operator". Comment changed.

What this is trying to test is that the replacements still happen as desired even if there are spaces around the scope resolution operator. Unfortunately, I inadvertently deleted those spaces when I clang-formatted this entire file... I've now restored the spaces.

118 ↗(On Diff #67830)

Good point. Done.

As it turns out, the detection didn't yet work quite correctly. This was due to the special format of LambdaExprs for generic lambdas. In particular, what tripped my test up is that the same TemplateTypeParmDecl is contained in two different FunctionTemplateDecls -- one for operator() and one for __invoke.

As a result, I've needed to rewrite the detection to do a little more work outside of the matcher. (There was no way to do it inside the matcher as the FunctionTemplateDecl for operator() is actually not visited by the RecursiveASTVisitor.)

aaron.ballman added inline comments.Aug 16 2016, 6:19 AM
46–56 ↗(On Diff #67830)

This code can be combined with the Global check below with something like:

Diag << ..., Twine(NNS->getPrefix()->getKind() == NestedNameSpecifier::Global ? "::" : "") + "std::" + ForwardName);
124–125 ↗(On Diff #67830)

Given that the user is possibly in a confused state when they wrote std::move() rather than std::forward(), I wonder if it makes more sense to word the diagnostic as an imperative rather than a question? The diagnostic as it is doesn't really explain why std::forward() would be right while std::move() could be wrong.

Perhaps: "forwarding reference passed to std::move() may unexpectedly leave lvalue references in an indeterminate state; use std::forward() instead for perfect forwarding"

27 ↗(On Diff #67830)

"If" should not be capitalized here.

78 ↗(On Diff #67830)

Uncertain whether we're doing this or not, but should we keep this list alphabetized?

32 ↗(On Diff #67830)

"is often written in the" -> "is sometimes written with the"

mboehme updated this revision to Diff 68322.Aug 17 2016, 1:52 AM

Reponses to reviewer comments

mboehme marked 2 inline comments as done.Aug 17 2016, 2:07 AM
mboehme added inline comments.
46–56 ↗(On Diff #67830)

It could -- but note that there is a third case that also needs to be handled here, namely that NNS has a prefix, but it's something other than "::". In other words, the code would need to go something like this:

if (!NNS->getPrefix() || NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) {
  Diag << ..., Twine(NNS->getPrefix() ? "::" : "") + "std::" + ForwardName);

This repeats part of the check, and I feel it's more opaque than explicitly spelling out the two cases I'm interested in, at the cost of a little duplication.

124–125 ↗(On Diff #67830)

I wonder if it makes more sense to word the diagnostic as an imperative rather than a question?

Good point -- done.

I've reworded your suggestion a bit:

  • I've phrased this simply as "may unexpectedly cause lvalues to be moved". That's shorter and, I'm hoping, more accessible than talking about "indeterminate state".
  • I've left out the reference to "perfect forwarding". If someone triggers this warning, chances are they won't know what it is anyway. They still have everything they need to find out about perfect forwarding; if they do a search for std::forward(), they should find relevant material.

What do you think?

78 ↗(On Diff #67830)

I think it's a good idea -- done!

mboehme marked an inline comment as done.Aug 26 2016, 12:12 AM

Any more comments here?

aaron.ballman accepted this revision.Aug 29 2016, 5:45 AM
aaron.ballman edited edge metadata.

One minor typo fix, but otherwise LGTM. Thank you for the check!

125–126 ↗(On Diff #68322)

Anyone understanding what an lvalue is will probably also understand indeterminate state. ;-) However, I think the new wording is fine.

31 ↗(On Diff #68322)

s/callsite/call site

This revision is now accepted and ready to land.Aug 29 2016, 5:45 AM
mboehme updated this revision to Diff 69662.Aug 30 2016, 3:43 AM
mboehme edited edge metadata.

Fix typo

mboehme marked an inline comment as done.Aug 30 2016, 3:45 AM
mboehme updated this revision to Diff 69665.Aug 30 2016, 5:16 AM

Update to current head

This revision was automatically updated to reflect the committed changes.