[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59806#1447929 <https://reviews.llvm.org/D59806#1447929>, @jordan_rose wrote:

> I don't think there's ever a reason to call `[super self]`, and doing so through a macro could easily indicate a bug.


Thank you for the verification! And agreed about the macro thing -- I am only worried about trying to issue a fix-it in that case, not the diagnostic itself.



================
Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:112
+      << Message->getMethodDecl()
+      << FixItHint::CreateReplacement(Message->getSourceRange(),
+                                      StringRef("[super init]"));
----------------
stephanemoore wrote:
> stephanemoore wrote:
> > aaron.ballman wrote:
> > > This could be dangerous if the `[super self]` construct is in a macro, couldn't it? e.g.,
> > > ```
> > > #define DERP self
> > > 
> > > [super DERP];
> > > ```
> > Good point. Let me add some test cases and make sure this is handled properly.
> Added some test cases where `[super self]` is expanded from macros.
You missed the test case I was worried about -- where the macro is mixed into the expression. I don't think we want to try to add a fix-it in that case.


================
Comment at: clang-tools-extra/test/clang-tidy/objc-super-self.m:41
+  INITIALIZER_IMPL();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: suspicious invocation of 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+}
----------------
Are you missing a `CHECK-FIXES` here?

Personally, I don't think we should try to generate a fixit for this case, so I would expect a CHECK-FIXES that ensures this doesn't get modified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59806