[Sema][ObjC] Disable -Wunused-parameter for ObjC methods

[ahatanak]

Summary

The warning isn't very useful when the function is an ObjC method as it generates lots of false positives.

[erik.pilkington]

but don't want to annotate each unused ObjC method parameters with __attribute__((unused)) or insert (void)unused_param into the body of the method to silence the warning since, unlike C/C++ functions, it's not possible to comment out the unused parameters of ObjC methods.

Thats also true of function parameters in C (and Objective-C), so I don't think that argument is very convincing. I think it is true that this is a much less useful diagnostic for methods though, because they're more likely to be overriding something that requires a certain selector. Maybe we should just suppress this warning in general for methods (and even virtual functions in C++)?

[rjmccall]

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

[erik.pilkington]

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

[rjmccall]

Sorry, yes, I meant in Objective-C methods, of course, not unconditionally even in C-like code whenever Objective-C is enabled.

[ahatanak]

In D61147#1479468, @erik.pilkington wrote:

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

Should we use the new warning to warn just about unused ObjC method parameters and have -Wunused-parameter warn about everything else? If we make -Wunused-parameter unconditionally ignore ObjC methods, a user might complain about it later if -Wunused-parameter didn't diagnose an unused parameter when it should have.

[erik.pilkington]

In D61147#1479530, @ahatanak wrote:

In D61147#1479468, @erik.pilkington wrote:

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

IMO this warning still makes sense for normal functions, or methods that are only declared in an @implementation. Adding a fix-it to cast to void in the function/method body would probably go a long way too.

Should we use the new warning to warn just about unused ObjC method parameters and have -Wunused-parameter warn about everything else? If we make -Wunused-parameter unconditionally ignore ObjC methods, a user might complain about it later if -Wunused-parameter didn't diagnose an unused parameter when it should have.

Yeah, that seems fine to me, leave the method diagnostic off-by-default, since I doubt many would actually want it. Do you think its worth emitting -Wunused-parameter in pure-@implementation methods?

[ahatanak]

Yes, I guess we can make -Wunused-parameter cover unused parameters of pure @implementation methods too since users can probably just remove them to silence the warning.

[rjmccall]

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

I would also recommend that you go fix the warning to never fire on virtual C++ methods.

[erik.pilkington]

In D61147#1479932, @rjmccall wrote:

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

[rjmccall]

In D61147#1479940, @erik.pilkington wrote:

In D61147#1479932, @rjmccall wrote:

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

What rule are you suggesting for whether a method is "only declared in an @implementation"? Where exactly are you proposing to look for other declarations?

Objective-C does not have a formalized notion of an @implementation-private method, or really even an unambiguous convention for them (underscoring is *library*-private by convention, not class-private). The Objective-C ecosystem includes a lot of informal protocols and conventions based around discovery via naming, and Objective-C programmers do (admittedly rarely) just override methods that they know are there but which they can't see. These things make me very worried about trying to port assumptions from other languages.

Anyway, put that aside for a minute. As I understand it, your current proposal is to make -Wunused-parameter not fire on the majority of Objective-C methods and then add a new -Wunused-objc-parameter warning that's not really formally related to -Wunused-parameter. I think the right way to think about that is that the new warning is an independent feature which can happen at its own pace. You will need to do some investigation in order to implement that feature, so in the short term, you should just not warn about unused parameters in Objective-C methods, and then you can do the investigation for generalizing the warning as time permits.

[erik.pilkington]

In D61147#1479956, @rjmccall wrote:

In D61147#1479940, @erik.pilkington wrote:

In D61147#1479932, @rjmccall wrote:

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

What rule are you suggesting for whether a method is "only declared in an @implementation"? Where exactly are you proposing to look for other declarations?

Objective-C does not have a formalized notion of an @implementation-private method, or really even an unambiguous convention for them (underscoring is *library*-private by convention, not class-private). The Objective-C ecosystem includes a lot of informal protocols and conventions based around discovery via naming, and Objective-C programmers do (admittedly rarely) just override methods that they know are there but which they can't see. These things make me very worried about trying to port assumptions from other languages.

Concretely, I was just thinking of using something like ObjCMethodDecl::isOverriding, but if that isn't a good enough heuristic for finding local methods then I guess we should just forgo methods entirely.

[ahatanak]

Disable the warning for ObjC methods.

[erik.pilkington]

LGTM, thanks!

[ahatanak]

In D61147#1479932, @rjmccall wrote:

I would also recommend that you go fix the warning to never fire on virtual C++ methods.

I tried building clang/llvm with -Wunused-parameter turned on and there are lots of places where parameters are unused other than virtual functions, in particular template functions and non-template functions that are called by other template functions. So disabling the warning on C++ virtual functions is probably not enough in order to make this warning more useful.

cfe/trunk/lib/Sema/SemaDecl.cpp

if (FD) {
     }
 
     assert((FD == getCurFunctionDecl() || getCurLambda()->CallOperator == FD) &&
            "Function parsing confused");
   } else if (ObjCMethodDecl *MD = dyn_cast_or_null<ObjCMethodDecl>(dcl)) {
     assert(MD == getCurMethodDecl() && "Method parsing confused");
     MD->setBody(Body);
     if (!MD->isInvalidDecl()) {
-      if (!MD->hasSkippedBody())
-        DiagnoseUnusedParameters(MD->parameters());
       DiagnoseSizeOfParametersAndReturnValue(MD->parameters(),
                                              MD->getReturnType(), MD);
 
       if (Body)
         computeNRVO(Body, getCurFunction());
     }
     if (getCurFunction()->ObjCShouldCallSuper) {
       Diag(MD->getEndLoc(), diag::warn_objc_missing_super_call)

cfe/trunk/test/SemaObjC/method-unused-attribute.m

 // RUN: %clang_cc1  -fsyntax-only -Wunused-parameter -verify -Wno-objc-root-class %s
 
+// -Wunused-parameter ignores ObjC method parameters that are unused.
+
+// expected-no-diagnostics
+
 @interface INTF
 - (void) correct_use_of_unused: (void *) notice : (id)another_arg;
 - (void) will_warn_unused_arg: (void *) notice : (id)warn_unused;
 - (void) unused_attr_on_decl_ignored: (void *)  __attribute__((unused)) will_warn;
 @end
 
 @implementation INTF
 - (void) correct_use_of_unused: (void *)  __attribute__((unused)) notice : (id) __attribute__((unused)) newarg{
 }
-- (void) will_warn_unused_arg: (void *) __attribute__((unused))  notice : (id)warn_unused {}  // expected-warning {{unused parameter 'warn_unused'}}
-- (void) unused_attr_on_decl_ignored: (void *)  will_warn{}  // expected-warning {{unused parameter 'will_warn'}}
+- (void) will_warn_unused_arg: (void *) __attribute__((unused))  notice : (id)warn_unused {}
+- (void) unused_attr_on_decl_ignored: (void *)  will_warn{}
 @end
 

cfe/trunk/test/SemaObjC/unused.m

 - (int)meth: (int)x : (int)y : (int)z ;
 @end
 
 @implementation foo
 - (int) meth: (int)x: // expected-warning {{'x' used as the name of the previous parameter rather than as part of the selector}} \
                       // expected-note {{introduce a parameter name to make 'x' part of the selector}} \
                       // expected-note {{or insert whitespace before ':' to use 'x' as parameter name and have an empty entry in the selector}}
 
-(int)y:  // expected-warning {{unused}}  expected-warning {{'y' used as the name of the previous parameter rather than as part of the selector}} \
+(int)y:  // expected-warning {{'y' used as the name of the previous parameter rather than as part of the selector}} \
          // expected-note {{introduce a parameter name to make 'y' part of the selector}} \
          // expected-note {{or insert whitespace before ':' to use 'y' as parameter name and have an empty entry in the selector}}
 (int) __attribute__((unused))z { return x; }
 @end
 
 //===------------------------------------------------------------------------===
 // The next test shows how clang accepted attribute((unused)) on ObjC
 // instance variables, which GCC does not.