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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:112
+      << Message->getMethodDecl()
+      << FixItHint::CreateReplacement(Message->getSourceRange(),
+                                      StringRef("[super init]"));
----------------
stephanemoore wrote:
> 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];
> }
> ```
> 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?

Essentially, yes. Macros can get arbitrarily complex and so our rule of thumb is to not apply fix-its when the code being fixed is within a macro. Another example that can be tricky is adding another layer of macros:
```
#define FOO super
#define BAR self
#define BAZ FOO BAR

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


================
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]
+}
----------------
stephanemoore wrote:
> 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.
Our typical way to check that a fix is not applied is to use `// CHECK-FIXES: <original code>` to test that the fix was not applied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59806