[clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.

[shuaiwang]

Summary

(Originally started as a clang-tidy check but there's already D45444 so shifted to just adding ExprMutationAnalyzer)

ExprMutationAnalyzer is a generally useful helper that can be used in different clang-tidy checks for checking whether a given expression is (potentially) mutated within a statement (typically the enclosing compound statement.)

This is a more general and more powerful/accurate version of isOnlyUsedAsConst, which is used in ForRangeCopyCheck, UnnecessaryCopyInitialization.

It should also be possible to construct checks like D45444 (suggest adding const to variable declaration) or https://bugs.llvm.org/show_bug.cgi?id=21981 (suggest adding const to member function) using this helper function.

This function is tested by itself and is intended to stay generally useful instead of tied to any particular check.

[Eugene.Zelenko]

See also D45444.

[JonasToth]

Hi @shuaiwang, i am currently working on the same check and we should definilty coordinate our work. :)

What do you think about joining the forces and to work together on such a check?

Things i would like to mention:

• i differentiate between values and handles. I want to be able to transform to const int * const if possible, similar for references
• i believe you do not test on all occasion, a variable can not be const, e.g. if it is casted, if it is catched by a lambda, array access, access through member functions and object members

Did you run the check over a codebase like llvm yet and what did you find?

[JonasToth]

clang-tidy/utils/ASTUtils.cpp

85 ↗

(On Diff #142588)

there is a util::isAssignmentOperator in clang-tidy already.

[shuaiwang]

Updated mostly isModified().
I'd like to mostly demonstrate that isModified() works given that there's https://reviews.llvm.org/D45444 and we'd like to merge.

[shuaiwang]

Change to just add a helper function isModified

[shuaiwang]

Hi @alexfh, @hokein, @JonasToth, I've updated this diff to be just adding the helper function isModified() with a set of dedicated unit tests. What do you think of the approach of getting this change committed and then @JonasToth can continue building D45444 by using isModified()?

[JonasToth]

Short note here too: I think having isModified return a track record, like a vector for each

1. modifications
2. non-const handle taking
3. (const usages)

would be nice. Every user can decide how to react to it. Then the function would be more like usageRecord with pointers to each usage.
That allows graph building, it allows detailed diagnostics and contains all relevant information for the expr.

Other interesting cases we need to consider here:

1. casts -> all kinds of casts forbid kinda forbid constness.
2. rvalue references, modification happens for std::move(v); std::forward(v); static_cast<T&&>(v)
3. ternary operator
4. lambdas with local usage only
5. lambdas that escape, e.g. int i; std::thread t([&](){ while(true) i++; }); Hard to analyze? I think we can consider all references into lambdas as escape

Maybe you can add an assertion about template types without concepts as a safeguard?

[JonasToth]

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

[Eugene.Zelenko]

In D45679#1069638, @JonasToth wrote:

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

But this is IR-level analysis. Clang has own include/clang/Analysis.

[shuaiwang]

In D45679#1069754, @Eugene.Zelenko wrote:

In D45679#1069638, @JonasToth wrote:

*hust* /llvm/tools/clang/lib/Analysis/PseudoConstantAnalysis.cpp

I will check this one first, before we get crazy and implemented something twice.

But this is IR-level analysis. Clang has own include/clang/Analysis.

I investigated a bit and it seems to be deadcode :(
It was once used by IdempotentOperationChecker which got deleted 4 years ago: http://llvm.org/viewvc/llvm-project?view=revision&revision=198476
I'll take a look and see if I can learn something from it anyway.

[shuaiwang]

Handle casts, ternary operator and lambdas.
Also optionally returns a chain of Stmt giving more details about how the conclusion is reached.

[shuaiwang]

Handle range-based for loop

[shuaiwang]

Better range for loop handling.

[JonasToth]

You are doing a great job and i learn new stuff :)

Inspired by the analysis tool in clangs repo:

• What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance
• Do we need to distinguish between Espaced and Modified? Having only two states will simplify some calculations (e.g. if (std::none_of(Expr, isModified) return EMK_Const)
• i think the multiple analysis sections could be functions on their own. If you create a class for the check, these should be private methods or helpers. At the moment, some sections are hard to understand and some simplifications could be made.
• what do you think creating a real ModificationReport that is stored per Expr? That can be helpful for a check like readability-complex-modification, dependency analysis and others. The Chain is in that direction, but not consistent from what i see. Storing this chain in the potential ModificationAnalyzer class would be superb.

clang-tidy/utils/ASTUtils.cpp

125 ↗

(On Diff #142882)

I am suprised that callExpr does not cover constructor calls. Or is there more?

149 ↗

(On Diff #142882)

Having the history for the trivial modifications would be nice, too.

I think treating all kinds of modifications is best.

160 ↗

(On Diff #142882)

Having such a lambda is somewhat weird and redundant, because it mimics the original function.

I think that should be refactored with a short discussion in the general comments if thats ok for you.

180 ↗

(On Diff #142882)

The implicit NotModified == 0 is hard to see.
Maybe a transition towards a std::any_of(Expr, isModified) is more readable. (see general comment)

188 ↗

(On Diff #142882)

Same + code duplication.

200 ↗

(On Diff #142882)

dito

220 ↗

(On Diff #142882)

This section of the code is hard to understand.

222 ↗

(On Diff #142882)

The pop is not clear to me

246 ↗

(On Diff #142882)

Code duplication for finding all declRefExpr to that expr.

252 ↗

(On Diff #142882)

same + code duplication

unittests/clang-tidy/IsModifiedTest.cpp

138 ↗

(On Diff #142882)

Could you please add tests for overloaded operators as free functions?

Arithmetic operators can usually be free functions and take by const& or &.

[Eugene.Zelenko]

In D45679#1071116, @JonasToth wrote:

You are doing a great job and i learn new stuff :)

• What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance

It may be reasonable to have variables/data members dependencies graph and mark them as constant/non constant.

[JonasToth]

I was thinking about that too.

• We can extract all DeclRefExpr for the modifiying expressions and

add dependencies.

Such analysis is definitly interesting, but should maybe be added later.
Having an internal Expr : Modified? mapping would suffice, too. The
isExprModified will then first check if it is calculated and start
calculation if not found.

Am 18.04.2018 um 19:32 schrieb Eugene Zelenko via Phabricator:

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45679#1071116, @JonasToth wrote:

You are doing a great job and i learn new stuff :)

• What do you think about having these functions in a class? Now, we need to recalculate and reanalyze the scope for every variable, multiple times (reference tracking). It would be nice to do it as lazy as possible and memorize the results. Especially addressing the use-case for the const-check, storing that a reference is not modified will save a lot of work = performance

It may be reasonable to have variables/data members dependencies graph and mark them as constant/non constant.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

[JonasToth]

clang-tidy/utils/ASTUtils.h

31 ↗

(On Diff #142882)

Maybe you could add an Unknown kind, e.g. if the expression is not found or similar.

[JonasToth]

Another note: https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/utils/DeclRefExprUtils.cpp
There is a isOnlyUsedAsConst with a slick implementation, using a set difference.

Edit: I used it to check, it has its shortcommings, too. It definitly fails for lambdas.

[JonasToth]

clang-tidy/utils/ASTUtils.cpp

226 ↗

(On Diff #142882)

What about transitive references and pointers?
It seems that only references are checked, but if a pointer is taken through that reference and vice versa, is that tracked correctly?

233 ↗

(On Diff #142882)

If the Exp is an array, that will not match array subscript?

Are there other similar cases?

[shuaiwang]

• Moved out of ASTUtils and becomes a separte class ExprMutationAnalyzer (bikeshedding on name a bit.)
• Reverted back to return bool instead of tri-valued enum, the meanings of Escaped and Modified are not well defined.
• Cosmetic changes like spliting large function and reducing duplicates.
• Addressed comments.

[shuaiwang]

Regarding full dependency analysis, I think it's out of the scope of this change at least for now. We're only trying to find one possible point where the given Expr is (assumed to be) mutated. This is known to be useful at this point for use cases like "check whether const can be added" or "check whether a by-value copy can be replace with a const-by-ref capture". The analysis is also designed to be performed within a limited scope because we're mostly targeting code-style issues (for example, even if we can do the analysis across multiple functions within the same TU, it might still not be a good idea because at that point it's much less obvious to human readers.) Figuring out the full dependency will be more costly than what we're doing right now and would be overkill for the applications we'd like to apply to.

For isOnlyUsedAsConst, I'm intending to replace that with this (ref one of my oldest comment.)

clang-tidy/utils/ASTUtils.cpp

125 ↗

(On Diff #142882)

Added test cases for this.
cxxConstructExpr is the only one I can think of that happens to not be covered by callExpr.

149 ↗

(On Diff #142882)

Could you elaborate what do you mean here?

222 ↗

(On Diff #142882)

Removed.

In case you wonder, I was intended to put a bit more information into Chain and here we're basically trying each element of LoopVars and see whether it's modified or not, because each recursive call appends to Chain we need to push the LoopVarStmt first before doing the recursive call, but if the recursive call concluded that the var is not modified, we need to pop the LoopVarStmt back out.
Anyway this is not removed.

226 ↗

(On Diff #142882)

Yes.

As soon as an address is taken we assume it's modified. Currently we don't follow pointers.
Examples:
int a = 10;
&a; <-- taking address, assumed modification point

int b = 10;
int& c = b; <-- follow the reference
&c; <-- taking address, assumed modification point, propagates back to "b"

233 ↗

(On Diff #142882)

If Exp is an array, we should first follow the array subscript expression and do a recursive call.

i.e.:
int x[2];
int& y = x[0];
y = 10;
isModified(x) -> isModified(x[0]) -> isModified(y) { return true /* because y = 10 */ }

clang-tidy/utils/ASTUtils.h

31 ↗

(On Diff #142882)

Reverted back to just bool.

[JonasToth]

I like your refactoring very much and i think we have a state that is close to commit.

My clang-tidy check is already based on top of your functionality, but i can not work on it this weekend and next week is already busy for me. I decided to analysis values and references only for now, and work on pointer semantics later, because of some uncertainty how to handle different things.

Right now, nothing comes to my mind that might be missing. Maybe you could add small helpers and utilities, that take a varDecl and run the analysis (implemented in my check). That would move the last piece into this section :)

clang-tidy/utils/ASTUtils.cpp

125 ↗

(On Diff #142882)

Are move and copy constructor covered by this?

149 ↗

(On Diff #142882)

I think i was mostly irritated and it is irrelevant with the new semantics (which i think are good for now!).

222 ↗

(On Diff #142882)

Ok.

233 ↗

(On Diff #142882)

I was wondering about the ternaryOperator and if some suprises might arise from it.

Thats something i will investigate next week within the clang-tidy check.

[JonasToth]

Something came to my mind:

conversion operators. Do they behave as normal overloaded operators? The tests should reflect both a const conversion and a modifying one.

Edit: Already covered

[JonasToth]

There are false positives.

void false_positive() {
// FIXME False positive
int np_local0 = 42;               
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'int' can be declared const
const int &r0_np_local0 = np_local0;
int &r1_np_local0 = np_local0;
r1_np_local0 = 43;
}

// FIXME false positive
int np_local0 = 42;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'int' can be declared const
const int *const p0_np_local0 = &np_local0;
int *const p1_np_local0 = &np_local0;
*p1_np_local0 = 43;

// FIXME False positive
ConstNonConstClass np_local0;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'ConstNonConstClass' can be declared const

np_local0.constMethod();
np_local0.nonConstMethod();

// FIXME array access was modifying
ConstNonConstClass np_local0[2];
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'ConstNonConstClass [2]' can be declared const
np_local0[0].constMethod();
np_local0[1].constMethod();
np_local0[1].nonConstMethod();

// FIXME false positive
int np_local5[3] = {1, 2, 3};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local5' of type 'int [3]' can be declared const [cppcoreguidelines-const]
int &np_local6 = np_local5[1] < np_local5[2] ? np_local5[0] : np_local5[2];
np_local6 = 42;

// FIXME false positive
int np_local7[3] = {1, 2, 3};
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local7' of type 'int [3]' can be declared const [cppcoreguidelines-const]
int *np_local8 = np_local7[1] < np_local7[2] ? &np_local7[0] : &np_local7[2];
*np_local8 = 42;

[shuaiwang]

Added an isMutated overload for Decl.
A few more test cases.

[JonasToth]

I migrated to the new Decl interface and adjusted my tests, there are no false positives left and I am not aware of more possible code constellation that would require testing.
If you have time you could check my tests, maybe you find something I missed.

Otherwise this is LGTM from me, but the reviewers must of course accept it (@aaron.ballman, @alexfh)

[aaron.ballman]

clang-tidy/utils/ExprMutationAnalyzer.cpp

43

Can elide the braces.

72

Don't use auto here.

83–84

Why does this not return the result of findMutation() like the other call above?

89

Should this also consider a DeclRefExpr to a volatile-qualified variable as a direct mutation?

What about using Expr::HasSideEffect()?

[shuaiwang]

Addressed review comments, notably added check for DeclRefExpr to volatile variables.

clang-tidy/utils/ExprMutationAnalyzer.cpp

83–84

find*Mutation is intended to return where the given Expr is mutated.
To make it a bit more useful, for Decl's I'm returning the DeclRefExpr and caller can chose to follow it to find where the DeclRefExpr is mutated if needed.
As a concrete example:

struct A { int x; };
struct B { A a; };
struct C { B b; };
C c;
C& c2 = c;
c2.b.a.x = 10;

If we start with DeclRefExpr to c we'll find mutation at a DeclRefExpr to c2, following that we can find the mutation Stmt being c2.b.a.x = 10.
Returning E here makes it possible to reveal intermediate checkpoint instead of directly saying c is mutated at c2.b.a.x = 10 which might be confusing.

89

Good catch about DeclRefExpr to volatile.

HasSideEffects means something different. Here find*Mutation means find a Stmt in ancestors that mutates the given Expr. HasSideEffects IIUC means whether anything is mutated by the Expr but doesn't care about what exactly is mutated.

[JonasToth]

clang-tidy/utils/ExprMutationAnalyzer.cpp

89

May I ask the exact semantics for volatile? I would add the test to my check, too.

It is possible to write const volatile int m = 42;, but if i understand correctly m could still change by hardware, or would this be UB?
If the const is missing, one must always assume external modification, right?

[aaron.ballman]

clang-tidy/utils/ExprMutationAnalyzer.cpp

89

HasSideEffects means something different. Here find*Mutation means find a Stmt in ancestors that mutates the given Expr. HasSideEffects IIUC means whether anything is mutated by the Expr but doesn't care about what exactly is mutated.

What I am getting at is that the code in findDirectMutation() seems to go through a lot of effort looking for expressions that have side effects and I was wondering if we could leverage that call instead of duplicating the effort.

89

May I ask the exact semantics for volatile? I would add the test to my check, too.

Basically, volatile objects can change in ways unknown to the implementation, so a volatile read must always perform an actual read (because the object's value may have changed since the last read) and a volatile write must always be performed as well (because writing a value may have further side effects if the object is backed by some piece of interesting hardware).

It is possible to write const volatile int m = 42;, but if i understand correctly m could still change by hardware, or would this be UB?

That could still be changed by hardware, and it would not be UB. For instance, the variable might be referencing some hardware sensor and so you can only read the values in (hence it's const) but the values may be changed out from under you (hence, it's not constant).

If the const is missing, one must always assume external modification, right?

You have to assume external modification regardless.

[shuaiwang]

Revert "DeclRefExpr to volatile handling" part of last update.

[shuaiwang]

I've reverted the handling of DeclRefExpr to volatile after rethinking about it.

The purpose of this analyzer is to find whether the given Expr is mutated within a scope, but doesn't really care about external changes. In other words we're trying to find mutations explicitly written within the specified scope.

clang-tidy/utils/ExprMutationAnalyzer.cpp

89

Unfortunately I don't think HasSideEffects help in here. We're finding one particular side effect that is mutating the specified Expr so we need to have explicitly equalsNode(Exp) for all the matches. HasSideEffects can simply claim f(a, b) or even g() has side effects (given void f(Foo&, const Bar&), void g()) while we need to require a link to the specific Expr we're analyzing (i.e. f(a, b) is mutating a but not b)

[aaron.ballman]

In D45679#1088696, @shuaiwang wrote:

I've reverted the handling of DeclRefExpr to volatile after rethinking about it.

The purpose of this analyzer is to find whether the given Expr is mutated within a scope, but doesn't really care about external changes. In other words we're trying to find mutations explicitly written within the specified scope.

Okay, that seems like a good reason to ignore volatile qualifiers. Thank you for the explanation!

clang-tidy/utils/ExprMutationAnalyzer.cpp

89

Ah, okay, thank you for the explanation.

I think you may want to look at the implementation for HasSideEffect() to identify other cases you may want to support. For instance, adding tests for statement expressions, paren expressions, bizarre things like VLA mutations in otherwise unevaluated contexts, etc.

[shuaiwang]

Handle unevaluated expressions.

[aaron.ballman]

clang-tidy/utils/ExprMutationAnalyzer.cpp

22–26

This can be replaced with return llvm::find(Node.capture_inits(), E);

35–39

I think these should be exposed as public AST matchers, as they're both generally useful. It can be done in a follow-up patch, or done before landing this patch, either is fine by me.

68

What about other unevaluated expressions, like typeof, _Generic, etc? You should search for ExpressionEvaluationContext::Unevaluated in Clang to see where we set up an unevaluated expression evaluation context to find all of the situations.

73

What is this trying to match with the typeLoc() matcher?

75–76

I think these are only approximations to testing whether it's unevaluated. For instance, won't this match these expressions, despite them being evaluated?

// C++
#include <typeinfo>

struct B {
  virtual ~B() = default;
};

struct D : B {};

B& get() {
  static B *b = new D;
  return *b;
}

void f() {
  (void)typeid(get()); // Evaluated, calls get()
}

/* C99 and later */
#include <stddef.h>

void f(size_t n) {
  (void)sizeof(int[++n]); // Evaluated, increments n
}

[shuaiwang]

Handle sizeof on VLA.
Added test case for typeof()
Added TODO for handling typeid and generic selection.

clang-tidy/utils/ExprMutationAnalyzer.cpp

35–39

Will leave as follow up patch.

68

I checked around and I think these are all we care about:

• decltype/typeof
• sizeof/alignof
• noexcept
• _Generic
• typeid

I've added TODO for _Generic and typeid for now because I think those need a bit more work to implement, while at the same time not supporting them for now only creates false positives from isMutated which is what we prefer over false negatives.

73

Added comment to explain.

75–76

Fixed handling of sizeof(VLA)
Added TODO for typeid

[shuaiwang]

Handle typeid and generic selection.

[shuaiwang]

Overestimated the work of supporting typeid and _Generic, done now.

[aaron.ballman]

Aside from a minor commenting nit, this LGTM.

clang-tidy/utils/ExprMutationAnalyzer.cpp

79–82

I think this comment is stale now.

[shuaiwang]

Remove stale comment

[shuaiwang]

Thanks a lot for the review!

Could you also help commit this diff as well? Thanks!

[JonasToth]

@shuaiwang I can commit it in 2 hours for you if you want, after my lecture.

[JonasToth]

Hmm. SVN does not want  me to commit anything :/

I will retry later today, but maybe i cant commit.

Best, Jonas

Am 06.06.2018 um 10:47 schrieb Aaron Ballman:

[lebedev.ri]

In D45679#1122826, @shuaiwang wrote:

Thanks a lot for the review!

Could you also help commit this diff as well? Thanks!

There are at least two previous accepted&committed differentials (D17586, D45702),
i think you can ask for commit bit now.

[aaron.ballman]

I've commit in r334604 with one minor change -- I added an include for <cctype> to the unit test so that std::isspace() would compile properly on all platforms.

[aaron.ballman]

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

[shuaiwang]

Don't include <typeinfo> in unit tests, should fix the test failures.

[shuaiwang]

Add include <cctype> for std::isspace() (thanks aaron.ballman!)

[shuaiwang]

In D45679#1131115, @aaron.ballman wrote:

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

Apparently I can't include <typeinfo> in unit test cases (somehow that works on my machine :p)
Changed to just forward declare std::type_info instead.

[JonasToth]

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Am 13.06.2018 um 23:08 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1131115, @aaron.ballman wrote:

I had to revert due to failing tests. The revert was done in r334606 and this is an example of a failing bot: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/31500

Apparently I can't include <typeinfo> in unit test cases (somehow that works on my machine :p)
Changed to just forward declare std::type_info instead.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

[alexfh]

In D45679#1132086, @JonasToth wrote:

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Tests should be self-contained and must not depend on any system headers. Standard library implementations may differ, which would make tests brittle. Instead, tests should mock implementations of the APIs they work with (if necessary, multiple times - once for every distinct standard library version the check supports).

[shuaiwang]

Could someone help commit this now that the build issue with header include is fixed? Thanks a lot!
(meanwhile I'm requesting commit access)

In D45679#1132327, @alexfh wrote:

In D45679#1132086, @JonasToth wrote:

It might be the case, that the test is run with -no-stdinc (or similar),
so the standard library is not available.

Tests should be self-contained and must not depend on any system headers. Standard library implementations may differ, which would make tests brittle. Instead, tests should mock implementations of the APIs they work with (if necessary, multiple times - once for every distinct standard library version the check supports).

Yep. It doesn't include any headers anymore :)

[alexfh]

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

583

FYI, this test had to be fixed for a ps4 buildbot. See r335799 for details.

[george.karpenkov]

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

[shuaiwang]

In D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

I think it should in theory be possible, though I need some advice about where exactly is the best place in clang.

[JonasToth]

I think clang/lib/Analysis is a good place.

I think it should in theory be possible, though I need some advice about where exactly is the best place in clang.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

[lebedev.ri]

clang-tidy/utils/ExprMutationAnalyzer.h

39

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

[lebedev.ri]

clang-tidy/utils/ExprMutationAnalyzer.h

39

(the statement Q within the statement S, of course)

[lebedev.ri]

clang-tidy/utils/ExprMutationAnalyzer.h

39

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

[shuaiwang]

In D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

clang-tidy/utils/ExprMutationAnalyzer.h

39

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

[JonasToth]

@shuaiwang What are you working on next? Do you have an idea on how we
will handle the pointee of a pointer? That would be very interesting for
my const-correctness check. if you need help with anything, just say so.
I would like to support

Am 10.09.2018 um 18:49 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38
+ const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+ const Stmt *findDeclMutation(const Decl *Dec);

+

lebedev.ri wrote:

lebedev.ri wrote:

lebedev.ri wrote:

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

(the statement Q within the statement S, of course)

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

[shuaiwang]

I have a rough idea about how findPointeeMutation would look like, I'm pretty sure I can use a lot of your help :)
My current plan:

• Finish the few existing pending changes
• Move the analyzer to clang/lib/Analysis (likely remove PseudoConstantAnalysis there as well since it appears to be dead code for years)
• Implement findPointeeMutation

In D45679#1229071, @JonasToth wrote:

@shuaiwang What are you working on next? Do you have an idea on how we
will handle the pointee of a pointer? That would be very interesting for
my const-correctness check. if you need help with anything, just say so.
I would like to support

Am 10.09.2018 um 18:49 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45679#1183116, @george.karpenkov wrote:

@aaron.ballman @alexfh @shuaiwang Would it be possible to move that code into a matcher, or into a something which could be used from Clang? We would also like to use similar functionality, but can't include stuff from clang-tidy.

FYI I haven't forgot about this, I'll look into doing it after a few pending changes are in.

Comment at: clang-tidy/utils/ExprMutationAnalyzer.h:38
+ const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+ const Stmt *findDeclMutation(const Decl *Dec);

+

lebedev.ri wrote:

lebedev.ri wrote:

lebedev.ri wrote:

@shuaiwang, @JonasToth hi.
Is it very intentional that this findDeclMutation() is private?

Is there some other way i should be doing if i have a statement S,
a declRefExpr D, and i want to find the statement Q, which mutates
the underlying variable to which the declRefExpr D refers?

(the statement Q within the statement S, of course)

@shuaiwang after a disscussion about this in IRC with @JonasToth, i have filed https://bugs.llvm.org/show_bug.cgi?id=38888
But i'm failing to CC you there. Are you not registered in the bugzilla?

There's no real reason findDeclMutation is private other than that there wasn't a use case :)
Feel free to make it public if you find it useful that way.

I'll take a look at the bug, thanks for reporting it!
(I don't have an account there yet, I'm requesting one right now, will follow up in the bug)

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45679

[JonasToth]

In D45679#1229176, @shuaiwang wrote:

I have a rough idea about how findPointeeMutation would look like, I'm pretty sure I can use a lot of your help :)
My current plan:

• Finish the few existing pending changes
• Move the analyzer to clang/lib/Analysis (likely remove PseudoConstantAnalysis there as well since it appears to be dead code for years)
• Implement findPointeeMutation

Sounds like a solid plan. I think the clang-tidy/utils/DeclRefExprUtils.{h,cpp} are not necessary anymore, too. They are used in 2 clang-tidy checks. Maybe i can remove them as well :)

clang-tidy/utils/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyUtils
   ASTUtils.cpp
   DeclRefExprUtils.cpp
+  ExprMutationAnalyzer.cpp
   ExprSequence.cpp
   FixItHintUtils.cpp
   HeaderFileExtensionsUtils.cpp
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
   NamespaceAliaser.cpp

clang-tidy/utils/ExprMutationAnalyzer.h

+//===---------- ExprMutationAnalyzer.h - clang-tidy -----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H
+
+#include <type_traits>
+
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/ADT/DenseMap.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// Analyzes whether any mutative operations are applied to an expression within
+/// a given statement.
+class ExprMutationAnalyzer {
+public:
+  ExprMutationAnalyzer(const Stmt *Stm, ASTContext *Context)
+      : Stm(Stm), Context(Context) {}
+
+  bool isMutated(const Decl *Dec) { return findDeclMutation(Dec) != nullptr; }
+  bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
+  const Stmt *findMutation(const Expr *Exp);
+
+private:
+  const Stmt *findExprMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+  const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
+  const Stmt *findDeclMutation(const Decl *Dec);
+
+  const Stmt *findDirectMutation(const Expr *Exp);
+  const Stmt *findMemberMutation(const Expr *Exp);
+  const Stmt *findArrayElementMutation(const Expr *Exp);
+  const Stmt *findCastMutation(const Expr *Exp);
+  const Stmt *findRangeLoopMutation(const Expr *Exp);
+  const Stmt *findReferenceMutation(const Expr *Exp);
+
+  const Stmt *const Stm;
+  ASTContext *const Context;
+  llvm::DenseMap<const Expr *, const Stmt *> Results;
+};
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRMUTATIONANALYZER_H

clang-tidy/utils/ExprMutationAnalyzer.cpp

+//===---------- ExprMutationAnalyzer.cpp - clang-tidy ---------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include "ExprMutationAnalyzer.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+using namespace ast_matchers;
+
+namespace {
+
+AST_MATCHER_P(LambdaExpr, hasCaptureInit, const Expr *, E) {
+  for (const auto *Init : Node.capture_inits()) {
+    if (Init == E)
+      return true;
+  }
+  return false;
+}
+
+AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt,
+              ast_matchers::internal::Matcher<DeclStmt>, InnerMatcher) {
+  const DeclStmt *const Range = Node.getRangeStmt();
+  return InnerMatcher.matches(*Range, Finder, Builder);
+}
+
+const auto nonConstReferenceType = [] {
+  return referenceType(pointee(unless(isConstQualified())));
+};
+
+} // namespace
+
+const Stmt *ExprMutationAnalyzer::findMutation(const Expr *Exp) {
+  const auto Memoized = Results.find(Exp);
+  if (Memoized != Results.end())
+    return Memoized->second;
+
+  for (const auto &Finder : {&ExprMutationAnalyzer::findDirectMutation,
+                             &ExprMutationAnalyzer::findMemberMutation,
+                             &ExprMutationAnalyzer::findArrayElementMutation,
+                             &ExprMutationAnalyzer::findCastMutation,
+                             &ExprMutationAnalyzer::findRangeLoopMutation,
+                             &ExprMutationAnalyzer::findReferenceMutation}) {
+    if (const Stmt *S = (this->*Finder)(Exp))
+      return Results[Exp] = S;
+  }
+
+  return Results[Exp] = nullptr;
+}
+
+const Stmt *
+ExprMutationAnalyzer::findExprMutation(ArrayRef<BoundNodes> Matches) {
+  for (const auto &Nodes : Matches) {
+    if (const Stmt *S = findMutation(Nodes.getNodeAs<Expr>("expr")))
+      return S;
+  }
+  return nullptr;
+}
+
+const Stmt *
+ExprMutationAnalyzer::findDeclMutation(ArrayRef<BoundNodes> Matches) {
+  for (const auto &DeclNodes : Matches) {
+    if (const Stmt *S = findDeclMutation(DeclNodes.getNodeAs<Decl>("decl")))
+      return S;
+  }
+  return nullptr;
+}
+
+const Stmt *ExprMutationAnalyzer::findDeclMutation(const Decl *Dec) {
+  const auto Refs = match(
+      findAll(declRefExpr(to(equalsNode(Dec))).bind("expr")), *Stm, *Context);
+  for (const auto &RefNodes : Refs) {
+    const auto *E = RefNodes.getNodeAs<Expr>("expr");
+    if (findMutation(E))
+      return E;
+  }
+  return nullptr;
+}
+
+const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+  // DeclRefExpr to a volatile qualified variable.
+  const auto RefToVolatile =
+      declRefExpr(hasType(isVolatileQualified()), equalsNode(Exp));
+
+  // LHS of any assignment operators.
+  const auto AsAssignmentLhs =
+      binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));
+
+  // Operand of increment/decrement operators.
+  const auto AsIncDecOperand =
+      unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
+                    hasUnaryOperand(equalsNode(Exp)));
+
+  // Invoking non-const member function.
+  const auto NonConstMethod = cxxMethodDecl(unless(isConst()));
+  const auto AsNonConstThis =
+      expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod), on(equalsNode(Exp))),
+                 cxxOperatorCallExpr(callee(NonConstMethod),
+                                     hasArgument(0, equalsNode(Exp)))));
+
+  // Taking address of 'Exp'.
+  // We're assuming 'Exp' is mutated as soon as its address is taken, though in
+  // theory we can follow the pointer and see whether it escaped `Stm` or is
+  // dereferenced and then mutated. This is left for future improvements.
+  const auto AsAmpersandOperand =
+      unaryOperator(hasOperatorName("&"),
+                    // A NoOp implicit cast is adding const.
+                    unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
+                    hasUnaryOperand(equalsNode(Exp)));
+  const auto AsPointerFromArrayDecay =
+      castExpr(hasCastKind(CK_ArrayToPointerDecay),
+               unless(hasParent(arraySubscriptExpr())), has(equalsNode(Exp)));
+
+  // Used as non-const-ref argument when calling a function.
+  const auto NonConstRefParam = forEachArgumentWithParam(
+      equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+  const auto AsNonConstRefArg =
+      anyOf(callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam));
+
+  // Captured by a lambda by reference.
+  // If we're initializing a capture with 'Exp' directly then we're initializing
+  // a reference capture.
+  // For value captures there will be an ImplicitCastExpr <LValueToRValue>.
+  const auto AsLambdaRefCaptureInit = lambdaExpr(hasCaptureInit(Exp));
+
+  // Returned as non-const-ref.
+  // If we're returning 'Exp' directly then it's returned as non-const-ref.
+  // For returning by value there will be an ImplicitCastExpr <LValueToRValue>.
+  // For returning by const-ref there will be an ImplicitCastExpr <NoOp> (for
+  // adding const.)
+  const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp)));
+
+  const auto Matches =
+      match(findAll(stmt(anyOf(RefToVolatile, AsAssignmentLhs, AsIncDecOperand,
+                               AsNonConstThis, AsAmpersandOperand,
+                               AsPointerFromArrayDecay, AsNonConstRefArg,
+                               AsLambdaRefCaptureInit, AsNonConstRefReturn))
+                        .bind("stmt")),
+            *Stm, *Context);
+  return selectFirst<Stmt>("stmt", Matches);
+}
+
+const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) {
+  // Check whether any member of 'Exp' is mutated.
+  const auto MemberExprs = match(
+      findAll(memberExpr(hasObjectExpression(equalsNode(Exp))).bind("expr")),
+      *Stm, *Context);
+  return findExprMutation(MemberExprs);
+}
+
+const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) {
+  // Check whether any element of an array is mutated.
+  const auto SubscriptExprs = match(
+      findAll(arraySubscriptExpr(hasBase(ignoringImpCasts(equalsNode(Exp))))
+                  .bind("expr")),
+      *Stm, *Context);
+  return findExprMutation(SubscriptExprs);
+}
+
+const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
+  // If 'Exp' is casted to any non-const reference type, check the castExpr.
+  const auto Casts =
+      match(findAll(castExpr(hasSourceExpression(equalsNode(Exp)),
+                             anyOf(explicitCastExpr(hasDestinationType(
+                                       nonConstReferenceType())),
+                                   implicitCastExpr(hasImplicitDestinationType(
+                                       nonConstReferenceType()))))
+                        .bind("expr")),
+            *Stm, *Context);
+  return findExprMutation(Casts);
+}
+
+const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
+  // If range for looping over 'Exp' with a non-const reference loop variable,
+  // check all declRefExpr of the loop variable.
+  const auto LoopVars =
+      match(findAll(cxxForRangeStmt(
+                hasLoopVariable(
+                    varDecl(hasType(nonConstReferenceType())).bind("decl")),
+                hasRangeInit(equalsNode(Exp)))),
+            *Stm, *Context);
+  return findDeclMutation(LoopVars);
+}
+
+const Stmt *ExprMutationAnalyzer::findReferenceMutation(const Expr *Exp) {
+  // If 'Exp' is bound to a non-const reference, check all declRefExpr to that.
+  const auto Refs = match(
+      stmt(forEachDescendant(
+          varDecl(
+              hasType(nonConstReferenceType()),
+              hasInitializer(anyOf(equalsNode(Exp),
+                                   conditionalOperator(anyOf(
+                                       hasTrueExpression(equalsNode(Exp)),
+                                       hasFalseExpression(equalsNode(Exp)))))),
+              hasParent(declStmt().bind("stmt")),
+              // Don't follow the reference in range statement, we've handled
+              // that separately.
+              unless(hasParent(declStmt(hasParent(
+                  cxxForRangeStmt(hasRangeStmt(equalsBoundNode("stmt"))))))))
+              .bind("decl"))),
+      *Stm, *Context);
+  return findDeclMutation(Refs);
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+

unittests/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANG_LINT_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
+  ExprMutationAnalyzerTest.cpp
   IncludeInserterTest.cpp
   GoogleModuleTest.cpp
   LLVMModuleTest.cpp
   NamespaceAliaserTest.cpp
   ObjCModuleTest.cpp
   OverlappingReplacementsTest.cpp
   UsingInserterTest.cpp
   ReadabilityModuleTest.cpp)

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

+//===---------- ExprMutationAnalyzerTest.cpp - clang-tidy -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher<Expr>;
+using StmtMatcher = internal::Matcher<Stmt>;
+
+ExprMatcher declRefTo(StringRef Name) {
+  return declRefExpr(to(namedDecl(hasName(Name))));
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+  return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
+  const auto *const S = selectFirst<Stmt>("stmt", Results);
+  const auto *const E = selectFirst<Expr>("expr", Results);
+  return utils::ExprMutationAnalyzer(S, &AST->getASTContext()).isMutated(E);
+}
+
+SmallVector<std::string, 1>
+mutatedBy(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) {
+  const auto *const S = selectFirst<Stmt>("stmt", Results);
+  SmallVector<std::string, 1> Chain;
+  utils::ExprMutationAnalyzer Analyzer(S, &AST->getASTContext());
+  for (const auto *E = selectFirst<Expr>("expr", Results); E != nullptr;) {
+    const Stmt *By = Analyzer.findMutation(E);
+    std::string buffer;
+    llvm::raw_string_ostream stream(buffer);
+    By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+    Chain.push_back(StringRef(stream.str()).trim().str());
+    E = dyn_cast<DeclRefExpr>(By);
+  }
+  return Chain;
+}
+
+std::string removeSpace(std::string s) {
+  s.erase(std::remove_if(s.begin(), s.end(),
+                         [](char c) { return std::isspace(c); }),
+          s.end());
+  return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+  const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RefToVolatile) {
+  const auto AST = tooling::buildASTFromCode("void f() { volatile int x; x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_TRUE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam<std::string> {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+  const std::string ModExpr = "x " + GetParam() + " 10";
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+                        Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+                               "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam<std::string> {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+  const std::string ModExpr = GetParam();
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+                        Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, NonConstOperator) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct Foo { Foo& operator=(int); }; Foo x; x = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, ConstOperator) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct Foo { int operator()() const; }; Foo x; x(); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ByValueArgument) {
+  auto AST =
+      tooling::buildASTFromCode("void g(int); void f() { int x; g(x); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A {}; A operator+(A, int); A x; x + 1; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(int); }; int x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(); A(A); }; A x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ByNonConstRefArgument) {
+  auto AST =
+      tooling::buildASTFromCode("void g(int&); void f() { int x; g(x); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A {}; A operator+(A&, int); A x; x + 1; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x + 1"));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(int&); }; int x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(); A(A&); }; A x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
+}
+
+TEST(ExprMutationAnalyzerTest, ByConstRefArgument) {
+  auto AST = tooling::buildASTFromCode(
+      "void g(const int&); void f() { int x; g(x); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A {}; A operator+(const A&, int); A x; x + 1; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(const int&); }; int x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A { A(); A(const A&); }; A x; A y(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ByNonConstRRefArgument) {
+  auto AST = tooling::buildASTFromCode(
+      "void g(int&&); void f() { int x; g(static_cast<int &&>(x)); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("g(static_cast<int &&>(x))"));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A {}; A operator+(A&&, int); "
+      "A x; static_cast<A &&>(x) + 1; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("static_cast<A &&>(x) + 1"));
+
+  AST = tooling::buildASTFromCode("void f() { struct A { A(int&&); }; "
+                                  "int x; A y(static_cast<int &&>(x)); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("static_cast<int &&>(x)"));
+
+  AST = tooling::buildASTFromCode("void f() { struct A { A(); A(A&&); }; "
+                                  "A x; A y(static_cast<A &&>(x)); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("static_cast<A &&>(x)"));
+}
+
+TEST(ExprMutationAnalyzerTest, ByConstRRefArgument) {
+  auto AST = tooling::buildASTFromCode(
+      "void g(const int&&); void f() { int x; g(static_cast<int&&>(x)); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { struct A {}; A operator+(const A&&, int); "
+      "A x; static_cast<A&&>(x) + 1; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode("void f() { struct A { A(const int&&); }; "
+                                  "int x; A y(static_cast<int&&>(x)); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode("void f() { struct A { A(); A(const A&&); }; "
+                                  "A x; A y(static_cast<A&&>(x)); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, Move) {
+  // Technically almost the same as ByNonConstRRefArgument, just double checking
+  const auto AST = tooling::buildASTFromCode(
+      "namespace std {"
+      "template<class T> struct remove_reference { typedef T type; };"
+      "template<class T> struct remove_reference<T&> { typedef T type; };"
+      "template<class T> struct remove_reference<T&&> { typedef T type; };"
+      "template<class T> typename std::remove_reference<T>::type&& "
+      "move(T&& t) noexcept; }"
+      "void f() { struct A {}; A x; std::move(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x)"));
+}
+
+TEST(ExprMutationAnalyzerTest, Forward) {
+  // Technically almost the same as ByNonConstRefArgument, just double checking
+  const auto AST = tooling::buildASTFromCode(
+      "namespace std {"
+      "template<class T> struct remove_reference { typedef T type; };"
+      "template<class T> struct remove_reference<T&> { typedef T type; };"
+      "template<class T> struct remove_reference<T&&> { typedef T type; };"
+      "template<class T> T&& "
+      "forward(typename std::remove_reference<T>::type&) noexcept;"
+      "template<class T> T&& "
+      "forward(typename std::remove_reference<T>::type&&) noexcept;"
+      "void f() { struct A {}; A x; std::forward<A &>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("std::forward<A &>(x)"));
+}
+
+TEST(ExprMutationAnalyzerTest, ReturnAsValue) {
+  const auto AST = tooling::buildASTFromCode("int f() { int x; return x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ReturnAsNonConstRef) {
+  const auto AST = tooling::buildASTFromCode("int& f() { int x; return x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("return x;"));
+}
+
+TEST(ExprMutationAnalyzerTest, ReturnAsConstRef) {
+  const auto AST =
+      tooling::buildASTFromCode("const int& f() { int x; return x; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ReturnAsNonConstRRef) {
+  const auto AST = tooling::buildASTFromCode(
+      "int&& f() { int x; return static_cast<int &&>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("return static_cast<int &&>(x);"));
+}
+
+TEST(ExprMutationAnalyzerTest, ReturnAsConstRRef) {
+  const auto AST = tooling::buildASTFromCode(
+      "const int&& f() { int x; return static_cast<int&&>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, TakeAddress) {
+  const auto AST =
+      tooling::buildASTFromCode("void g(int*); void f() { int x; g(&x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("&x"));
+}
+
+TEST(ExprMutationAnalyzerTest, ArrayToPointerDecay) {
+  const auto AST =
+      tooling::buildASTFromCode("void g(int*); void f() { int x[2]; g(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
+}
+
+TEST(ExprMutationAnalyzerTest, FollowRefModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
+      "int& r3 = r2; r3 = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("r0", "r1", "r2", "r3", "r3 = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, FollowRefNotModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
+      "int& r3 = r2; int& r4 = r3; int& r5 = r4;}");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, FollowConditionalRefModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x, y; bool b; int &r = b ? x : y; r = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("r", "r = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, FollowConditionalRefNotModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x, y; bool b; int& r = b ? x : y; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, ArrayElementModified) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x[2]; x[0] = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x[0] = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, ArrayElementNotModified) {
+  const auto AST = tooling::buildASTFromCode("void f() { int x[2]; x[0]; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, NestedMemberModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct A { int vi; }; struct B { A va; }; "
+      "struct C { B vb; }; C x; x.vb.va.vi = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.vb.va.vi = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, NestedMemberNotModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { struct A { int vi; }; struct B { A va; }; "
+      "struct C { B vb; }; C x; x.vb.va.vi; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CastToValue) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; static_cast<double>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CastToRefModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x; static_cast<int &>(x) = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("static_cast<int &>(x) = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, CastToRefNotModified) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; static_cast<int&>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, CastToConstRef) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x; static_cast<const int&>(x); }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByValue) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; [=]() { x = 10; }; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, LambdaExplicitCaptureByValue) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; [x]() { x = 10; }; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByRef) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; [&]() { x = 10; }; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre(ResultOf(removeSpace, "[&](){x=10;}")));
+}
+
+TEST(ExprMutationAnalyzerTest, LambdaExplicitCaptureByRef) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x; [&x]() { x = 10; }; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre(ResultOf(removeSpace, "[&x](){x=10;}")));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForArrayByRefModified) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x[2]; for (int& e : x) e = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForArrayByRefNotModified) {
+  const auto AST =
+      tooling::buildASTFromCode("void f() { int x[2]; for (int& e : x) e; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForArrayByValue) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x[2]; for (int e : x) e = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
+  const auto AST = tooling::buildASTFromCode(
+      "void f() { int x[2]; for (const int& e : x) e; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForNonArrayByRefModified) {
+  const auto AST =
+      tooling::buildASTFromCode("struct V { int* begin(); int* end(); };"
+                                "void f() { V x; for (int& e : x) e = 10; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10"));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForNonArrayByRefNotModified) {
+  const auto AST =
+      tooling::buildASTFromCode("struct V { int* begin(); int* end(); };"
+                                "void f() { V x; for (int& e : x) e; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForNonArrayByValue) {
+  const auto AST = tooling::buildASTFromCode(
+      "struct V { const int* begin() const; const int* end() const; };"
+      "void f() { V x; for (int e : x) e; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+TEST(ExprMutationAnalyzerTest, RangeForNonArrayByConstRef) {
+  const auto AST = tooling::buildASTFromCode(
+      "struct V { const int* begin() const; const int* end() const; };"
+      "void f() { V x; for (const int& e : x) e; }");
+  const auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang