Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_add_deref_star_not_modifiable_lvalue : Note<
"add '*' to dereference it">;

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<
Expand Down
24 changes: 22 additions & 2 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13273,6 +13273,23 @@ enum {
ConstUnknown, // Keep as last element
};

static void MaybeSuggestDerefFixIt(Sema &S, const Expr *E, SourceLocation Loc) {
ExprResult Deref;
Expr *TE = const_cast<Expr *>(E);
{
Sema::TentativeAnalysisScope Trap(S);
Deref = S.ActOnUnaryOp(S.getCurScope(), Loc, tok::star, TE);
}
Comment on lines +13279 to +13282
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. If ActOnUnaryOp triggers template instantiation, then TentativeAnalysisScope will not suppress diagnostics outside of the immediate context, potentially leading to additional spurious diagnostics being produced.
  3. 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).

if (Deref.isUsable() &&
Deref.get()->isModifiableLvalue(S.Context, &Loc) == Expr::MLV_Valid &&
!E->getType()->isObjCObjectPointerType()) {
S.Diag(E->getBeginLoc(),
diag::note_typecheck_add_deref_star_not_modifiable_lvalue)
<< E->getSourceRange()
<< FixItHint::CreateInsertion(E->getBeginLoc(), "*");
Comment on lines +13286 to +13289
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}
}

/// Emit the "read-only variable not assignable" error and print notes to give
/// more information about why the variable is not assignable, such as pointing
/// to the declaration of a const variable, showing that a method is const, or
Expand Down Expand Up @@ -13367,6 +13384,7 @@ static void DiagnoseConstAssignment(Sema &S, const Expr *E,
if (!DiagnosticEmitted) {
S.Diag(Loc, diag::err_typecheck_assign_const)
<< ExprRange << ConstVariable << VD << VD->getType();
MaybeSuggestDerefFixIt(S, E, Loc);
DiagnosticEmitted = true;
}
S.Diag(VD->getLocation(), diag::note_typecheck_assign_const)
Expand Down Expand Up @@ -13587,10 +13605,12 @@ 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 {
S.Diag(Loc, DiagID) << E->getSourceRange() << Assign;
MaybeSuggestDerefFixIt(S, E, Loc);
}
return true;
}

Expand Down
1 change: 1 addition & 0 deletions clang/test/C/drs/dr1xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ void dr126(void) {
*/
*object = 12; /* ok */
++object; /* expected-error {{cannot assign to variable 'object' with const-qualified type 'const IP' (aka 'int *const')}} */
/* expected-note@-1 {{add '*' to dereference it}} */
}

/* WG14 DR128: yes
Expand Down
49 changes: 49 additions & 0 deletions clang/test/Sema/debug-93066.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify %s

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

}
};

void f(int* a, int* const b, const int* const c, __UINTPTR_TYPE__ d) {
// expected-note@-1 {{variable 'b' declared const here}}
// expected-note@-2 {{variable 'c' declared const here}}
(int*)d = 4; // expected-error {{assignment to cast is illegal, lvalue casts are not supported}}
// expected-note@-1 {{add '*' to dereference it}}

++a;
++b; // expected-error {{cannot assign to variable 'b' with const-qualified type 'int *const'}}
// expected-note@-1 {{add '*' to dereference it}}
++c; // expected-error {{cannot assign to variable 'c' with const-qualified type 'const int *const'}}

reinterpret_cast<int*>(42) += 3; // expected-error {{expression is not assignable}}
// expected-note@-1 {{add '*' to dereference it}}

const int x = 42;
(const_cast<int*>(&x)) += 3; // expected-error {{expression is not assignable}}
// expected-note@-1 {{add '*' to dereference it}}
}

template <typename T>
void f(T& t) {
// expected-note@* 2 {{variable 't' declared const here}}
++t;
// expected-error@-1 {{cannot assign to variable 't' with const-qualified type 'int *const &'}}
// expected-error@-2 {{cannot assign to variable 't' with const-qualified type 'const int *const &'}}
// expected-note@-3 {{add '*' to dereference it}}
}

void g() {
int* a;
int* const b = a;
const int* const c = a;
f(a);
f(b); // expected-note {{in instantiation of function template specialization 'f<int *const>' requested here}}
f(c); // expected-note {{in instantiation of function template specialization 'f<const int *const>' requested here}}
}
2 changes: 2 additions & 0 deletions clang/test/Sema/exprs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a diagnostic regression.

#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}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a diagnostic regression.

}

void test6(void) {
Expand Down
1 change: 1 addition & 0 deletions clang/test/Sema/va_arg_x86_32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@

int a(void) {
__builtin_va_arg((char*)0, int); // expected-error {{expression is not assignable}}
// expected-note@-1 {{add '*' to dereference it}}
__builtin_va_arg((void*){0}, int); // expected-error {{first argument to 'va_arg' is of type 'void *'}}
}
13 changes: 13 additions & 0 deletions clang/test/SemaObjCXX/sel-address.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,20 @@ void h() {
SEL s = @selector(dealloc);
SEL* ps = &s;

/*
FIXME: https://github.com/llvm/llvm-project/pull/94159

TLDR; This is about inserting '*' to deref.

This would assign the value of s to the SEL object pointed to by
@selector(dealloc). However, in Objective-C, selectors are not pointers,
they are special compile-time constructs representing method names, and
they are immutable, so you cannot assign values to them.

Therefore, this syntax is not valid for selectors in Objective-C.
*/
@selector(dealloc) = s; // expected-error {{expression is not assignable}}
// expected-note@-1 {{add '*' to dereference it}}

SEL* ps2 = &@selector(dealloc);

Expand Down
Loading