This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix wrong FixIt in performance-move-const-arg
ClosedPublic

Authored by Sockke on Aug 4 2021, 5:19 AM.

Details

Summary

There are incorrect Fixit and missing warnings:
case :
A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.

void showInt(int&&) {}
void testInt() {
    int a = 10;
    // expect: warning + nofix
    showInt(std::move(a));  // showInt(a) <--- wrong fix
}

struct Tmp {};
void showTmp(Tmp&&) {}
void testTmp() {
    Tmp t;
    // expect: warning + nofix
    showTmp(std::move(t));  // showTmp(t) <--- wrong fix
}

Diff Detail

Event Timeline

Sockke created this revision.Aug 4 2021, 5:19 AM
Sockke requested review of this revision.Aug 4 2021, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 5:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Sockke updated this revision to Diff 364404.Aug 5 2021, 3:48 AM

format.

Quuxplusone requested changes to this revision.Aug 5 2021, 5:53 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
75

You're right that some diagnostic should be given here, but this is not the right diagnostic. The issue is not that the std::move is non-functional, but rather that it's superfluous; a move will happen, with or without the std::move. (And the std::move prevents move elision, so removing it may actually improve codegen.)

257

This warning is incorrect, as indicated in your PR summary. Isn't the point of this PR mainly to remove this incorrect warning?

This revision now requires changes to proceed.Aug 5 2021, 5:53 AM
MTC added inline comments.Aug 5 2021, 6:29 AM
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
75

Correct! @Sockke would you self-examination these cases again? I have checked this case, see https://godbolt.org/z/3ch3sbG17.

257

I guess what @Sockke wants to express is that applying std::move on the integer type makes no sense.

whisperity added inline comments.Aug 5 2021, 6:44 AM
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
51

I am not sure if ignoringImplicit(ignoringParenCasts(_)) is the same as ignoringParenImpCasts(_), but I've seen the latter one appear many times in other matchers.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

Yes, but there is no FixIt for this warning, but even if the user fixes this manually according to the warning, it'll result in

error: cannot bind rvalue reference of type int&& to lvalue of type int

because showInt takes int&&!

So this is not just unfixable automatically (hence no CHECK-FIXES line), it isn't fixable at all.

Quuxplusone added inline comments.Aug 5 2021, 6:51 AM
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

At a really high level, yeah, this code is bad code. :) But this is exactly Sockke's "case 1":

A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.

So a warning here would have to be, like, "std::move of a, of trivially copyable type int, has no runtime effect; consider changing showInt's parameter from int&& to int."
Also, should add a test case of the form

void showInt(int&&);
template<class T> void forwardToShowInt(T&& t) { showInt(static_cast<T&&>(t)); }
void test() {
    int a = 10;
    forwardToShowInt(std::move(a));
}

to make sure the diagnostic (or lack thereof) plays well with templates. Here, refactoring forwardToShowInt's parameter from T&& to T is unlikely to be a realistic option.

whisperity added inline comments.Aug 5 2021, 6:56 AM
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

Yes, I was thinking the same, maybe it could warn about the function (in a note or something). But then we reach the point that the check's name is fuzzy at best... (It has an already outdated name, though.)

Sockke added a comment.Aug 5 2021, 9:10 AM

Thank you for your reviews, @Quuxplusone, @whisperity, @MTC. I was also considering adding new diagnoses to the current check for these special cases, but I am not sure how to classify and describe them correctly, and whether they need to be added to the new check.
First of all, I want to make sure that the timing of warning and FxiIt in these cases is correct? If the above is no problem, I think I need to classify and modify the diagnostic information in the next step, is that right?

Sockke updated this revision to Diff 365455.Aug 10 2021, 6:09 AM
MTC added inline comments.Aug 10 2021, 6:33 AM
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

Change 'int'&& -> 'int&&' and 'int&' -> int.

Make consider changing showInt's parameter from 'int'&& to 'int'& as a note instead of a warning. And I don't have a strong preference for the position of the note, but I personally want to put it in the source location of the function definition. and

I'm not at all convinced that any of this complexity is worth it. Have you run this check on any large codebase to see how many false positives it has? Does it catch any true positives?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
280

Here, the more immediate red flag is that the programmer is using std::move on an rvalue expression.

std::string&& a();
std::string& b();
std::string c();
auto aa = std::move(a());  // not great, moving an xvalue
auto bb = std::move(b());  // OK, moving an lvalue
auto cc = std::move(c());  // not great, moving a prvalue

However, the more complicated and guess-worky these diagnostics get, the more I want to see how they interact with templates and generic code.

template<class F>
auto w(F f) {
    return std::move(f());
}
std::string callw1() { std::string s; return w([&]() -> std::string { return s; });
std::string callw2() { std::string s; return w([&]() -> std::string& { return s; });

callw1 ends up applying std::move to a prvalue inside w, but when callw2 calls w, the std::move actually does something. Possible strategies here include "Warn on code that's only sometimes correct," "Don't warn on code that's only sometimes wrong," and "Don't implement the diagnostic at all because it's too icky."

283

This suggestion seems bad. If Tmp&& was originally a typo, then the machine can't know what the programmer might have meant. If Tmp&& was trying to indicate "take ownership of," then the right fix would probably be to pass Tmp by value.

MTC added a comment.Aug 11 2021, 12:28 AM

I think we're a bit off track, and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

  1. Fix the wrong AutoFix which blocks the compilation.
  2. Find more 'unrecommended' std::move usage and give correct warning messages.
  3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check should be consistent with -Wpessimizing-move, see https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
280

Agree! For Tmp t2 = std::move(Tmp());, what should be reported here is moving a prvalue. However, the original behavior is emitting this message.

And the original matcher will ignore template instantiation, see unless(isInTemplateInstantiation()).

Sockke marked 8 inline comments as done.Aug 12 2021, 5:26 AM

I think we're a bit off track, and @Sockke wants to accomplish more than one goal in the same patch. I have summarized what we are currently discussing as follow shows:

  1. Fix the wrong AutoFix which blocks the compilation.
  2. Find more 'unrecommended' std::move usage and give correct warning messages.
  3. Whether template should be taken into account.

In addition, I would like to mention that we need to ensure that this check should be consistent with -Wpessimizing-move, see https://reviews.llvm.org/D7633, which has done the perfect job.

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

Yes, I originally wanted to make some improvements on the premise of keeping the old version of the idea. As Quuxplusone said, this improvement still does not meet the final requirements, which may require rewriting the entire checker. Any other thoughts? @Quuxplusone,@aaron.ballman,@whisperity,@alexfh

I suggest that this patch be divided into two patches. In the current patch, fix the wrong AutoFix. What the current check should look like is left in the second patch for discussion. @Sockke do you mind simplifying this patch and only achieving the first goal?

FWIW, I agree. The original "case 1" was fixing wrong behavior; the original "case 2" was adding a new feature. Let's see just the "case 1" bugfix (and regression test), and then once that's being properly handled and regression-tested, it'll be safer to mess around with new feature work.

Sockke updated this revision to Diff 366225.Aug 13 2021, 2:28 AM
Sockke retitled this revision from [clang-tidy] Fix wrong and missing warnings in performance-move-const-arg to [clang-tidy] Fix wrong FIxIt in performance-move-const-arg.
Sockke edited the summary of this revision. (Show Details)

Fix the wrong AutoFix which blocks the compilation.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
263

forwardToShowInt takes T&&, not int&&, and so it can't be changed in the way the diagnostic is suggesting. I think the right answer here is not to emit the diagnostic at all, when the offending function is a template.

(Relatively minor nits, all moot because this diagnostic should not be emitted at all: 'int'&& should be 'int&&', trivially copyable should not be hyphenated, and int& is a strictly worse suggestion than either const int& or plain int. IMVHO it would be reasonable to land a very trivial patch to remove the hyphen from trivially copyable and update the tests, and then rebase this PR on top of that.)

271

This is precisely the bogus message you're trying to eliminate, isn't it? Removing std::move would make the code fail to compile because t is an lvalue but showTmp requires an rvalue.

Sockke updated this revision to Diff 366815.Aug 17 2021, 12:06 AM

update! Fixed some diagnostic descriptions.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
263

Okay, I have no complaints about the diagnostics being tested by these tests anymore.
I am puzzled about why the tool is suggesting to rewrite int&& as int, but not suggesting to rewrite Tmp&& as Tmp; those cases should be exactly isomorphic, right?
But the goal of this PR is now to eliminate wrong fixits, and I believe you've succeeded at that.
Btw, I'm not qualified to review the actual code change in MoveConstArgCheck.cpp; I'll let someone else do that.

Sockke updated this revision to Diff 367474.Aug 19 2021, 6:07 AM
Sockke marked 2 inline comments as done.

Thanks for your reply @Quuxplusone. I have updated!

This comment was removed by Sockke.
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
263

Keep the original diagnostic behavior, but without "remove std::move" message.

263

Thank you for your reply! Yes, they should be exactly isomorphic when Tmp is a trivially copyable type.

Any thoughts? : )

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
262–263
forwardToShowInt(std::move(a));
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect

I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

Which do you prefer? @Quuxplusone

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
262–263
forwardToShowInt(std::move(a));
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect

I continue to want-not-to-block this PR, since I think it's improving the situation. However, FWIW...
It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all? If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

Yes, I agree with you. I did not remove warning to maintain the original behavior, which will make the current patch clearer. In any case, it is easy for me to make this modification if you want.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
262–263

I'll defer on this subject to whomever you get to review the code change in MoveConstArgCheck.cpp. (@whisperity perhaps?)

I'm a bit confused about this, but it has been a while since I read this patch. Am I right to think that the code in the patch and the original submission (the patch summary) diverged since the review started? I do not see anything "removed" from the test files, only a new addition. Or did you accidentally upload a wrong diff somewhere? The original intention of the patch was to remove bogus printouts.

In general: the test file ends without any testing for functions with multiple parameters. Are they out-of-scope by design?

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
107–137
108–136

+? Shouldn't this be ||? I know they're equivalent over Z_2, but if we're using && instead of * then let's adhere to the style.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

but I personally want to put it in the source location of the function definition

(I think you submitted the comment too early, @MTC, as there's an unterminated sentence there.)

But I agree with this. It should be put there when we're talking about the function's signature. The reason being the developer reading the warning could dismiss a potential false positive earlier if they can also immediately read (at least a part of...) the function's signature.

@Sockke you can do this by doing another diag() call with the Note value given as the 3rd parameter. And you can specify the SourceLocation of the affected parameter for this note.

262–263

(Negative, it would be @aaron.ballman or @alexfh or the other maintainers. I'm just trying to give back to the community after taking away a lot of reviewer effort with a humongous check that I just recently pushed in after years of (re-)development.)

262–263

It's good that this message doesn't contain a fixit, since there's nothing the programmer can really do to "fix" this call. But then, should this message be emitted at all?

As an individual not representing but myself, I would say no. At least not in a check which is not sold as a style-like or guideline-enforcement analysis implementation.

If this were clang -Wall, then we definitely would not want to emit a "noisy" warning where there's basically nothing the programmer can do about it. Does clang-tidy have a similar philosophy? Or is it okay for clang-tidy to say "This code looks wonky" even when there's no obvious way to fix it?

I think in general tools should have this policy. 😇

267–270

Is there a test case covering if there are separate invocations of the function? Just to make sure that the consider changing the parameter suggestion won't become an annoyance either, and result in conflicts with other parts of the code.

271

We could ensure that the symbol name is printed the same way as the other placeholder-injected named decls.

Sockke marked an inline comment as not done.Aug 31 2021, 5:38 AM

Thanks for your review, I greatly appreciate it. @whisperity
There is no wrong case in the original test file, which is why we did not catch the bug in the test suite. From this, I added some new cases.
I tested the functions with multi-parameters, there is no problem, I will add them to the test file.

I'm a bit confused about this, but it has been a while since I read this patch. Am I right to think that the code in the patch and the original submission (the patch summary) diverged since the review started? I do not see anything "removed" from the test files, only a new addition. Or did you accidentally upload a wrong diff somewhere? The original intention of the patch was to remove bogus printouts.

In general: the test file ends without any testing for functions with multiple parameters. Are they out-of-scope by design?

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
107–137
108–136

+? Shouldn't this be ||? I know they're equivalent over Z_2, but if we're using && instead of * then let's adhere to the style.

The reason for this is that the maximum capacity of diag is 10.

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
257

but I personally want to put it in the source location of the function definition

(I think you submitted the comment too early, @MTC, as there's an unterminated sentence there.)

But I agree with this. It should be put there when we're talking about the function's signature. The reason being the developer reading the warning could dismiss a potential false positive earlier if they can also immediately read (at least a part of...) the function's signature.

@Sockke you can do this by doing another diag() call with the Note value given as the 3rd parameter. And you can specify the SourceLocation of the affected parameter for this note.

Yes, it is reasonable to add a note, but would you mind adding this modification to the patch that fixes AutoFix? If you don't mind, I will update it.

267–270

The warning will only be given when calling this function and passing in the parameters wrapped with std::move. Is my understanding correct?

271

Yes, this is a better implementation.

kindly ping.

Hi, Could you please take some time to review this diff again? @whisperity

whisperity added inline comments.Sep 29 2021, 2:16 AM
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
254

From int &? Isn't the parameter int && there?

257

I think it is okay if the additional note for this is added in this patch, there is no need for a separate patch on that. It helps understand WHY the FixIt is wrong or not wrong, anyway.

267–270

Yes, that is what I mean. But I would like to see a test case where the "target function" is called multiple times with different arguments. One call with std::move(x), one with a direct temporary, etc. So we can make sure that if a function is called more than 1 time, we can still be sure there won't be bad warnings or bad fixits that contradict each other.

Sockke updated this revision to Diff 378168.Oct 8 2021, 4:50 AM
Sockke retitled this revision from [clang-tidy] Fix wrong FIxIt in performance-move-const-arg to [clang-tidy] Fix wrong FixIt in performance-move-const-arg.

Sorry for the delayed reply because of National Day. I have updated. @whisperity

Sockke marked 12 inline comments as done.Oct 13 2021, 9:00 AM

Kindly ping.

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman

whisperity added inline comments.Oct 26 2021, 5:50 AM
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
170
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
260

I think this is becoming okay and looks safe enough. I can't really grasp what HasCheckedMoveSet means, and how it synergises with storing CallExprs so maybe a better name should be added. Do you mean AlreadyCheckedMoves? When is it possible that the same CallExpr of std::move will be matched multiple times?

In general going from T&& to const T& would be a broken change if the function implementation calls a non-const member method on the T instance, but I can be convinced that saying consider [...] instead of generating a bogus fix-it is enough, there is hopefully no need to extensively analyse and guard against this.

There has been a new release since the patch was proposed so a rebase should be needed.
@aaron.ballman What do you think, does this warrant a ReleaseNotes entry?

There has been a new release since the patch was proposed so a rebase should be needed.
@aaron.ballman What do you think, does this warrant a ReleaseNotes entry?

I think a release note for this would be helpful, yes!

Sockke marked 2 inline comments as done.Oct 27 2021, 6:01 AM

I think this is becoming okay and looks safe enough. I can't really grasp what HasCheckedMoveSet means, and how it synergises with storing CallExprs so maybe a better name should be added. Do you mean AlreadyCheckedMoves? When is it possible that the same CallExpr of std::move will be matched multiple times?

Yes, you're right!

Okay. I think I am convinced. And removing a bogus automated fix is always a positive change!

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
174

I am not sure if this will always return something reasonable as a printed name.

Could you please add a test case where the receiving function is a constructor? E.g. a testStringView that takes, let's say, const char*&&?

clang-tools-extra/docs/ReleaseNotes.rst
116–119 ↗(On Diff #382632)

I tried to make the release note entry a bit easier to understand. Of course, please reflow!

Sockke updated this revision to Diff 384706.Nov 4 2021, 5:03 AM

I improved the logic of judgments needing to report suggesting notes in some cases. And more tests were added here.

Sockke updated this revision to Diff 384728.Nov 4 2021, 6:28 AM

In addition, the splicing logics of "FunctionName" and "ExpectParmTypeName" were improved.

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman

This generally looks fine for me, but I'd rather have other people who are more knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, or something like that. And I bet after such meetings everyone would take a few more days off from looking at C++ stuff.

This generally looks fine for me, but I'd rather have other people who are more knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, or something like that. And I bet after such meetings everyone would take a few more days off from looking at C++ stuff.

Ok, that sounds great, thanks a lot!

aaron.ballman added inline comments.Dec 9 2021, 11:55 AM
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
139–143
175

Not all CallExpr objects have a direct callee, so this will crash (such as calls through a function pointer rather than a direct function call). It may be worth adding test coverage for this.

179–183

Rather than doing this, you should be able to just pass the NamedDecl * in to the call to Diag below and it will be properly formatted and quoted.

184

Please spell the type out in the declaration.

clang-tools-extra/docs/ReleaseNotes.rst
118 ↗(On Diff #384728)

Can you re-flow this to 80 cols too?

clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
262–263

I'm just trying to give back to the community after taking away a lot of reviewer effort with a humongous check that I just recently pushed in after years of (re-)development.

And your review efforts are very much seen and appreciated, @whisperity! Thank you for the excellent help!

Sockke updated this revision to Diff 396385.Dec 28 2021, 3:41 AM

Sorry for the late reply. I made improvements to the patch in response to the questions you raised. @aaron.ballman

Sockke marked 13 inline comments as done.Dec 28 2021, 3:48 AM
Sockke added inline comments.
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
175

Use InvocationParm to determine whether objects have a direct callee.

aaron.ballman added inline comments.Jan 5 2022, 10:35 AM
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
170–172

It looks like ReceivingExpr can be null (see line 78 that you added), so the first dyn_cast will crash if given null. Further, if the result of the first dynamic cast is null, then the second dyn_cast will also crash.

Should there be a null pointer check? Should we use cast instead of dyn_cast for the first one?

174

getDirectCallee() can return null.

178–184

getDirectCallee() can return nullptr.

MTC added a comment.Jan 7 2022, 1:13 AM

This patch has become very complicated now. I summarized this patch and give a figure to illustrate what we have reached. And @Sockke please add some comments to explain the complex part or other means to make this patch more readable.

Sockke updated this revision to Diff 398077.Jan 7 2022, 1:16 AM
Sockke marked an inline comment as done.

Add some checks for null and comments for codes.

Sockke added inline comments.Jan 7 2022, 1:21 AM
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
154

ReceivingExpr is not null if IsRVRefParam is true.

158

Add a check for getDirectCallee().

168

I have added a check for getDirectCallee before here.

aaron.ballman accepted this revision.Jan 14 2022, 4:45 AM

LGTM! @Quuxplusone, you're still marked as requesting changes, are you okay with the way this has progressed?

Quuxplusone accepted this revision.Jan 14 2022, 7:23 AM

@Sockke: Throughout, trivially-copyable should be trivially copyable (two words).
Other than that, sure, I have no particular opinions at this point.

clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
172

Throughout, Supress should be Suppress.

clang-tools-extra/docs/ReleaseNotes.rst
118 ↗(On Diff #398077)
This revision is now accepted and ready to land.Jan 14 2022, 7:23 AM
Sockke updated this revision to Diff 401470.Jan 19 2022, 6:28 PM

Improve some description.

Do you need someone to commit on your behalf?

This revision was automatically updated to reflect the committed changes.

Do you need someone to commit on your behalf?

Thanks for your review, I committed it myself.