[PATCH] D129202: [clang] Add a fixit for warn-self-assign if LHS is a field with the same name as parameter on RHS

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I think it's a nice new diagnostic. You should add a release note to describe it. One question I have is about how chatty this is over real world code, especially older code bases. There used to be two prevailing ways to silence the "unused variable" warnings you would get when a variable is used in an `assert`: `(void)whatever;` and `whatever = whatever;`; I'm worried the latter case is still prevalent enough that having this warning on by default would be a problem, but I'm also optimistic my worry is unfounded.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6617
+def note_self_assign_insert_this : Note<
+  "did you mean to assign to member %0">;
 
----------------
We don't normally have trailing punctuation... except for question marks which we will happily use when asking the user a question.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14645-14650
+  if (!llvm::isa<ParmVarDecl>(RHSDecl))
+    return;
+  if (auto *Method =
+          llvm::dyn_cast_or_null<CXXMethodDecl>(S.getCurFunctionDecl(true))) {
+
+    CXXRecordDecl *Parent = Method->getParent();
----------------
Removing the `llvm::` from those mostly because we already call `cast<>` without the prefix. Oddly, I find myself liking the `llvm::` in `llvm::find_if` though, so take or leave the suggestions about it.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14656
+
+    // Only check the fields declared in Parent, without traversing base classes
+    auto Field = llvm::find_if(
----------------
Why wouldn't we want to look through base classes for a member there? Considering a case like:
```
struct Base {
  int X;
};

struct Derived : Base {
  Derived(int X) { X = X; } // Oooooops
};
```
it seems like we would want to use a real lookup operation instead of the fields of the class. Or did you try that and found it was a performance concern?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:14661-14664
+    if (Field != Parent->fields().end())
+      S.Diag(LHSDeclRef->getBeginLoc(), diag::note_self_assign_insert_this)
+          << *Field
+          << FixItHint::CreateInsertion(LHSDeclRef->getBeginLoc(), "this->");
----------------
Use of a note like this isn't awful, but I'm not super happy that the note always shows up on the same line as the warning. It seems like it would be cleaner (in terms of the user experience) to use a `%select` on the warning diagnostic to decide whether to add the "did you mean" information, and associate the fix directly with the warning. However, there's a slight functional difference with that approach which may matter to you: fixits attached to a note are not automatically applied when in fixit mode, but they are when attached to a warning or error. I don't know a reason why applying the fix-it automatically would be a bad thing in this case, so use of a note loses a bit of useful functionality.


================
Comment at: clang/test/SemaCXX/warn-self-assign-builtin.cpp:69-73
+struct Foo {
+  int A;
+  void setA(int A) {
+    A = A; // expected-warning{{explicitly assigning}} expected-note {{did you mean to assign to member 'A'}}
+  }
----------------
An additional test case I'd like to see is:
```
struct S {
  int X;

  S(int X) : X(X) {} // Don't warn about this
};
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129202/new/

https://reviews.llvm.org/D129202