[Clang] Add a fix-it hint for `++this` -> `++*this`

[Sirraide]

There was a suggestion to add a fix-it hint that would correct ++this to ++*this following a discussion about #92439 now that we diagnose the former even in dependent contexts. From what I can tell, this would entail adding a fix-it (probably in CheckForModifiableLvalue in SemaExpr.cpp) that inserts a * before the expression, if

• it is of pointer type, and
• the expression itself is not a modifiable lvalue, and
• the pointee type is modifiable (e.g. don’t suggest ++this -> ++*this in a const member function).

Notably, doing this in CheckForModifiableLvalue would cover more cases than just ++. Possible test cases I can think of would be:

void f(int* a, int* const b, const int* const c, __UINTPTR_TYPE__ d) {
    (int*)d = 4; // Suggest a fixit; `(int*)d` is a prvalue.
    ++a;         // No fixit; this is fine.
    ++b;         // Suggest a fixit; `b` is const but `*b` isn’t.
    ++c;         // No fixit; `++c` is invalid, but so is `++*c`.
}

struct S {
    void f() { 
        // Suggest a fixit. `++*this` is probably what the user meant.
        //
        // Note: if this is in a template, then we generally won’t have
        // any way of knowing whether there even is an overload of `++` 
        // for this type, but it makes sense to assume there is because
        // it’s unlikely that a user would just try and increment it any-
        // way if that was the case.
        ++this; 
    }

    void g() const { 
        // No fixit here. Of course, one *could* define an overload of
        // `++` that is `const`, but doing so would be rather confusing
        // considering what this operator usually does, so most people
        // won’t do that and suggesting a `*` therefore would just lead
        // to a lot of false positives.
        ++this; 
    }
};

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: None (Sirraide)

Details

There was a suggestion to add a fix-it hint that would correct `++this` to `++*this` following a discussion about #92439 now that we diagnose the former even in dependent contexts. From what I can tell, this would entail adding a fix-int (probably in `CheckForModifiableLvalue` in SemaExpr.cpp) that inserts a `*` before the expression, if - it is of pointer type, and - the expression itself is not a modifiable lvalue, and - the pointee type is modifiable (i.e. don’t suggest `++this` -> `++*this` in a `const` member function).

Notably, doing this in CheckForModifiableLvalue would cover more cases than just ++. Possible test cases I can think of would be:

void f(int* a, int* const b, const int* const c, __UINTPTR_TYPE__ d) {
    (int*)d = 4; // Suggest a fixit; `(int*)d` is a prvalue.
    ++a;         // No fixit; this is fine.
    ++b;         // Suggest a fixit; `b` is const but `*b` isn’t.
    ++c;         // No fixit; `++c` is invalid, but so is `++*c`.
}

struct S {
    void f() { 
        // Suggest a fixit. `++*this` is probably what the user meant.
        //
        // Note: if this is in a template, then we generally won’t have
        // any way of knowing whether there even is an overload of `++` 
        // for this type, but it makes sense to assume there is because
        // it’s unlikely that a user would just try and increment it any-
        // way if that was the case.
        ++this; 
    }

    void g() const { 
        // No fixit here. Of course, one *could* define an overload of
        // `++` that is `const`, but doing so would be rather confusing
        // considering what this operator usually does, so most people
        // won’t do that and suggesting a `*` therefore would just lead
        // to a lot of false positives.
        ++this; 
    }
};

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (Sirraide)

Details

There was a suggestion to add a fix-it hint that would correct `++this` to `++*this` following a discussion about #92439 now that we diagnose the former even in dependent contexts. From what I can tell, this would entail adding a fix-int (probably in `CheckForModifiableLvalue` in SemaExpr.cpp) that inserts a `*` before the expression, if - it is of pointer type, and - the expression itself is not a modifiable lvalue, and - the pointee type is modifiable (i.e. don’t suggest `++this` -> `++*this` in a `const` member function).

Notably, doing this in CheckForModifiableLvalue would cover more cases than just ++. Possible test cases I can think of would be:

void f(int* a, int* const b, const int* const c, __UINTPTR_TYPE__ d) {
    (int*)d = 4; // Suggest a fixit; `(int*)d` is a prvalue.
    ++a;         // No fixit; this is fine.
    ++b;         // Suggest a fixit; `b` is const but `*b` isn’t.
    ++c;         // No fixit; `++c` is invalid, but so is `++*c`.
}

struct S {
    void f() { 
        // Suggest a fixit. `++*this` is probably what the user meant.
        //
        // Note: if this is in a template, then we generally won’t have
        // any way of knowing whether there even is an overload of `++` 
        // for this type, but it makes sense to assume there is because
        // it’s unlikely that a user would just try and increment it any-
        // way if that was the case.
        ++this; 
    }

    void g() const { 
        // No fixit here. Of course, one *could* define an overload of
        // `++` that is `const`, but doing so would be rather confusing
        // considering what this operator usually does, so most people
        // won’t do that and suggesting a `*` therefore would just lead
        // to a lot of false positives.
        ++this; 
    }
};

[Rajveer100]

@Sirraide
Any other particular insights or starters you would like to provide w.r.t the issue and as mentioned this has dependency with #92439, which I believe is the actual fix-it, whereas this provides just the hint?

[Sirraide]

Any other particular insights or starters you would like to provide w.r.t the issue and as mentioned this has dependency with 92439, which I believe is the actual fix-it, whereas this provides just the hint?

There was some amount of confusion around that issue. Diagnosing the cases above has already been implemented, so we’re only missing the fix-it hint here.

[Sirraide]

Diagnosing the cases above has already been implemented, so we’re only missing the fix-it hint here.

That is, we already issue an error that this is wrong, but it would be nice to also tell the user how to fix it (i.e. insert * before the operand of ++).

[Rajveer100]

Sounds good!

[Rajveer100]

Under CheckForModifiableLvalue :

case Expr::MLV_ClassTemporary:
    DiagID = diag::err_typecheck_expression_not_modifiable_lvalue;
    break;

Do we use the isConstantInitializer to check the constant object part in void <some_func> const presuming I am under the right place here to throw the fix-it hint for this?

[samolisov]

I'm a new in the clang development. I've also as well as @Rajveer100 found a point where the diagnostic is emitted and assume there are two ways: extend the logic of the DiagID assignment in the CheckForModifiableLvalue function, so take into account whether this is this, whether we are in the template context, etc. The second way is to introduce another value in MLV_ and assign it in the Expr::isModifiableLvalue() method. Maybe a new option in the Expr::Classification::Kinds would be needed, CL_ThisPRValue, something like this. The new option in MLV_ will have a separate branch in the switch (IsLV) switch what simplifies the handling a bit. But it looks very ugly to extend already designed enums for every possible case in the code. So, my idea is to extend the handling of ... case Expr::MLV_ClassTemporary: in the CheckForModifiableLvalue method.

It would be great if someone was able to give a suggest and explain a bit the difference between the layers in SemaExpr and ExprClassification. Currently, it looks as a border between parsing (libAST) and semantic analysis (libSema).

I will not be able to address this issue shortly (within the following week). @Rajveer100, if you would like to manage the issue, this makes sense to mark yourself as the assignee.