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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:112
+      << Message->getMethodDecl()
+      << FixItHint::CreateReplacement(Message->getSourceRange(),
+                                      StringRef("[super init]"));
----------------
aaron.ballman wrote:
> 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.
Added a test case though at the moment it generates a fixit.

Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?

```
#if FLAG
#define INVOCATION self
#else
#define INVOCATION init
#endif

- (instancetype)init {
  return [super INVOCATION];
}
```


================
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]
+}
----------------
aaron.ballman wrote:
> 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.
No fix is currently generated for this case which is why there is no `CHECK-FIXES`. I also agree that no fix should be generated for this case.

I must confess that I have yet to fully understand why the fix for this case is discarded (though I am grateful for the behavior). Let me dig around to try to better understand why no fixit is generated for this case and assess adding a condition for emitting the fixit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59806