[analyzer] Fix NSErrorChecker false positives

[vsavchenko]

Summary

NSErrorChecker used to suggest changing 'void' return type for
constructors and delete operators. This makes little sense because
return types of these functions could not be altered.

[NoQ]

Looks great, thanks! Do you already have commit access or should i commit this for you?

clang/test/Analysis/SpecialFunctionsCFError.cpp

2

-analyzer-store=region is redundant; it's the default and the only possible value for this option. We should remove it from other tests as well.

[Szelethus]

clang/test/Analysis/SpecialFunctionsCFError.cpp

2

The default lit command is a bit silly. What does -setup-analyzer or something like that even do?

[vsavchenko]

In D77551#1964441, @NoQ wrote:

Looks great, thanks! Do you already have commit access or should i commit this for you?

Thanks :-) No, I don't have commit access

clang/test/Analysis/SpecialFunctionsCFError.cpp

2

Should I remove it from here in this commit or we should clean up all tests at once?

[NoQ]

clang/test/Analysis/SpecialFunctionsCFError.cpp

2

The default lit command is a bit silly. What does -setup-analyzer or something like that even do?

1029   if (PPOpts.SetUpStaticAnalyzer)
1030     Builder.defineMacro("__clang_analyzer__");

:)

Should I remove it from here in this commit or we should clean up all tests at once?

I'll remove when committing, thx!

clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp

 //=- NSErrorChecker.cpp - Coding conventions for uses of NSError -*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 public:
   CFErrorFunctionChecker() : II(nullptr) {}
 
   void checkASTDecl(const FunctionDecl *D,
                     AnalysisManager &mgr, BugReporter &BR) const;
 };
 }
 
+static bool hasReservedReturnType(const FunctionDecl *D) {
+  if (isa<CXXConstructorDecl>(D))
+    return true;
+
+  // operators delete and delete[] are required to have 'void' return type
+  auto OperatorKind = D->getOverloadedOperator();
+  return OperatorKind == OO_Delete || OperatorKind == OO_Array_Delete;
+}
+
 void CFErrorFunctionChecker::checkASTDecl(const FunctionDecl *D,
                                         AnalysisManager &mgr,
                                         BugReporter &BR) const {
   if (!D->doesThisDeclarationHaveABody())
     return;
   if (!D->getReturnType()->isVoidType())
     return;
+  if (hasReservedReturnType(D))
+    return;
 
   if (!II)
     II = &D->getASTContext().Idents.get("CFErrorRef");
 
   bool hasCFError = false;
   for (auto I : D->parameters())  {
     if (IsCFError(I->getType(), II)) {
       hasCFError = true;

clang/test/Analysis/SpecialFunctionsCFError.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.coreFoundation.CFError \
+// RUN:   -analyzer-store=region -verify %s
+
+typedef unsigned long size_t;
+struct __CFError {};
+typedef struct __CFError *CFErrorRef;
+void *malloc(size_t);
+
+class Foo {
+public:
+  Foo(CFErrorRef *error) {} // no-warning
+
+  void operator delete(void *pointer, CFErrorRef *error) { // no-warning
+    return;
+  }
+
+  void operator delete[](void *pointer, CFErrorRef *error) { // no-warning
+    return;
+  }
+
+  // Check that we report warnings for operators when it can be useful
+  void operator()(CFErrorRef *error) {} // expected-warning {{Function accepting CFErrorRef* should have a non-void return value to indicate whether or not an error occurred}}
+};
+
+// Check that global delete operator is not bothered as well
+void operator delete(void *pointer, CFErrorRef *error) { // no-warning
+  return;
+}