[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444