[clang-tidy] implement new check for const-correctness

[JonasToth]

Summary

This check aims to determine values and references that could be declared
as const, but are not.

The first version I am aiming at only analyzes local variables.
No fixits are emitted.

This check aims to implement CPPCG-ES. 25: Declare an object const or constexpr unless you want to modify its value later on
and HICPP-7.1.2 Use const whenever possible.

Because const-correctness includes many issues this check will be implemented
in multiple stages.

[JonasToth]

@aaron.ballman, @lebedev.ri, @alexfh and everyone else, i would love to hear your opinion on my approach. i never implemented a similar check and i might miss some cases, that need to be considered.

The current state is just a proof-of-concept to see how such a check could be implemented.

[Eugene.Zelenko]

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

clang-tidy/cppcoreguidelines/ConstCheck.cpp

14

Please remove empty line.

docs/ReleaseNotes.rst

60

Please move into new checks list in alphabetical order.

63

Please add short description here.

docs/clang-tidy/checks/cppcoreguidelines-const.rst

7

Preliminary documentation will be helpful.

[Eugene.Zelenko]

Will be good idea to add HICPP alias.

[JonasToth]

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

Agreed. I aim at a mark handles const(default on), mark values const(default on for objects), mark pointer (const int * >> const <<) const (default off)

What do you mean by built-in types/enums? That they are configurable separate to objects?

[Eugene.Zelenko]

In D45444#1063291, @JonasToth wrote:

It'll be good idea to have option to apply this check for pointer/references only, or include built-in types/enums.

Agreed. I aim at a mark handles const(default on), mark values const(default on for objects), mark pointer (const int * >> const <<) const (default off)

What do you mean by built-in types/enums? That they are configurable separate to objects?

I meant integers/floating point numbers/Boolean/enums. It may be overkill to apply this check for legacy code base in single pass.

[lebedev.ri]

Thank you for working on this check!

clang-tidy/cppcoreguidelines/ConstCheck.h

61

I'm guessing these should be llvm::DenseMap.

[JonasToth]

• [Feature] implement array and method access
• [Feature] finalize value semantic

[JonasToth]

• [Feature] start working on handle semantic

[JonasToth]

• [Misc] unify handle and value modification detection
• [Misc] found new caveats, maybe take a look at refactoring first an require this check to produce clean compiles on llvm

[JonasToth]

@aaron.ballman @lebedev.ri The check is getting really complex. I run it over LLVM and detected some warnings, where iam not sure if they are valid or not. Its already a somewhat usable state, but its hard to determine false positives and false negatives.

For me, that false positives are worse for now, because they annoy. Its easier to add false negatives especially with user feedback. My idea is now: Take a look at refactoring the code to introduce const as fixit and iterate the check until LLVM compiles after the check fixed all missing consts. What do you think about that strategy? And could you take a look at the todo list at the top of ConstCheck.cpp. Did I miss something?

[shuaiwang]

Coming from https://reviews.llvm.org/D45679, which I should have sent out much earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help incorporate mine into this one.

I do have a strong opinion that requires non-trivial refactoring of your diff though: having a standalone reusable helper function like isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

• performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a much less sophisticated isOnlyUsedAsConst(), and can benefit from a more powerful one.
• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".
• It's cleaner to follow (non-const) reference chains and only mark a decl as "can't be const" if everything on the chain is not modified, avoiding false negatives when a variable is bound to a never-modified non-const reference.
• It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

[JonasToth]

Coming from https://reviews.llvm.org/D45679, which I should have sent out much earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help incorporate mine into this one.

I would love to, yours is more elegant :)

I do have a strong opinion that requires non-trivial refactoring of your diff though: having a standalone reusable helper function like isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

Yes please!

performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a much less sophisticated isOnlyUsedAsConst(), and can benefit from a more powerful one.

I did not think about such possibilities, but maybe other analysis task could benefit too? Warnings?

I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Using my approach, you can check if any member variable is used as non-const. Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.

It's cleaner to follow (non-const) reference chains and only mark a decl as "can't be const" if everything on the chain is not modified, avoiding false negatives when a variable is bound to a never-modified 
non-const reference.

For warning only yes. But i want automatic code transformation which will break such code if there are non-const references taken. Having it in one pass is certainly nice. With my approach multiple runs are required if the handles are marked as const.

It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

Does your approach work with the nesting? Maybe something recursive or hasDescendent(modifying) could do it?

Is read your comment on this check later, i first saw your new check. Maybe we should discuss only under one of both :) (which one is not important for me :))

[Eugene.Zelenko]

In D45444#1068496, @shuaiwang wrote:

• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Probably this should be separate check. See also PR21981.

[shuaiwang]

I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Using my approach, you can check if any member variable is used as non-const. Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.

You'll also need to check:

• a member function calling another non-const member function -> *this can't be const
• *this is passed as non-const reference param to a function -> *this can't be const

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I think member variables are a separate topic: I think we should just treat them as globals and not check on them, the same argument that they can be accessed from multiple translation unit applies to global/namespace scoped variables and class scoped variables. We can only reliably check function/block scope variables.
(reliable meaning being able to achieve zero false positives with useful level of recall, false negatives are inevitable because whenever a modifiable reference/handle escape the current block/translation unit we'll have to assume it's modified.)

It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" modifies "v". This one is particularly tricky because the modifying behavior happens at "c = 10" while the variable we'd like to mark as "can't be const" is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be const" because that'll introduce way too many false negatives, ... and I haven't figure out a way to write a matcher to match memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... <arbitrary layers deep> ... (VarDeclRef) ...), not to mention any layer could either member access or array subscript access.

Does your approach work with the nesting? Maybe something recursive or hasDescendent(modifying) could do it?

Yes my approach is doing multi-pass matching by calling isModified() recursively. I consider the multi-pass matching "necessary evil" because otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that makes things easier: varDeclRef(hasAncestor(modifying)) would match both "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the modifying behavior does link back to the particular varDeclRef.

Reviewers, what are your thoughts about the approaches?

[shuaiwang]

In D45444#1068967, @Eugene.Zelenko wrote:

In D45444#1068496, @shuaiwang wrote:

• I would imagine things could get messier if this check expands to also check for turning member functions const: it's basically checking CxxThisExpr, being a handle, is not modified within a member function, but note there's no VarDecl for "this".

Probably this should be separate check. See also PR21981.

Sure, a separate check sounds better.
Which makes even strong argument of having a reusable utility checking whether something is modified that can be shared between different checks :)

[JonasToth]

You'll also need to check:

• a member function calling another non-const member function -> *this can't be const
• *this is passed as non-const reference param to a function -> *this can't be const

Yes. That is similar behaviour to checking if a function can be noexcept. But this is all future stuff :)

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I cant follow you on that one. I consider every path that allows future modification (like taking a non-const reference/pointer) as cant be const. That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const handle to it, which must have been created at some point.

I think member variables are a separate topic: I think we should just treat them as globals and not check on them, the same argument that they can be accessed from multiple translation unit applies to global/namespace scoped variables and class scoped variables. We can only reliably check function/block scope variables.
(reliable meaning being able to achieve zero false positives with useful level of recall, false negatives are inevitable because whenever a modifiable reference/handle escape the current block/translation unit we'll have to assume it's modified.)

You are probably right. The only zero-false positive case is: "only const methods called". One could split implementation of a class into several translation units and render the analysis approach useless.

Yes my approach is doing multi-pass matching by calling isModified() recursively. I consider the multi-pass matching "necessary evil" because otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that makes things easier: varDeclRef(hasAncestor(modifying)) would match both "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the modifying behavior does link back to the particular varDeclRef.

Example as reference for later: https://godbolt.org/g/cvhoUN
I will add such cases to my tests.

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin
• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

[shuaiwang]

I've updated https://reviews.llvm.org/D45679 and I think the isModified() function there is now sufficiently covering the cases you've covered here and can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following differences:

• All unused local variables removed, my check will issue a warning for them because technically they're not modified, but I understand why you don't want to cover them. I don't feel strongly about it and I removed it from my copy-pasted test cases because I just to demonstrate isModified()
• Better recall in some_reference_taking, both np_local0 and r1_np_local0 can be caught, similar for np_local1 and non_const_ref in handle_from_array.
• direct_class_access np_local5 triggers with my check (shouldn't it?)
• In range_for non-const loop variables triggers with my check (shouldn't they?)
• In range_for local arrays doesn't trigger with my check, because I'm treating ArrayToPointerDecay the same as taking address of a variable, and looping over an array would involve ArrayToPointerDecay when the implicit __begin/__end are initialized. I added a note inside isModified for future improvements.
• My check over triggers for template (with my failed attempt to fix), but I think it's more of some mistake in the check itself rather than in isModified

Also when marking decls as "can't be const" we'll have to do record separately for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary IMO.

I cant follow you on that one. I consider every path that allows future modification (like taking a non-const reference/pointer) as cant be const. That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const handle to it, which must have been created at some point.

Sorry about the confusion.
Basically consider this example:

class Foo {
public:
  void a() { x = 10; }
  void b() { // nothing }
  void c() { a(); }
  void d() { b(); }
  
private:
  int x;
};

If we check whether isModified(dereference(cxxThisExpr())) for each CompondStmt(hasParent(functionDecl())), we would conclude that:

• a() should stay non-const, because there's this->x = 10 modifying *this
• b() should be changed to const, because nothing modifies *this
• c() should stay non-const, because we're invoking non-const member function on this
• d() should also stay non-const, with the same reason for c(). (We can in theory be smarter about this one because the definition of b() happens to be inside the same TU but that's out of scope right now)

However if we use a per-TU map to record whether x can be const, we'll conclude that x is modified thus can't be const, missing the one that b() is not modifying x. To fix that we'll need two-layered map recording that x is only modified in a() and potentially modified indirectly from c() and d(), so that in the end we can figure out that b() is safe to be marked const.

Anyway, all I was trying to say is: let's use the isModified() approach as it's simpler & cleaner for future use cases like adding const to member functions. And it feels to me that we've already reached agreement there :)

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

Not really, but I can get myself familiar with it.

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin

It doesn't have to be DeclRefExpr, any Expr should work, and it's useful to make it accept any Expr:

• It can be used to check whether dereference(cxxThisExpr()) is modified or not.
• For pointer types, it can be used to check both declRefExpr(isPointerType()) and dereference(declRefExpr(isPointerType())), and suggest adding const at different level

• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

Great catch!

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

I believe we can issue warning + fixit within one pass:

int f() {
  int a = 10;
  int& b = a;
  return b;
}

We should be able to issue warnings for both a and b, because b itself is a varDecl without modifying behavior, and when checking a we'll just repeat a bit more work that was already done when checking b by following the reference chain to be able to find that a can be const as well.

[JonasToth]

In D45444#1069488, @shuaiwang wrote:

I've updated https://reviews.llvm.org/D45679 and I think the isModified() function there is now sufficiently covering the cases you've covered here and can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following differences:

• All unused local variables removed, my check will issue a warning for them because technically they're not modified, but I understand why you don't want to cover them. I don't feel strongly about it and I removed it from my copy-pasted test cases because I just to demonstrate isModified()

Nice!

• Better recall in some_reference_taking, both np_local0 and r1_np_local0 can be caught, similar for np_local1 and non_const_ref in handle_from_array.

Yes. But i feel we need to gather more information. For potential checks, it might be nice to control to which scope modification is defined and/or emit notes for interesting events like taking a non-const handle which isnt modified, too.

It kinda feels like we are converging to a dependency graph for every variable that contains the information for dependencies and dependents + modification history. I dont think it is bad, but maybe complicated :)
What do you think about returning a ModificationRecord that contains the number of direct modification, the number of local non-const handles and escaping non-const handles.
Including const handles would be nice in the future for more and different analysis.

• direct_class_access np_local5 triggers with my check (shouldn't it?)

Yes can be const: Godbolt

• In range_for non-const loop variables triggers with my check (shouldn't they?)

• If the loop copies every value no modification occurs (its not a copy, its initialization. If the constructor would modify its value, the source cant be const either. Is it allowed?)
• if a handle is taken the usual rules apply.

• In range_for local arrays doesn't trigger with my check, because I'm treating ArrayToPointerDecay the same as taking address of a variable, and looping over an array would involve ArrayToPointerDecay when the implicit __begin/__end are initialized. I added a note inside isModified for future improvements.

Given this is implemented here, can you use it?

• My check over triggers for template (with my failed attempt to fix), but I think it's more of some mistake in the check itself rather than in isModified

I removed templates while matching for potential candidates. That can be moved out of the isModified(). isModified() can only reason about instantiated functions. For the actual template we need concepts.

Sorry about the confusion.
Basically consider this example:

class Foo {
public:
  void a() { x = 10; }
  void b() { // nothing }
  void c() { a(); }
  void d() { b(); }
  
private:
  int x;
};

If we check whether isModified(dereference(cxxThisExpr())) for each CompondStmt(hasParent(functionDecl())), we would conclude that:

• a() should stay non-const, because there's this->x = 10 modifying *this
• b() should be changed to const, because nothing modifies *this
• c() should stay non-const, because we're invoking non-const member function on this
• d() should also stay non-const, with the same reason for c(). (We can in theory be smarter about this one because the definition of b() happens to be inside the same TU but that's out of scope right now)

However if we use a per-TU map to record whether x can be const, we'll conclude that x is modified thus can't be const, missing the one that b() is not modifying x. To fix that we'll need two-layered map recording that x is only modified in a() and potentially modified indirectly from c() and d(), so that in the end we can figure out that b() is safe to be marked const.
Anyway, all I was trying to say is: let's use the isModified() approach as it's simpler & cleaner for future use cases like adding const to member functions. And it feels to me that we've already reached agreement there :)

Yes.

@shuaiwang Are you in IRC from time to time? I think it will be better to discuss changes in chat, when i start to introduce your approach here.

Not really, but I can get myself familiar with it.

We dont need to chat. The review discussion works well. :)

isModified(Expr, Stmt), that checks whether Expr is (potentially) modified within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

• implement a utility taking DeclRefExpr and check if there is modification in some scope (Stmt, e.g. CompoundStmt) -> isModified or cantBeModifiedWithin

It doesn't have to be DeclRefExpr, any Expr should work, and it's useful to make it accept any Expr:

• It can be used to check whether dereference(cxxThisExpr()) is modified or not.
• For pointer types, it can be used to check both declRefExpr(isPointerType()) and dereference(declRefExpr(isPointerType())), and suggest adding const at different level

• match all relevant non-const variable declarations as potential candidates
• track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and static declared variables. They have TU scope.

Great catch!

My deviations:

• a variable should be considered modified if a non-const handle is create from it, even if that handle does not modify its referenced value. As first step, those handles should be marked const, then the original value can be marked as const. That is required to produce compiling code after potential code-transformation.

I believe we can issue warning + fixit within one pass:

int f() {
  int a = 10;
  int& b = a;
  return b;
}

We should be able to issue warnings for both a and b, because b itself is a varDecl without modifying behavior, and when checking a we'll just repeat a bit more work that was already done when checking b by following the reference chain to be able to find that a can be const as well.

We can certainly try it. Speaking from previous experience: There is always some weird way to not catch everything and to get false positives in edge cases, thats why i tried the conservative way first. But i was stumbling around, too. I think the new function can give us more confidence, because its easier to test in isolation.

One interesting thing i found in code: int &some_var = condition ? var1 : var2. I am not sure if we already find that as reference taking :)

[JonasToth]

• [Feature] refactor check to use a utility function to determine constness

[JonasToth]

@shuaiwang I implemented the check on top of you utility function. It does fail right now (concentrating on values only for now). I added a FIXME for all false positives/negatives for the values test cases.

[JonasToth]

• [Misc] mark false positives

[JonasToth]

• [Feature] use new statefull const checker
• [Test] add more tests finding false positives

[shuaiwang]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

229–237

I think we need to loop over usages instead of just checking the first, i.e.:

for (const auto &Nodes : Usage) {
  const auto* UseExpr = Nodes.getNodeAs<Expr>("use");
  if (UseExpr && isMutated(UseExpr)) return true;
}

I'll add a helper function in the MutationAnalyzer for checking varDecl directly as well per your comment there, which you'll be able to use directly in this check. Before that's ready (and if you have time of course) could you help check how many false positives are left with this change?

[JonasToth]

I will migrate to the new API today evening (european time).

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Am 29.04.2018 um 20:19 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:229-237
+ const auto *UseExpr = selectFirst<Expr>("use", Usage);
+
+ The declared variables was used in non-const conserving way and can not
+ be declared as const.
+ if (UseExpr && Scopes[Scope]->isMutated(UseExpr)) {
+ // diag(UseExpr->getLocStart(), "Investigating starting with this use",

+ // DiagnosticIDs::Note);

I think we need to loop over usages instead of just checking the first, i.e.:

for (const auto &Nodes : Usage) {
  const auto* UseExpr = Nodes.getNodeAs<Expr>("use");
  if (UseExpr && isMutated(UseExpr)) return true;
}

I'll add a helper function in the MutationAnalyzer for checking varDecl directly as well per your comment there, which you'll be able to use directly in this check. Before that's ready (and if you have time of course) could you help check how many false positives are left with this change?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• migrate to Decl interface
• add template metaprogramming test

[JonasToth]

• fix bad template instantiation

[shuaiwang]

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:

int a; // <- varDecl
const int& b = a;  // <- first DeclRefExpr
int& c = a; // <- second DeclRefExpr

const int& b2 = b;
int& c2 = c;
c2 = 10;

If we start from the first DeclRefExpr, we'll only trace to b and then to b2. We need to start from varDecl (or loop over all DeclRefExpr) to be able to trace to the second DeclRefExpr from where we can get to c -> c2 -> c2 = 10.

[JonasToth]

I see. Thank you for fixing + explaining :)

Am 30.04.2018 um 23:39 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a Expr basis?

Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:

int a; // <- varDecl
const int& b = a;  // <- first DeclRefExpr
int& c = a; // <- second DeclRefExpr

const int& b2 = b;
int& c2 = c;
c2 = 10;

If we start from the first DeclRefExpr, we'll only trace to b and then to b2. We need to start from varDecl (or loop over all DeclRefExpr) to be able to trace to the second DeclRefExpr from where we can get to c -> c2 -> c2 = 10.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• rebase on commited ExpMutationAnalyzer
• clean up, documentation

[JonasToth]

• [Misc] order in release notes

[JonasToth]

• fixed some comments

[JonasToth]

• Merge branch 'master' into check_const

[lebedev.ri]

Ping, i guess :)
Anything needed to get this going?

[JonasToth]

Time :/

But review comments are still appreciated :)

Am 14.07.2018 um 16:34 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

Ping, i guess :)
Anything needed to get this going?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[0x8000-0000]

In D45444#1162715, @JonasToth wrote:

Time :/

Is this available as a public Git branch somewhere, or is downloading the diff the preferred way to interact with this code?

[JonasToth]

Yes.

https://github.com/JonasToth/clang-tools-extra/tree/check_const

This is the branch i work on. I got it up to date with the current
master for CTE. :)

Am 14.07.2018 um 20:59 schrieb Florin Iucha via Phabricator:

0x8000-0000 added a comment.

In https://reviews.llvm.org/D45444#1162715, @JonasToth wrote:

Time :/

Is this available as a public Git branch somewhere, or is downloading the diff the preferred way to interact with this code?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

@0x8000-0000 are you interested in working on the check? If i recall correctly the branch for 7.0 will happen on 1. august. That would be the timewindow in which it makes sense to give you the patch. I will have time in september following, but not right now.
I try to commit 2 bug fixes in this time, because they are almost done already.

@aaron.ballman What do you think is missing for this check? Do you think it is doable until 1. august?

[JonasToth]

Removed one unresolved comment, probably thats why it was stuck :D

[JonasToth]

• Merge branch 'master' into check_const
• [Misc] remove comment and iostream

[0x8000-0000]

In D45444#1164671, @JonasToth wrote:

@0x8000-0000 are you interested in working on the check? If i recall correctly the branch for 7.0 will happen on 1. august. That would be the timewindow in which it makes sense to give you the patch. I will have time in september following, but not right now.
I try to commit 2 bug fixes in this time, because they are almost done already.

@JonasToth
I am interested in helping - is there any area you'd like me to look at? I have reviewed the test cases and they seem fine. The code is a bit more intimidating, given that I just started looking at clang/llvm internals. But I have pulled the patch into my personal build at home and at ${BIG_CO} and I am using the check "in production". I have detected no crashes and no false positives yet.

[JonasToth]

What do you mean by "The code is a bit more intimidating", the check
itself or the amount of tests?

In general the utility piece that was commited before this check should
already analyze the constness good, given that this check builds upon it
the functionality should be ok.

In my opinion there is nothing major missing, but the reviewers (usually
@aaron.ballman) have to judge that. After that i would come back to you.

If you have a run on a codebase, you could attach logfile to see how
noisy the check is itself. :)

If you do have time, there are always bugs to fix :P

But you could also take a look at the enforcement of some coding
guidelines (my personal todolists:
https://github.com/JonasToth/CppCoreGuidelinesTooling 
https://github.com/JonasToth/HighIntegrityTooling). There are always low
hanging fruit checks. Implementing something there helps getting used to
the codebase of clang, too.

Am 17.07.2018 um 12:41 schrieb Florin Iucha via Phabricator:

@JonasToth
I am interested in helping - is there any area you'd like me to look at? I have reviewed the test cases and they seem fine. The code is a bit more intimidating, given that I just started looking at clang/llvm internals. But I have pulled the patch into my personal build at home and at ${BIG_CO} and I am using the check "in production". I have detected no crashes and no false positives yet.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[Eugene.Zelenko]

But you could also take a look at the enforcement of some coding
guidelines (my personal todolists:
https://github.com/JonasToth/CppCoreGuidelinesTooling 
https://github.com/JonasToth/HighIntegrityTooling). There are always low
hanging fruit checks. Implementing something there helps getting used to
the codebase of clang, too.

You may be also interested to look onto enhancements requests for Clang-tidy in http://bugs.llvm.org.

[0x8000-0000]

Any plans for fix-its that will add the suggested 'const' qualifiers?

[JonasToth]

Yes, but that is complicated. Functionality for that will first be
implemented in the libTooling and then utilized here later.

Am 19.07.2018 um 04:16 schrieb Florin Iucha via Phabricator:

0x8000-0000 added a comment.

Any plans for fix-its that will add the suggested 'const' qualifiers?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

@aaron.ballman Are you missing something in this check/tests? I think i will remove the big comment with my thoughts on what has to be done, or do you think it would help for reference?

I would probably be able to finalize this week if there is no major issue, so it could be committed for the release.

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

get check up to 8.0

[aaron.ballman]

I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!

clang-tidy/cppcoreguidelines/ConstCheck.cpp

33

Do you intend to support Obj-C object pointers as well?

142–143

Why is this check disabled for C code?

150

Drop the comma; that -> which

153–154

Can this use unless(anyOf(...)) instead?

165–166

No need for this -- check() won't be called unless there are registered matchers.

168

Can you pick a slightly different name -- Scope is the name of a type in the clang namespace.

191

const -> 'const'

clang-tidy/cppcoreguidelines/ConstCheck.h

26

Grammar nit, perhaps: This check warns on variables which could be declared const but are not.

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

43

This name leaves a bit to be desired. How about cppcoreguidelines-const-correctness?

docs/clang-tidy/checks/cppcoreguidelines-const.rst

7

that -> which

9

coding guidelines for example -> coding guidelines, such as:

13

type based -> type-based

[JonasToth]

I'm sorry for the delay in reviewing this; I'm not certain how it fell off my radar for so long!

No problem :)

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

33

For now not, because I have no experience nor knowledge with Obj-C.

[xbolva00]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

29

typo
paramters -> parameters

[JonasToth]

• Merge branch 'master' into check_const
• [Misc] rename and first review comments
• language stuff

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCheck.cpp

142–143

No actual reason. I will allow it for C, too.

[JonasToth]

• doc list

[JonasToth]

• revert breaking change in LocalVar matcher.

Reverting the change from allOf(...) to unless(anyOf(..)). I did not investigate
the reason for it breaking, because basic logic suggests that transformation
should be correct.

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

147 ↗

(On Diff #159220)

@aaron.ballman The change was not valid for some reason. I leave it like it is if thats ok with you.

[JonasToth]

• fix typo

[JonasToth]

• update ReleaseNotes

[aaron.ballman]

The functionality is looking good, aside from a few small nits remaining. However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter? Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?

clang-tidy/cppcoreguidelines/ConstCheck.cpp

33

Okay, then please add a comment mentioning that they're explicitly not handled yet (perhaps with a FIXME).

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

147 ↗

(On Diff #159220)

That's.... really odd. I am fine leaving it as-is for this patch, but it would be good to understand why that code fails as it seems like a reasonable exposition.

183 ↗

(On Diff #159226)

Still missing the single quotes around const in the diagnostic.

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

147 ↗

(On Diff #159220)

Added a TODO. But maybe i did the transformation incorrectly too. Not all conditions are negative. Maybe all the negative ones can be inverted by demorgan. That should be correct.

I will check this out later.

[JonasToth]

clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp

183 ↗

(On Diff #159226)

Ups. The comment has them :D

[JonasToth]

• address review issues, todos/fixmes and diag nit

[JonasToth]

In D45444#1189262, @aaron.ballman wrote:

However, I'm wondering how this should integrate with other const-correctness efforts like readability-non-const-parameter?

I think this check/functionality will kinda replace the readability-non-const-parameter check. The readability check does not a full const-analysis too and i think only works on pointers or sth like this.
Maybe the check name will still exist, but use the ExprMutAnalyzer or it will become an alias to this with special configuration.
I would like to add support for marking methods const plus the ability for code transformation. Currently looking into clang-refactor framework to implement general refactoring primitives necessary for that.
In general its probably better to have one check, that handles all const issues.

Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?

I will prepare a report for this tomorrow. Currently the LLVM builds take very long on my laptop :(

[JonasToth]

For reference, here is the output for llvm/lib.

const_correctness_2_llvm_lib_references1 MBDownload

const_correctness_2_llvm_lib_values_pointers18 MBDownload

Things i noticed:

• lambdas are warned as potential const, i think they should be excluded for the values
• for-loops that initialize two values (usually the start and end-iterator) are correctly diagnosed, but the diagnosis might be misleading. Especially because there is no way to have a const-iterator initialized together with the changing iterator.

for (auto begin = vec.begin(), end = vec.end(); ...)

• there seems to be a false positive with array-to-pointer decay. ExprMutAnalyzer does think of it, but maybe there is a bug in it.
• pointer diagnostics don't seem to worker (pointer-as-value). The test-case does work, This must be analyzed further
• there was a bug in the check method, where AnalyzeValues == 0 did imply no analysis is done.

Given the volume of diagnostics for the value-case i did not investigate many cases manually. I think it makes sense to strive for code-transformation and check if the code still compiles.

References on the other hand seem to function correctly, and most references are single-var-decls, too. That means code transformation is very simple and therefor an easy target to check functionality.
References and values are in general treated the same, so it might give a hint about the performance for values.

[shuaiwang]

• there seems to be a false positive with array-to-pointer decay. ExprMutAnalyzer does think of it, but maybe there is a bug in it.

Could you give a concrete example of this?

[JonasToth]

Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

[JonasToth]

• fix bug with AnalyzeValues==false skipping whole check, adjust test code to 'const' instead of const

[shuaiwang]

In D45444#1191874, @JonasToth wrote:

Could you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

Looks like related to template.
the exact type of num depends on Float
and the CallExpr is not even resolved because we don't know the type of the arguments yet (adl, overload resolution, etc.)
So the AST doesn't contain any array to pointer decay.

I guess we should suppress the check in such cases.

[JonasToth]

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Am 10.08.2018 um 22:12 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45444#1191874, @JonasToth wrote:

lCould you give a concrete example of this?

vi llvm/lib/Demangle/ItaniumDemangle.cpp +1762

/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1762:7: warning:
variable 'num' of type 'char [FloatData<Float>::max_demangled_size]' can
be declared 'const' [cppcoreguidelines-const-correctness]
      char num[FloatData<Float>::max_demangled_size] = {0};
     ^
/home/jonas/opt/llvm/lib/Demangle/ItaniumDemangle.cpp:1763:7: warning:
variable 'n' of type 'int' can be declared 'const'
[cppcoreguidelines-const-correctness]
      int n = snprintf(num, sizeof(num), FloatData<Float>::spec, value);
     ^

Looks like related to template.
the exact type of num depends on Float
and the CallExpr is not even resolved because we don't know the type of the arguments yet (adl, overload resolution, etc.)
So the AST doesn't contain any array to pointer decay.

I guess we should suppress the check in such cases.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• explicitly ignore lambdas in VarDecls

[shuaiwang]

In D45444#1196271, @JonasToth wrote:

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Agreed :)
I'll create a diff handling various template related cases in ExprMutationAnalyzer.

[JonasToth]

Thank you very much :)

Am 12.08.2018 um 00:17 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D45444#1196271, @JonasToth wrote:

Always the same with the templates ;) So uninstantiated templates should
just be ignored.

I think it would be better to have it in the ExprMutAnalyzer, because
that part can not decide on const-ness. Fixing it here would just
circumvent the problem but not fix it, would you agree?

Agreed :)
I'll create a diff handling various template related cases in ExprMutationAnalyzer.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• get up to date

[JonasToth]

• ignore lambdas properly

[JonasToth]

llvm_lib_references.out486 KBDownload

llvm_lib_pointer_values.out9 MBDownload

llvm_lib_values.out12 MBDownload

Here are results for llvm/lib with the current version.

I could not find any outstanding issues right now. I feel that the check needs transformation capabilties. Then i would try to produce invalid const transformations to find false positives. Spoting false negatives is harder though.

[JonasToth]

• update to ExprMutAnalyzer living in clang now

[xbolva00]

Yeah, it would be super useful if Clang can add const to all places where possible :) love this work, great!

[JonasToth]

Soonish it might be able to do so ;)

Am 14.09.2018 um 17:13 schrieb Dávid Bolvanský via Phabricator:

xbolva00 added a comment.

Yeah, it would be super useful if Clang can add const to all places where possible :) love this work, great!

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• fix actually use clang-analyses correctly

[xbolva00]

Are you still looking at this?

[JonasToth]

I am, but this revision relies on support on the analysis in the
frontend. This seems currently stalled as the developer there seems busy
with other stuff.

Once i implement transofmration-capabilites i will evaluate if there are
any false-positives that would break. Once that is done it might still
land even if there is no frontend progress.

Right now we are aware of false-negatives in the const-ness detection,
that should not influence this check though. And of course obscure
(template) cases might still be buggy.

Am 31.10.18 um 20:33 schrieb Dávid Bolvanský via Phabricator:

xbolva00 added a comment.

Are you still looking at this?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

• Merge branch 'master' into check_const

[JonasToth]

• avoid bitrot

[tsdgeos]

Would this warn with stuff like

double borderWidth;
[... code that doesn't use borderWidth ...]
borderWidth = border->getWidth(); 
[... code that reads borderWidth ...]

[JonasToth]

In D45444#1685995, @tsdgeos wrote:

Would this warn with stuff like

double borderWidth;
[... code that doesn't use borderWidth ...]
borderWidth = border->getWidth(); 
[... code that reads borderWidth ...]

Warn in what sense? That borderWidth could be initialized early and then be const?
No. It would only suggest const (if configured to make values const) when borderWidth is initialized and then untouched.

[JonasToth]

• update license
• rebase to master and patch for adding const

[0x8000-0000]

Thank you for rebasing on current master.

I have ran it today on our code base and found three classes of false positives:

1. Writing to a bitfield of a struct. The struct still is suggested it should be const.
2. Using a variable with an ostream extraction; like "int foo; cin >> foo;", except it was a template on the stream instance.
3. In a for loop, with a somewhat strange pair (for auto [foo, bar] = std::pair {}; foo < big_foo; ++ foo).

Let me know if you can't create test cases from these descriptions and I can try to extract the code.

[JonasToth]

In D45444#1766156, @0x8000-0000 wrote:

Thank you for rebasing on current master.

I have ran it today on our code base and found three classes of false positives:

1. Writing to a bitfield of a struct. The struct still is suggested it should be const.
2. Using a variable with an ostream extraction; like "int foo; cin >> foo;", except it was a template on the stream instance.
3. In a for loop, with a somewhat strange pair (for auto [foo, bar] = std::pair {}; foo < big_foo; ++ foo).

Let me know if you can't create test cases from these descriptions and I can try to extract the code.

Thank you for testing, I would appreciate if you test later versions, too!
I will focus on landing the utility function for adding const first. I noticed false positives in LLVM as well, I try to reproduce them all and fix/workaround the issues.

[0x8000-0000]

In D45444#1766852, @JonasToth wrote:

Thank you for testing, I would appreciate if you test later versions, too!
I will focus on landing the utility function for adding const first. I noticed false positives in LLVM as well, I try to reproduce them all and fix/workaround the issues.

I will test it as often as you rebase it :) Thank you for the useful tool!

Are you going to keep updating it on https://github.com/JonasToth/llvm-project/commits/feature_check_const ?

[JonasToth]

Are you going to keep updating it on https://github.com/JonasToth/llvm-project/commits/feature_check_const ?

Yup, thats my backup if something goes wrong and to synchronize my pcs. :)

[JonasToth]

Abonding this revision in favor of: https://reviews.llvm.org/D54943
It doesn't make sense to have both open at the same time, as the const-transformation happens in its own patch and no heavy lifting is done here.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidGotoCheck.cpp
+  ConstCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp

clang-tidy/cppcoreguidelines/ConstCheck.h

+//===--- ConstCheck.h - clang-tidy-------------------------------*- C++ -*-===//
+//
+//                     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_CPPCOREGUIDELINES_CONSTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/ExprMutationAnalyzer.h"
+
+namespace clang {
+namespace tidy {
+
+namespace utils {
+class ExprMutationAnalyzer;
+}
+
+namespace cppcoreguidelines {
+
+/// This check warns for every variable, that could be declared as const, but
+/// isn't.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-const.html
+class ConstCheck : public ClangTidyCheck {
+public:
+  ConstCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        AnalyzeValues(Options.get("AnalyzeValues", 1)),
+        AnalyzeReferences(Options.get("AnalyzeReferences", 1)),
+        WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void registerScope(const CompoundStmt *Scope, ASTContext *Context);
+  bool usedNonConst(const VarDecl *Variable, const CompoundStmt *Scope,
+                    const ast_matchers::MatchFinder::MatchResult &Result);
+
+  using MutationAnalyzer = std::unique_ptr<utils::ExprMutationAnalyzer>;
+  llvm::DenseMap<const CompoundStmt *, MutationAnalyzer> Scopes;
+
+  const bool AnalyzeValues;
+  const bool AnalyzeReferences;
+  const bool WarnPointersAsValues;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_CONSTCHECK_H

clang-tidy/cppcoreguidelines/ConstCheck.cpp

+//===--- ConstCheck.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 "ConstCheck.h"
+// #include "../utils/ASTUtils.h"
+// #include "../utils/DeclRefExprUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include <iostream>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/*
+ * General Thoughts
+ * ================
+ *
+ * For now: Only local variables are considered. Globals/namespace variables,
+ * paramters and class members are not analyzed.
+ * Parameters have a check already: readability-non-const-parameter
+ *
+ *
+ * Handle = either a pointer or reference
+ * Value  = everything else (Type variable_name;)
+ *
+ * Value Semantic
+ * ==============
+ *  - it is neither global nor namespace level                        + CHECK
+ *  - it never gets assigned to after initialization                  + CHECK
+ *    -> every uninitialized variable can not be const                + CHECK
+ *  - no non-const handle is created with it                          + CHECK
+ *    - no non-const pointer from it                                  + CHECK
+ *    - no non-const pointer argument                                 + CHECK
+ *    - no non-const reference from it                                + CHECK
+ *    - no non-const reference argument                               + CHECK
+ *    - no non-const capture by reference in a lambda                 + CHECK
+ *  - it is not returned as non-const handle from a function          + CHECK
+ *  - it address is not assigned to an out pointer parameter          + CHECK
+ *
+ * primitive Builtins
+ * ----------------
+ *  - it is not modified with an operator (++i,i++,--i,i--)           + CHECK
+ *  - it is not modified with an operator-assignment                  + CHECK
+ *
+ * objects
+ * -------
+ *  - there is no non-const access to a member
+ *  - there is no call to a non-const method                          + CHECK
+ *  - there is no call to an non-const overloaded operator            + CHECK
+ *  - there is no non-const iterator created from this type           + CHECK
+ *    (std::begin and friends)
+ *
+ * arrays
+ * ------
+ *  - there is no non-const operator[] access                         + CHECK
+ *  - there is no non-const handle creation of one of the elements    + CHECK
+ *  - there is no non-const iterator created from this type           + CHECK
+ *    (std::begin and friends)
+ *
+ * templated variables
+ * -------------------
+ *  - one can not reason about templated variables, because every sensible
+ *    operation is overloadable and different instantiations will result
+ *    in types with different const-properties.
+ *  - Example: operator+(T& lhs, T& rhs) -> modification might occur for this
+ * type
+ *    -> this fordbids `val = val1 + val2` val1 and val2 to be const
+ *  - only concepts give possibility to infer constness of templated variables
+ *
+ * Handle Semantic
+ * ===============
+ *  - modification of the pointee prohibit constness
+ *  - Handles follow the typ of the pointee
+ *
+ *  - no assignment to the target of the handle
+ *
+ * pointers
+ * --------
+ *  - match both for value and handle semantic
+ *
+ * references
+ * ----------
+ *  - only handle semantic applies
+ *  - references to templated types suffer from the same problems as templated
+ *    values
+ *
+ * forwarding reference
+ * --------------------
+ *  - same as references?
+ *
+ * Implementation strategy
+ * =======================
+ *
+ *  - Register every declared local variable/constant with value semantic.
+ *    (pointers, values)
+ *    Store if they can be made const.
+ *    (const int i = 10 : no,
+ *     int *const = &i  : no,
+ *     int i = 10       : yes, -> const int i = 10
+ *     int *p_i = &i    : yes, -> int *const p_i = &i)
+ *  - Register every declared local variable/constant with handle semantic.
+ *    (pointers, references)
+ *    Store if they can be made const, meaning if they can be a const target
+ *    (const int *cp_i = &i : no,
+ *     const int &cr_i = i  : no,
+ *     int *p_i = &i        : yes, -> const int *p_i = &i
+ *     int &r_i = i         : yes, -> const int &r_i = i)
+ *  - Keep 2 dictionaries for values and handles
+ *
+ *  - Match operations/events that forbid values to be const -> mark then 'no'
+ *  - Match operations/events that forbid handles to be const -> mark then 'no'
+ *
+ *  - once the translation unit is finished, determine what can be const, by
+ *    just iterating over all keys and check if they map to 'true'.
+ *    - values that can be const -> emit warning for their type and name
+ *    - handles that can be const -> emit warning for the pointee type and name
+ *    - ignore the rest
+ *
+ */
+
+namespace {
+AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); }
+} // namespace
+
+void ConstCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AnalyzeValues", AnalyzeValues);
+  Options.store(Opts, "AnalyzeReferences", AnalyzeReferences);
+  Options.store(Opts, "WarnPointersAsValues", WarnPointersAsValues);
+}
+
+void ConstCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  const auto ConstType = hasType(isConstQualified());
+  const auto ConstReference = hasType(references(isConstQualified()));
+  const auto TemplateType = anyOf(hasType(templateTypeParmType()),
+                                  hasType(substTemplateTypeParmType()));
+
+  // Match local variables, that could be const.
+  // Example: `int i = 10`, `int i` (will be used if program is correct)
+  const auto LocalValDecl = varDecl(allOf(
+      isLocal(), hasInitializer(anything()), unless(ConstType),
+      unless(ConstReference), unless(TemplateType), unless(isImplicit())));
+
+  // Match the function scope for which the analysis of all local variables
+  // shall be run.
+  const auto FunctionScope =
+      functionDecl(allOf(hasBody(compoundStmt().bind("scope")),
+                         findAll(LocalValDecl.bind("new-local-value"))));
+  Finder->addMatcher(FunctionScope, this);
+}
+
+void ConstCheck::check(const MatchFinder::MatchResult &Result) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  const auto *Scope = Result.Nodes.getNodeAs<CompoundStmt>("scope");
+  registerScope(Scope, Result.Context);
+
+  const auto *Variable = Result.Nodes.getNodeAs<VarDecl>("new-local-value");
+  assert(Variable && "Did not match local variable definition");
+
+  if (Variable->getType()->isPointerType() && !WarnPointersAsValues)
+    return;
+
+  if (Variable->getType()->isReferenceType() && !AnalyzeReferences)
+    return;
+
+  if (usedNonConst(Variable, Scope, Result) || !AnalyzeValues)
+    return;
+
+  diag(Variable->getLocStart(), "variable %0 of type %1 can be declared const")
+      << Variable << Variable->getType();
+#if 0
+  if (const auto *Variable =
+          Result.Nodes.getNodeAs<VarDecl>("new-local-handle")) {
+    assert(AnalyzeHandles && "Matched local handle without analyzing them");
+
+    if (usedNonConst(Variable, Scope, Result))
+      return;
+
+    // if (!utils::decl_ref_expr::isOnlyUsedAsConst(*Variable, *Scope,
+    // *Result.Context))
+    // return;
+
+    // Differentiate between pointers and references.
+    QualType HandleType = Variable->getType();
+    if (HandleType->isReferenceType())
+      diag(Variable->getLocStart(),
+           "reference variable %0 of type %1 can be declared const")
+          << Variable << HandleType;
+    else if (HandleType->isPointerType())
+      diag(Variable->getLocStart(),
+           "pointer variable %0 of type %1 can be declared const")
+          << Variable << HandleType;
+    else
+      llvm_unreachable("Expected handle type");
+  }
+#endif
+}
+
+void ConstCheck::registerScope(const CompoundStmt *Scope, ASTContext *Context) {
+  if (Scopes.find(Scope) == Scopes.end()) {
+    Scopes.insert(std::make_pair(
+        Scope, llvm::make_unique<utils::ExprMutationAnalyzer>(Scope, Context)));
+  }
+}
+
+bool ConstCheck::usedNonConst(const VarDecl *Variable,
+                              const CompoundStmt *Scope,
+                              const MatchFinder::MatchResult &Result) {
+
+  // find usage of that variable
+  // if a declRefExpr is found -> analyze
+  // else -> constant
+  const auto Usage = match(
+      findAll(declRefExpr(hasDeclaration(equalsNode(Variable))).bind("use")),
+      *Scope, *Result.Context);
+  const auto *UseExpr = selectFirst<Expr>("use", Usage);
+
+  // The declared variables was used in non-const conserving way and can not
+  // be declared as const.
+  if (UseExpr && Scopes[Scope]->isMutated(UseExpr)) {
+    // diag(UseExpr->getLocStart(), "Investigating starting with this use",
+    // DiagnosticIDs::Note);
+    return true;
+  }
+  return false;
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "AvoidGotoCheck.h"
+#include "ConstCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"

 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
+    CheckFactories.registerCheck<ConstCheck>(
+        "cppcoreguidelines-const");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
     CheckFactories.registerCheck<OwningMemoryCheck>(
         "cppcoreguidelines-owning-memory");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(

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 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 *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) {
+    const auto Refs = match(
+        findAll(declRefExpr(to(equalsNode(DeclNodes.getNodeAs<Decl>("decl"))))
+                    .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) {
+  // 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(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
+

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`cppcoreguidelines-const
+  <clang-tidy/checks/cppcoreguidelines-const>` check
+
+  FIXME: add release notes.
+
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
 
 - New module ``portability``.
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
 - New :doc:`android-comparison-in-temp-failure-retry

docs/clang-tidy/checks/cppcoreguidelines-const.rst

+.. title:: clang-tidy - cppcoreguidelines-const
+
+cppcoreguidelines-const
+=======================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

docs/clang-tidy/checks/list.rst

.. toctree::
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc50-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
+   cppcoreguidelines-const
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-no-malloc
    cppcoreguidelines-owning-memory
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast

test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// RUN: --
+
+void potential_const_pointer() {
+  double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *p_local0 = &np_local0[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared const
+}

test/clang-tidy/cppcoreguidelines-const-values.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// ------- Provide test samples for primitive builtins ---------
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  int np_local0 = 42;
+
+  {
+    int p_local1 = 42;
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared const
+    np_local0 *= 2;
+  }
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = &p_local0;
+  int *const p1_p_local0 = &p_local0;
+
+  // 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;
+
+  int np_local1 = 42;
+  function_inout_pointer(&np_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = &np_local2; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  const int *const p0_p_local1 = &p_local1;
+
+  int p_local2 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(&p_local2);
+}
+
+void function_inout_ref(int &inout);
+void function_in_ref(const int &in);
+
+void some_reference_taking() {
+  // 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;
+  const int &r2_np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int &r0_p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return &np_local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return &p_local1;
+}
+
+double &non_const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 42.42;
+  return np_local0;
+}
+
+const double &const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return p_local1;
+}
+
+double *&return_non_const_pointer_ref() {
+  double *np_local0 = nullptr;
+  return np_local0;
+}
+
+void overloaded_arguments(const int &in);
+void overloaded_arguments(int &inout);
+void overloaded_arguments(const int *in);
+void overloaded_arguments(int *inout);
+
+void function_calling() {
+  int np_local0 = 42;
+  overloaded_arguments(np_local0);
+
+  const int np_local1 = 42;
+  overloaded_arguments(np_local1);
+
+  int np_local2 = 42;
+  overloaded_arguments(&np_local2);
+
+  const int np_local3 = 42;
+  overloaded_arguments(&np_local3);
+}
+
+template <typename T>
+void define_locals(T np_arg0, T &np_arg1, int np_arg2) {
+  T np_local0 = 0;
+  np_local0 += np_arg0 * np_arg1;
+
+  T np_local1 = 42;
+  np_local0 += np_local1;
+
+  // Used as argument to an overloaded function with const and non-const.
+  T np_local2 = 42;
+  overloaded_arguments(np_local2);
+
+  int np_local4 = 42;
+  // non-template values are ok still.
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  np_local4 += p_local0;
+}
+
+void template_instantiation() {
+  const int np_local0 = 42;
+  int np_local1 = 42;
+
+  define_locals(np_local0, np_local1, np_local0);
+  define_locals(np_local1, np_local1, np_local1);
+}
+
+struct ConstNonConstClass {
+  ConstNonConstClass();
+  ConstNonConstClass(double &np_local0);
+  double nonConstMethod() {}
+  double constMethod() const {}
+  double modifyingMethod(double &np_arg0) const;
+
+  double NonConstMember;
+  const double ConstMember;
+
+  double &NonConstMemberRef;
+  const double &ConstMemberRef;
+
+  double *NonConstMemberPtr;
+  const double *ConstMemberPtr;
+};
+
+void direct_class_access() {
+  // 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();
+
+  ConstNonConstClass p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass' can be declared const
+  p_local0.constMethod();
+
+  ConstNonConstClass p_local1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'ConstNonConstClass' can be declared const
+  double np_local1;
+  p_local1.modifyingMethod(np_local1);
+
+  double np_local2;
+  ConstNonConstClass p_local2(np_local2);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'ConstNonConstClass' can be declared const
+
+  ConstNonConstClass np_local3;
+  np_local3.NonConstMember = 42.;
+
+  ConstNonConstClass np_local4;
+  np_local4.NonConstMemberRef = 42.;
+
+  ConstNonConstClass p_local3;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'ConstNonConstClass' can be declared const
+  const double val0 = p_local3.NonConstMember;
+  const double val1 = p_local3.NonConstMemberRef;
+  const double val2 = *p_local3.NonConstMemberPtr;
+
+  ConstNonConstClass p_local4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'ConstNonConstClass' can be declared const
+  *np_local4.NonConstMemberPtr = 42.;
+}
+
+void class_access_array() {
+  // 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();
+
+  ConstNonConstClass p_local0[2];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'ConstNonConstClass [2]' can be declared const
+  p_local0[0].constMethod();
+  np_local0[1].constMethod();
+}
+
+struct OperatorsAsConstAsPossible {
+  OperatorsAsConstAsPossible &operator+=(const OperatorsAsConstAsPossible &rhs);
+  OperatorsAsConstAsPossible operator+(const OperatorsAsConstAsPossible &rhs) const;
+};
+
+struct NonConstOperators {
+};
+NonConstOperators operator+(NonConstOperators &lhs, NonConstOperators &rhs);
+NonConstOperators operator-(NonConstOperators lhs, NonConstOperators rhs);
+
+void internal_operator_calls() {
+  OperatorsAsConstAsPossible np_local0;
+  OperatorsAsConstAsPossible np_local1;
+  OperatorsAsConstAsPossible p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'OperatorsAsConstAsPossible' can be declared const
+  OperatorsAsConstAsPossible p_local1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'OperatorsAsConstAsPossible' can be declared const
+
+  np_local0 += p_local0;
+  np_local1 = p_local0 + p_local1;
+
+  NonConstOperators np_local2;
+  NonConstOperators np_local3;
+  NonConstOperators np_local4;
+
+  np_local2 = np_local3 + np_local4;
+
+  NonConstOperators p_local2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'NonConstOperators' can be declared const
+  NonConstOperators p_local3 = p_local2 - p_local2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'NonConstOperators' can be declared const
+}
+
+struct MyVector {
+  double *begin();
+  const double *begin() const;
+
+  double *end();
+  const double *end() const;
+
+  double &operator[](int index);
+  double operator[](int index) const;
+
+  double values[100];
+};
+
+void vector_usage() {
+  double np_local0[10];
+  np_local0[5] = 42.;
+
+  MyVector np_local1;
+  np_local1[5] = 42.;
+
+  double p_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double [10]' can be declared const
+  double p_local1 = p_local0[5];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+
+  // The following subscript calls suprisingly choose the non-const operator
+  // version.
+  MyVector np_local2;
+  double p_local2 = np_local2[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'double' can be declared const
+
+  MyVector np_local3;
+  const double np_local4 = np_local3[42];
+
+  // This subscript results in const overloaded operator.
+  const MyVector np_local5{};
+  double p_local3 = np_local5[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double' can be declared const
+}
+
+void const_handle(const double &np_local0);
+void const_handle(const double *np_local0);
+
+void non_const_handle(double &np_local0);
+void non_const_handle(double *np_local0);
+
+void handle_from_array() {
+  // Non-const handle from non-const array forbids declaring the array as const
+  double np_local0[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *p_local0 = &np_local0[1]; // Could be `double *const`, but warning deactivated by default
+
+  double np_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double &non_const_ref = np_local1[1];
+  non_const_ref = 42.;
+
+  double np_local2[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  double *np_local3;
+  np_local3 = &np_local2[5];
+
+  double np_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  non_const_handle(np_local4[2]);
+  double np_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  non_const_handle(&np_local5[2]);
+
+  // Constant handles are ok
+  double p_local1[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double [10]' can be declared const
+  const double *p_local2 = &p_local1[2]; // Could be `const double *const`, but warning deactivated by default
+
+  double p_local3[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'double [10]' can be declared const
+  const double &const_ref = p_local3[2];
+
+  double p_local4[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local4' of type 'double [10]' can be declared const
+  const double *const_ptr;
+  const_ptr = &p_local4[2];
+
+  double p_local5[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local5' of type 'double [10]' can be declared const
+  const_handle(p_local5[2]);
+  double p_local6[10] = {0., 1., 2., 3., 4., 5., 6., 7., 8., 9.};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local6' of type 'double [10]' can be declared const
+  const_handle(&p_local6[2]);
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  for (int &non_const_ref : np_local0) {
+    non_const_ref = 42;
+  }
+
+  int np_local1[2] = {1, 2};
+  for (auto &non_const_ref : np_local1) {
+    non_const_ref = 43;
+  }
+
+  int np_local2[2] = {1, 2};
+  for (auto &&non_const_ref : np_local2) {
+    non_const_ref = 44;
+  }
+
+  // FIXME the warning message is suboptimal. It could be defined as
+  // `int *const np_local3[2]` because the pointers are not reseated.
+  // But this is not easily deducable from the warning.
+  int *np_local3[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared const
+  for (int *non_const_ptr : np_local3) {
+    *non_const_ptr = 45;
+  }
+
+  // FIXME same as above, but silenced
+  int *const np_local4[2] = {&np_local0[0], &np_local0[1]};
+  for (auto *non_const_ptr : np_local4) {
+    *non_const_ptr = 46;
+  }
+
+  int p_local0[2] = {1, 2};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int [2]' can be declared const
+  for (int value : p_local0) {
+    // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'value' of type 'int' can be declared const
+  }
+
+  int p_local1[2] = {1, 2};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int [2]' can be declared const
+  for (const int &const_ref : p_local1) {
+  }
+
+  int *p_local2[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared const
+  for (const int *con_ptr : p_local2) {
+  }
+
+  int *p_local3[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared const
+  for (const auto *con_ptr : p_local3) {
+  }
+}
+
+inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
+
+struct Value {
+};
+void placement_new() {
+  Value Mem;
+  Value *V = new (&Mem) Value;
+}
+
+struct ModifyingConversion {
+  operator int() { return 15; }
+};
+struct NonModifyingConversion {
+  operator int() const { return 15; }
+};
+void conversion_operators() {
+  ModifyingConversion np_local0;
+  NonModifyingConversion p_local0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'NonModifyingConversion' can be declared const
+
+  int np_local1 = np_local0;
+  np_local1 = p_local0;
+}
+
+void casts() {
+  decltype(sizeof(void *)) p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'decltype(sizeof(void *))' (aka 'unsigned long') can be declared const
+  auto np_local0 = reinterpret_cast<void *>(p_local0);
+  np_local0 = nullptr;
+
+  int p_local1 = 43;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  short p_local2 = static_cast<short>(p_local1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'short' can be declared const
+
+  int np_local1 = p_local2;
+  int &np_local2 = static_cast<int &>(np_local1);
+  np_local2 = 5;
+}
+
+void ternary_operator() {
+  int np_local0 = 1, np_local1 = 2;
+  int &np_local2 = true ? np_local0 : np_local1;
+  np_local2 = 2;
+
+  int p_local0 = 3, np_local3 = 5;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int &np_local4 = true ? p_local0 : ++np_local3;
+
+  // 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;
+}

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()));
+}
+
+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()));
+}
+
+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"));
+}
+
+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()));
+}
+
+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)"));
+}
+
+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()));
+}
+
+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