Page MenuHomePhabricator

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

Authored by mwyman on Mar 3 2020, 2:33 PM.

Details

Summary

Such method declarations don't provide any benefit, as even without the declaration the compiler would complain about calling the method as it doesn't exist.

Diff Detail

Event Timeline

mwyman created this revision.Mar 3 2020, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 2:33 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
104

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
7

Please enclose attribute((unavailable)) in double back-ticks.

Eugene.Zelenko retitled this revision from New ClangTidy check for methods marked __attribute__((unavailable)) that do not override a method from a superclass. to [clang-tidy] New check for methods marked __attribute__((unavailable)) that do not override a method from a superclass..Mar 3 2020, 7:11 PM
Eugene.Zelenko added a project: Restricted Project.
mwyman updated this revision to Diff 248396.Mar 4 2020, 10:32 PM

Updated to explicitly check for attribute((unavailable)), to avoid flagging methods marked based on platform availability. Updated test file to validate this.

mwyman updated this revision to Diff 248399.Mar 4 2020, 10:35 PM

Updated documentation per review comments.

mwyman marked 2 inline comments as done.Mar 4 2020, 10:37 PM

Fix the test case.

clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
30

Should probably be in an anonymous namespace this as you don't intend it to be visible in another translation unit.

35
clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
35

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

52

This is not a satisfactory check, it will ensure at least one method has been deleted but it wont ensure all methods have been deleted. Would probably be safer putting a unique comment on the line and having a check fix that checks for an empty line and the comment for each case.

mwyman marked 6 inline comments as done.Mar 5 2020, 2:26 PM
mwyman added inline comments.
clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
35

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.

52

Thanks for the suggestion! Done.

mwyman marked 2 inline comments as done.Mar 5 2020, 2:31 PM

I goofed on updating with Arcanist—the changes I marked done will be incoming shortly!

mwyman updated this revision to Diff 248611.Mar 5 2020, 2:36 PM

Don't provide fix-it hints when the unavailable attribute is inside a macro, unless within a config-whitelisted macro.

njames93 added inline comments.Mar 5 2020, 3:36 PM
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
47–49

return llvm::is_contained(Macros, Tok.getRawIdentifier().str())

52

Should probably return an llvm::Optional<FixItHint>

80

This check is surplus to requirements now that there is the isLanguageVersionSupported check

84

objcMethodDecl is implicitly an allOf matcher, so there is no need to specify allOf

mwyman updated this revision to Diff 248654.Mar 5 2020, 11:17 PM
mwyman marked 4 inline comments as done.

Updated per review feedback.

mwyman marked an inline comment as done.Mar 5 2020, 11:17 PM
mwyman added inline comments.
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
80

Oops, forgot to delete it here.

aaron.ballman added inline comments.Mar 6 2020, 7:23 AM
clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
21

You should also document the user-facing option for the check.

mwyman updated this revision to Diff 248788.Mar 6 2020, 10:22 AM

Update documentation to include description of the FixMacroNames config option.

mwyman marked an inline comment as done.Mar 6 2020, 10:22 AM
aaron.ballman added inline comments.Mar 6 2020, 1:15 PM
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
64

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)?

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
30

This looks wrong to me -- it's a semicolon-delimited list, isn't it?

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
35

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)?

Can objc instance methods be redeclared, the AST suggests so, if so you should probably only warm on the first occurrence but create fix its for each.

njames93 added inline comments.Mar 10 2020, 11:08 AM
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
35

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

mwyman updated this revision to Diff 249709.Mar 11 2020, 11:48 AM
mwyman marked 5 inline comments as done.

After some discussion, have decided to remove the fix-it entirely and update the diagnostic message; removing the method altogether may not be the correct behavior, as previously deprecated methods that have since been removed may want to have an unavailable attribute attached with a message explaining what to use instead, even though they don't override a superclass method.

But in general still feel like non-overriding, non-messaged attributes are probably reasonable to flag.

mwyman marked 3 inline comments as done.Mar 11 2020, 11:50 AM
mwyman added inline comments.
clang-tools-extra/clang-tidy/objc/MethodUnavailableNotOverrideCheck.cpp
35

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.

64

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.

clang-tools-extra/docs/clang-tidy/checks/objc-method-unavailable-not-override.rst
30

Obsolete, deleted.

clang-tools-extra/test/clang-tidy/checkers/objc-method-unavailable-not-override.m
35

Obsolete, fix-it deleted.