[clang] Fix-it hint for `++this` -> `++*this` when deref is modifiable #94159

Rajveer100 · 2024-06-02T13:08:29Z

Resolves #93066

llvmbot · 2024-06-02T13:08:59Z

@llvm/pr-subscribers-clang

Author: Rajveer Singh Bharadwaj (Rajveer100)

Changes

Resolves #93066

Full diff: https://github.com/llvm/llvm-project/pull/94159.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/lib/Sema/SemaExpr.cpp (+13-2)
(added) clang/test/Sema/debug-93066.cpp (+14)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 270b0a1e01307..0f5445296e45f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8777,6 +8777,9 @@ def err_typecheck_incomplete_type_not_modifiable_lvalue : Error<
 def err_typecheck_lvalue_casts_not_supported : Error<
   "assignment to cast is illegal, lvalue casts are not supported">;
 
+def note_typecheck_expression_not_modifiable_lvalue : Note<
+  "dereference the pointer to modify">; 
+
 def err_typecheck_duplicate_vector_components_not_mlvalue : Error<
   "vector is not assignable (contains duplicate components)">;
 def err_block_decl_ref_not_modifiable_lvalue : Error<
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ff9c5ead36dcf..39e7962fcb2a6 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -13587,10 +13587,21 @@ static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
   SourceRange Assign;
   if (Loc != OrigLoc)
     Assign = SourceRange(OrigLoc, OrigLoc);
-  if (NeedType)
+  if (NeedType) {
     S.Diag(Loc, DiagID) << E->getType() << E->getSourceRange() << Assign;
-  else
+  } else {
+    ExprResult Deref;
+    unsigned FixitDiagID = 0;
+    {
+      Sema::TentativeAnalysisScope Trap(S);
+      Deref = S.ActOnUnaryOp(S.getCurScope(), Loc, tok::star, E);
+    }
     S.Diag(Loc, DiagID) << E->getSourceRange() << Assign;
+    if (Deref.isUsable() && Deref.get()->isModifiableLvalue(S.Context, &Loc) == Expr::MLV_Valid) {
+      FixitDiagID = diag::note_typecheck_expression_not_modifiable_lvalue;
+      S.Diag(Loc, FixitDiagID) << E->getSourceRange() << Assign;
+    }
+  }
   return true;
 }
 
diff --git a/clang/test/Sema/debug-93066.cpp b/clang/test/Sema/debug-93066.cpp
new file mode 100644
index 0000000000000..7abedcd0d4d80
--- /dev/null
+++ b/clang/test/Sema/debug-93066.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify %s
+
+struct S {
+  void f() {
+    ++this;
+    // expected-error@-1 {{expression is not assignable}}
+    // expected-note@-2 {{dereference the pointer to modify}}
+  }
+
+  void g() const {
+    ++this;
+    // expected-error@-1 {{expression is not assignable}}
+  }
+};

Rajveer100 · 2024-06-02T13:09:03Z

cc @Sirraide

github-actions · 2024-06-02T13:12:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Sirraide

In addition to what I’ve pointed out below, there are a few more things to be done:

This needs a release note (in clang/docs/ReleaseNotes.rst, probably under the ‘Improvements to Clang’s Diagnostics’ section).
I find it hard to believe that the test you added is the only thing affected by this; it’s very likely that there are other tests that are failing now because you also need to add the diagnostic there; please run the Clang test suite locally (e.g. ninja check-clang if you’re using Ninja as your generator) and fix any tests that this is probably breaking.
Some more tests, including tests involving templates, would also be a good idea.

clang/lib/Sema/SemaExpr.cpp

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/test/Sema/debug-93066.cpp

Rajveer100 · 2024-06-03T12:01:31Z

Tests which fail currently:

Clang :: Sema/exprs.c
Clang :: Sema/va_arg_x86_32.c
Clang :: SemaObjCXX/sel-address.mm

Also, what do you think about the fix-it hint wording for errors such as assignment to cast is illegal, lvalue casts are not supported?

Sirraide · 2024-06-03T13:23:43Z

Also, what do you think about the fix-it hint wording for errors such as assignment to cast is illegal, lvalue casts are not supported?

Don’t remember what that was for off the top of my head, but just the same fix-it should be fine imo

clang/lib/Sema/SemaExpr.cpp

Rajveer100 · 2024-06-06T12:59:36Z

@Sirraide
Let me know if any further changes are needed. All tests should pass once CI finishes.

clang/lib/Sema/SemaExpr.cpp

clang/test/SemaObjCXX/sel-address.mm

Sirraide

This is already looking better.

(Also, for the record, this is just a me thing and not official policy or anything, but I’d find it easier if you didn’t force-push because then I’d have an easier time telling what’s changed in recent commits; everything is going to get squashed into a single commit on merge anyway, so individual commits to the pr don’t have to be perfect or anything.)

clang/lib/Sema/SemaExpr.cpp

Rajveer100 · 2024-06-08T13:50:29Z

I have added a FIXME for now regarding the Obj-C part.

The function change is also done.

clang/include/clang/Basic/DiagnosticSemaKinds.td

Sirraide

The implementation looks fine, but a few more tests involving templates would be nice to see what this does with dependent types, e.g.

template <typename T>
void f(T& t) {
    ++t;
}

void g() {
    int* a;
    int* const b = a;
    const int* const c = a;
    f(a); // Ok.
    f(b); // Error and fixit.
    f(c); // Error, but no fixit.
}

I’d expect an error to be reported at instantiation time only; I doubt that there will be any complications here, but more tests are never a bad idea.

clang/test/SemaObjCXX/sel-address.mm

Sirraide

Two last comments, but other than that this LGTM.

clang/test/Sema/debug-93066.cpp

clang/lib/Sema/SemaExpr.cpp

Resolves llvm#93066

Rajveer100 · 2024-06-11T18:57:16Z

I have done the last few changes as well.

Regarding the force-push, I will try to do single commits and later squash them!

Sirraide · 2024-06-11T19:06:30Z

I will try to do single commits and later squash them!

You don’t even need to squash them yourself; Github does that automatically because we use ‘squash and merge’.

Rajveer100 · 2024-06-12T05:58:43Z

@Sirraide
Can you land this for me?

Sirraide · 2024-06-13T06:51:56Z

@Sirraide Can you land this for me?

Sure; looks like everything is fine.

zygoloid

I have some significant concerns about the approach of this PR. It seems to be producing notes that in a lot of cases suggest doing something that doesn't make sense or won't compile, and tentatively building a *p expression is unsafe.

Can we revert and consider other approaches?

zygoloid · 2024-06-17T19:25:15Z

clang/lib/Sema/SemaExpr.cpp

+  {
+    Sema::TentativeAnalysisScope Trap(S);
+    Deref = S.ActOnUnaryOp(S.getCurScope(), Loc, tok::star, TE);
+  }


It's not really feasible to use a TentativeAnalysisScope for this. Here are three reasons:

If ActOnUnaryOp produces an "immediate context" diagnostic (the common case), such as a warning, then the note you produce below will be attached to that diagnostic instead of to the original diagnostic, so your note will be suppressed.

If ActOnUnaryOp triggers template instantiation, then TentativeAnalysisScope will not suppress diagnostics outside of the immediate context, potentially leading to additional spurious diagnostics being produced.

If this function is used to attach a note to a warning / extension diagnostic rather than an error (which doesn't appear to be the case right now, but it seems plausible that someone would make such a change to CheckForModifiableLValue in the future), then the diagnostics produced by (2) would lead to rejecting a valid program.

Checking for an overloaded operator* seems overly complicated here, so I think the right thing to test here is just whether the expression has pointer type, and the pointee type is an object type (not void or a function type).

zygoloid · 2024-06-17T19:27:46Z

clang/test/Sema/debug-93066.cpp

+struct S {
+  void f() {
+    ++this; // expected-error {{expression is not assignable}}
+    // expected-note@-1 {{add '*' to dereference it}}


This is a bad diagnostic change: adding the * doesn't help.

That’s true. Looking at this again, I’m candidly a bit confused that this seemed fine to me at the time...

zygoloid · 2024-06-17T19:27:51Z

clang/test/Sema/debug-93066.cpp

+  }
+
+  void g() const {
+    ++this; // expected-error {{expression is not assignable}}


zygoloid · 2024-06-17T19:30:38Z

clang/test/Sema/exprs.c

@@ -65,8 +65,10 @@ void test4(void) {

 void test5(int *X, float *P) {
  (float*)X = P;   // expected-error {{assignment to cast is illegal, lvalue casts are not supported}}
+  // expected-note@-1 {{add '*' to dereference it}}


This is a diagnostic regression.

zygoloid · 2024-06-17T19:30:44Z

clang/test/Sema/exprs.c

 #define FOO ((float*) X)
  FOO = P;   // expected-error {{assignment to cast is illegal, lvalue casts are not supported}}
+  // expected-note@-1 {{add '*' to dereference it}}


This is a diagnostic regression.

zygoloid · 2024-06-17T19:32:01Z

clang/lib/Sema/SemaExpr.cpp

+    S.Diag(E->getBeginLoc(),
+           diag::note_typecheck_add_deref_star_not_modifiable_lvalue)
+        << E->getSourceRange()
+        << FixItHint::CreateInsertion(E->getBeginLoc(), "*");


We shouldn't be suggesting adding a * unless that would actually make sense in context. Eg, given:

const int *const p; const int *const q; p = q;

... we should not suggest a dereference -- the problem is clearly that p is const, and adding a * doesn't help.

I suspect we simply don't have enough information to do this from within CheckForModifiableLvalue.

I suspect we simply don't have enough information to do this from within CheckForModifiableLvalue.

Yeah, my idea was that by trying to build a * expression we might be able to figure out whether that makes sense in the context, whereas just looking for e.g. pointer types would cause too many false positives (at least that’s what I thought), but it might just be that you end up with too many false positives either way...

Sirraide · 2024-06-17T19:43:30Z

Can we revert and consider other approaches?

Yeah, a lot of the points you’ve brought up make sense, and if you’re saying it’s a bad idea then I’ll trust you and revert this for now so we can think about this some more...

Also, I suppose part of the issue for me at least was that I wasn’t 100% sure about this approach either, but I also didn’t want to create even more work for other people than they already have by asking someone else to review this too... I’ll be a bit more careful with this in the future.

I also marked this as a good first issue because I thought it shouldn’t be that complicated to do this, but it seems I was wrong (this is also why I usually don’t like using that label...)

Sirraide · 2024-06-17T19:51:23Z

Checking for an overloaded operator* seems overly complicated here, so I think the right thing to test here is just whether the expression has pointer type, and the pointee type is an object type (not void or a function type).

That’s also what I was going for initially, but I thought that doing just that might lead to too many false positives...

That said, considering all the issues that using TentativeAnalysisScope seems to come with, I suppose it’s not too surprising that it’s used very sparingly (most of the uses are apparently in Open MP–related code)

zygoloid · 2024-06-17T22:45:16Z

Perhaps we can start by supplying CheckForModifiableLvalue with extra information about why we're doing the check. The current diagnostic we get for incrementing a non-modifiable lvalue:

<source>:2:3: error: cannot assign to variable 'n' with const-qualified type 'const int'
    2 |   ++n;
      |   ^ ~

... is really bad -- this isn't an assignment, it's an increment. An enum of possible reasons why we're doing the check would let us significantly improve the diagnostic here.

Sirraide · 2024-06-18T07:08:56Z

Having more context would probably help yeah. We should be able to improve that a bit even if the fix-it isn’t viable...

@zygoloid

…odifiable" (#95833) Reverts #94159 @zygoloid had some concerns about the implementation of this, which make sense to me, so I’d like to revert this for now so we can have more time to think about how to best approach this (if at all...)

Rajveer100 · 2024-06-18T08:02:31Z

@Sirraide @zygoloid
Can I be of any help to improve this?

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 2, 2024

Rajveer100 force-pushed the branch-for-issue-93066 branch 2 times, most recently from e637dc8 to 95ce40b Compare June 2, 2024 13:12

Sirraide reviewed Jun 3, 2024

View reviewed changes

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

clang/test/Sema/debug-93066.cpp Show resolved Hide resolved

Sirraide requested review from AaronBallman and cor3ntin June 3, 2024 04:08

Rajveer100 force-pushed the branch-for-issue-93066 branch from 95ce40b to 6dec67c Compare June 4, 2024 14:01

Rajveer100 requested a review from Sirraide June 4, 2024 14:01

Rajveer100 commented Jun 4, 2024

View reviewed changes

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-93066 branch from 6dec67c to 5092ffd Compare June 6, 2024 12:55

Sirraide reviewed Jun 6, 2024

View reviewed changes

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

clang/test/SemaObjCXX/sel-address.mm Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-93066 branch from 5092ffd to e38b387 Compare June 7, 2024 08:54

Rajveer100 requested a review from Sirraide June 7, 2024 12:48

Sirraide reviewed Jun 7, 2024

View reviewed changes

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-93066 branch from e38b387 to 1a9ef88 Compare June 8, 2024 13:44

Rajveer100 requested a review from Sirraide June 8, 2024 18:03

samolisov reviewed Jun 10, 2024

View reviewed changes

clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-93066 branch from 1a9ef88 to 3b25887 Compare June 10, 2024 11:52

Sirraide reviewed Jun 10, 2024

View reviewed changes

clang/test/SemaObjCXX/sel-address.mm Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-93066 branch from 3b25887 to 765176a Compare June 11, 2024 10:58

Rajveer100 requested a review from Sirraide June 11, 2024 11:00

Sirraide approved these changes Jun 11, 2024

View reviewed changes

clang/test/Sema/debug-93066.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaExpr.cpp Outdated Show resolved Hide resolved

[clang] Fix-it hint for ++this -> ++*this when deref is modifiable

bf11465

Resolves llvm#93066

Rajveer100 force-pushed the branch-for-issue-93066 branch from 765176a to bf11465 Compare June 11, 2024 18:55

Sirraide merged commit 9d7299f into llvm:main Jun 13, 2024
8 checks passed

zygoloid reviewed Jun 17, 2024

View reviewed changes

Sirraide mentioned this pull request Jun 17, 2024

Revert "[clang] Fix-it hint for ++this -> ++*this when deref is modifiable" #95833

Merged

[clang] Fix-it hint for ++this -> ++*this when deref is modifiable #94159

[clang] Fix-it hint for ++this -> ++*this when deref is modifiable #94159

Uh oh!

Conversation

Rajveer100 commented Jun 2, 2024

Uh oh!

llvmbot commented Jun 2, 2024

Uh oh!

Rajveer100 commented Jun 2, 2024

Uh oh!

github-actions bot commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rajveer100 commented Jun 3, 2024

Uh oh!

Sirraide commented Jun 3, 2024

Uh oh!

Uh oh!

Rajveer100 commented Jun 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rajveer100 commented Jun 8, 2024

Uh oh!

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Rajveer100 commented Jun 11, 2024

Uh oh!

Sirraide commented Jun 11, 2024

Uh oh!

Rajveer100 commented Jun 12, 2024

Uh oh!

Sirraide commented Jun 13, 2024

Uh oh!

Uh oh!

zygoloid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sirraide commented Jun 17, 2024

Uh oh!

Sirraide commented Jun 17, 2024

Uh oh!

zygoloid commented Jun 17, 2024

Uh oh!

[clang] Fix-it hint for `++this` -> `++*this` when deref is modifiable #94159

[clang] Fix-it hint for `++this` -> `++*this` when deref is modifiable #94159

github-actions bot commented Jun 2, 2024 •

edited

Loading