[PATCH] D75569: [clang-tidy] New check for methods marked __attribute__((unavailable)) that do not override a method from a superclass.

Michael Wyman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 11 11:53:22 PDT 2020


mwyman marked 3 inline comments as done.
mwyman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp:34
+// Matches Objective-C methods that are not overriding a superclass method.
+AST_MATCHER(ObjCMethodDecl, isNotOverriding) { return !Node.isOverriding(); }
+
----------------
njames93 wrote:
> Not a point for this patch, but maybe this should be made into an actual matcher, though not complimented, or better yet the `CXXMethodDecl` `isOverride` matcher be extended for `ObjCMethodDecl`
I agree, and also that it's probably best left to a later patch. Not sure how to deal with the making one that works for both `CXXMethodDecl` and `ObjCMethodDecl`.


================
Comment at: clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp:63
+  }
+  return FixItHint::CreateRemoval(MD->getSourceRange());
+}
----------------
aaron.ballman wrote:
> I'm not an ObjC expert, so I apologize if this is a dumb question, but why is the fix-it removing the entire method as opposed to just the attribute?
> 
> Also, are you sure that the source range for the method declaration is sufficient to remove without breaking code? e.g., what about the definition of the method? Or can there be any trailing bits after the method that are not accounted for in the source range (such as trailing attributes, if those are possible in ObjC)?
Removing the attribute is undesirable, as that would make the compiler allow calling the method, when it has explicitly been marked unavailable. In my experience the majority of the cases in Objective-C for using this attribute are to mark a superclass method unavailable (e.g. a designated initializer that shouldn't be called on the subclass, even just -init, favoring its own declared designated initializer instead), but it can also be used to mark a previously-deprecated-now-removed method with a message to point updaters what method to migrate to.

Which brings up an interesting point that if a message is provided, it could be that an API has finally finished allowing a deprecated method to be called and removed it entirely, and so if a message is provided it may be indicating that and telling what new API to use, rather than overriding the availability of a superclass method. But a method being marked unavailable without any message doesn't really provide much help on that versus the method just not existing. Perhaps if a message is provided this check shouldn't complain at all.

I've removed the fix-it entirely, and replaced it with a different warning message.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst:29
+
+  Comma-separated list of names of macros that can define the unavailable
+  attribute for which fixes should be suggested. The default is an empty list.
----------------
aaron.ballman wrote:
> This looks wrong to me -- it's a semicolon-delimited list, isn't it?
Obsolete, deleted.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m:34
+// Verify check when using a macro that expands to the unavailable attribute.
+- (void)methodC NS_UNAVAILABLE;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: method 'methodC' is marked unavailable but does not override a superclass method [objc-method-unavailable-not-override]
----------------
aaron.ballman wrote:
> mwyman wrote:
> > njames93 wrote:
> > > Generally we are cautious about modifying MACRO usages in clang_tidy as we don't know if its definition can change based on configuration, perhaps its safer to just warn instead of providing a fix it
> > Sounds reasonable; I made this the default behavior.
> > 
> > However for Objective-C, it's quite common for developers to use a macro from the Apple SDKs like NS_UNAVAILABLE that are unconditional in any situations I know of. I added a config option to allow whitelisting macros that should have fix-its provided.
> If the common practice is to use macros like NS_UNAVAILABLE, should that be on the default list of options (along with any other common-use macro names that do the same thing)?
Obsolete, fix-it deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75569





More information about the cfe-commits mailing list