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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 3 inline comments as done.
njames93 added a comment.

In D129202#3633618 <https://reviews.llvm.org/D129202#3633618>, @aaron.ballman wrote:

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

It's not adding a new warning, so chattiness of this patch shouldn't really be much of an issue.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:14656
+
+    // Only check the fields declared in Parent, without traversing base classes
+    auto Field = llvm::find_if(
----------------
aaron.ballman wrote:
> 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?
Once you start getting into deep class hierarchies we start to lose confidence that the fix is what the user intended. Then there is also the issue of access where the field may not even be accessible in Derived and it just adds complexity that I'm not sure is really warranted.


================
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->");
----------------
aaron.ballman wrote:
> 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.
That is exactly the reason for the note. Because we aren't fixing invalid code(self assignment is suspicious but not an error) and the fix has a noticeable change of behaviour, it doesn't seem wise to automatically apply the fix-it.
I don't have a plan down the line to implement this, but what if the intent of the programmer was actually a near miss, like in the, heavily contrived, example below.
```lang=c++
struct Foo {
  int Bar;
  void Stuff(int Bar) {
    int bar;
    // ...
    Bar = Bar; // Did they intend bar = Bar or this->Bar = Bar?
  }
};
``` 



================
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'}}
+  }
----------------
aaron.ballman wrote:
> An additional test case I'd like to see is:
> ```
> struct S {
>   int X;
> 
>   S(int X) : X(X) {} // Don't warn about this
> };
> ```
This patch isn't adding a warning, it's just adding a fix to an existing warning. However there is currently no test cases for that particular instance so I can see value for that use case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129202