[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.

include/clang/Basic/DiagnosticGroups.td

 def UnusedPrivateField : DiagGroup<"unused-private-field">;
 def UnusedFunction : DiagGroup<"unused-function", [UnneededInternalDecl]>;
 def UnusedTemplate : DiagGroup<"unused-template", [UnneededInternalDecl]>;
 def UnusedMemberFunction : DiagGroup<"unused-member-function",
                                      [UnneededMemberFunction]>;
 def UnusedLabel : DiagGroup<"unused-label">;
 def UnusedLambdaCapture : DiagGroup<"unused-lambda-capture">;
 def UnusedParameter : DiagGroup<"unused-parameter">;
+def UnusedParameterNonObjCMethod : DiagGroup<"unused-parameter-non-objc-method">;
+
 def UnusedResult : DiagGroup<"unused-result">;
 def PotentiallyEvaluatedExpression : DiagGroup<"potentially-evaluated-expression">;
 def UnevaluatedExpression : DiagGroup<"unevaluated-expression",
                                       [PotentiallyEvaluatedExpression]>;
 def UnusedValue : DiagGroup<"unused-value", [UnusedComparison, UnusedResult,
                                              UnevaluatedExpression]>;
 def UnusedConstVariable : DiagGroup<"unused-const-variable">;
 def UnusedVariable : DiagGroup<"unused-variable",

include/clang/Basic/DiagnosticSemaKinds.td

def warn_interrupt_attribute_invalid : Warning<
    "functions that have %select{no parameters|a 'void' return type}1">,
    InGroup<IgnoredAttributes>;
 def warn_riscv_repeated_interrupt_attribute : Warning<
   "repeated RISC-V 'interrupt' attribute">, InGroup<IgnoredAttributes>;
 def note_riscv_repeated_interrupt_attribute : Note<
   "repeated RISC-V 'interrupt' attribute is here">;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup<UnusedParameter>, DefaultIgnore;
+def warn_unused_parameter_non_objc_method : Warning<
+  "unused parameter %0">,
+  InGroup<UnusedParameterNonObjCMethod>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
   InGroup<UnusedVariable>, DefaultIgnore;
 def warn_unused_local_typedef : Warning<
   "unused %select{typedef|type alias}0 %1">,
   InGroup<UnusedLocalTypedef>, DefaultIgnore;
 def warn_unused_property_backing_ivar :
   Warning<"ivar %0 which backs the property is not "
   "referenced in this property's accessor">,

include/clang/Sema/Sema.h

public:
   void ActOnFinishInlineFunctionDef(FunctionDecl *D);
 
   /// ActOnFinishDelayedAttribute - Invoked when we have finished parsing an
   /// attribute for which parsing is delayed.
   void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs);
 
   /// Diagnose any unused parameters in the given sequence of
   /// ParmVarDecl pointers.
-  void DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters);
+  void DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters,
+                                bool IsObjCMethodParam = false);
 
   /// Diagnose whether the size of parameters or return value of a
   /// function or obj-c method definition is pass-by-value and larger than a
   /// specified threshold.
   void
   DiagnoseSizeOfParametersAndReturnValue(ArrayRef<ParmVarDecl *> Parameters,
                                          QualType ReturnTy, NamedDecl *D);
 

lib/Sema/SemaDecl.cpp

/* FIXME: setting StartLoc == Loc.
      location for the unnamed parameters, embedding the parameter's type? */
   ParmVarDecl *Param = ParmVarDecl::Create(Context, DC, Loc, Loc, nullptr,
                                 T, Context.getTrivialTypeSourceInfo(T, Loc),
                                            SC_None, nullptr);
   Param->setImplicit();
   return Param;
 }
 
-void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) {
+void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters,
+                                    bool IsObjCMethodParam) {
   // Don't diagnose unused-parameter errors in template instantiations; we
   // will already have done so in the template itself.
   if (inTemplateInstantiation())
     return;
 
   for (const ParmVarDecl *Parameter : Parameters) {
     if (!Parameter->isReferenced() && Parameter->getDeclName() &&
         !Parameter->hasAttr<UnusedAttr>()) {
       Diag(Parameter->getLocation(), diag::warn_unused_parameter)
         << Parameter->getDeclName();
+      if (!IsObjCMethodParam)
+        Diag(Parameter->getLocation(),
+             diag::warn_unused_parameter_non_objc_method)
+            << Parameter->getDeclName();
     }
   }
 }
 
 void Sema::DiagnoseSizeOfParametersAndReturnValue(
     ArrayRef<ParmVarDecl *> Parameters, QualType ReturnTy, NamedDecl *D) {
   if (LangOpts.NumLargeByValueCopy == 0) // No check.
     return;
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());
+        DiagnoseUnusedParameters(MD->parameters(), true);
       DiagnoseSizeOfParametersAndReturnValue(MD->parameters(),
                                              MD->getReturnType(), MD);
 
       if (Body)
         computeNRVO(Body, getCurFunction());
     }
     if (getCurFunction()->ObjCShouldCallSuper) {
       Diag(MD->getEndLoc(), diag::warn_objc_missing_super_call)

test/Sema/warn-unused-parameters.c

 
 // CHECK: 5:12: warning: unused parameter 'y'
 // CHECK: 12:15: warning: unused parameter 'y'
 // CHECK-unused: 1 warning generated
 
 // RUN: %clang_cc1 -fblocks -fsyntax-only -Weverything %s 2>&1 | FileCheck -check-prefix=CHECK-everything %s
 // RUN: not %clang_cc1 -fblocks -fsyntax-only -Weverything -Werror %s 2>&1 | FileCheck -check-prefix=CHECK-everything-error %s
 // RUN: %clang_cc1 -fblocks -fsyntax-only -Weverything -Wno-unused %s 2>&1 | FileCheck -check-prefix=CHECK-everything-no-unused %s
-// CHECK-everything: 6 warnings generated
-// CHECK-everything-error: 5 errors generated
-// CHECK-everything-no-unused: 5 warnings generated
+// CHECK-everything: 8 warnings generated
+// CHECK-everything-error: 7 errors generated
+// CHECK-everything-no-unused: 7 warnings generated
 

test/SemaObjC/method-unused-attribute.m

 // RUN: %clang_cc1  -fsyntax-only -Wunused-parameter -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1  -fsyntax-only -Wunused-parameter-non-objc-method -verify -Wno-objc-root-class -DOBJMETHOD %s
 
 @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
 
+void foo(int unused1) {} // expected-warning {{unused parameter 'unused1'}}
+
+#ifndef OBJMETHOD
+// expected-warning@-7 {{unused parameter 'warn_unused'}}
+// expected-warning@-7 {{unused parameter 'will_warn'}}
+#endif