[clang-tidy] Fix bugs in misc-unused-parameters for Constructors calls site

[mehdi_amini]

Summary

These weren't tracked and so weren't updated when applying fixes.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

157–159

I think this fix is incorrect and should not be applied; it changes the meaning of the interface from having a converting constructor to having a default constructor. I think that needs to be optional behavior because it's a pretty invasive change to apply automatically. This patch builds on top of the existing incorrect behavior, but would be fine if it was only applied when the option to modify constructors is enabled.

[mehdi_amini]

Thanks @aaron.ballman for the review!

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

157–159

I'm not against an option, but I'd like to get to a default behavior that is "safe". So I guess my first patch should be to undo the transformation happening here in the test already.
We can either never touch any C++ constructor or try to find a conservative logic for it.
I mentioned in the other review that we may avoid touching a Ctor with a single parameter.

Then we also can't do it for a Ctor with two parameters as it'll turn it into a converting ctor. Unless you can eliminate both parameters, in which case it is become a default ctor (which can conflict with an existing one, in which case it can be just deleted?)

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

157–159

I'm not against an option, but I'd like to get to a default behavior that is "safe".

Definitely agreed!

So I guess my first patch should be to undo the transformation happening here in the test already.

I think that's a good approach.

We can either never touch any C++ constructor or try to find a conservative logic for it.

Initially, I'd say let's not touch any C++ constructors. We may be able to find conservative logic for it, but that'll take time to get right.

clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

static FixItHint removeParameter(const MatchFinder::MatchResult &Result,
                                  const FunctionDecl *Function, unsigned Index) {
   return FixItHint::CreateRemoval(removeNode(
       Result, Index > 0 ? Function->getParamDecl(Index - 1) : nullptr,
       Function->getParamDecl(Index),
       Index + 1 < Function->getNumParams() ? Function->getParamDecl(Index + 1)
                                            : nullptr));
 }
 
+template <typename CallExprT>
 static FixItHint removeArgument(const MatchFinder::MatchResult &Result,
-                                const CallExpr *Call, unsigned Index) {
+                                const CallExprT *Call, unsigned Index) {
   return FixItHint::CreateRemoval(removeNode(
       Result, Index > 0 ? Call->getArg(Index - 1) : nullptr,
       Call->getArg(Index),
       Index + 1 < Call->getNumArgs() ? Call->getArg(Index + 1) : nullptr));
 }
 
 class UnusedParametersCheck::IndexerVisitor
     : public RecursiveASTVisitor<IndexerVisitor> {
 public:
-  IndexerVisitor(ASTContext &Ctx) { TraverseAST(Ctx); }
+  IndexerVisitor(ASTContext &Ctx) : mgr(Ctx.getSourceManager()) {
+    TraverseAST(Ctx);
+  }
 
   const std::unordered_set<const CallExpr *> &
   getFnCalls(const FunctionDecl *Fn) {
     return Index[Fn->getCanonicalDecl()].Calls;
   }
 
+  const std::unordered_set<const CXXConstructExpr *> &
+  getCtorCalls(const FunctionDecl *Fn) {
+    return Index[Fn->getCanonicalDecl()].CtorRefs;
+  }
+
   const std::unordered_set<const DeclRefExpr *> &
   getOtherRefs(const FunctionDecl *Fn) {
     return Index[Fn->getCanonicalDecl()].OtherRefs;
   }
 
   bool shouldTraversePostOrder() const { return true; }
 
   bool WalkUpFromDeclRefExpr(DeclRefExpr *DeclRef) {
     if (const auto *Fn = dyn_cast<FunctionDecl>(DeclRef->getDecl())) {
       Fn = Fn->getCanonicalDecl();
       Index[Fn].OtherRefs.insert(DeclRef);
     }
     return true;
   }
 
+  bool WalkUpFromCXXConstructExpr(CXXConstructExpr *Call) {
+    if (const auto *Ctor =
+            dyn_cast_or_null<FunctionDecl>(Call->getConstructor())) {
+      Ctor = Ctor->getCanonicalDecl();
+      Index[Ctor].CtorRefs.insert(Call);
+    }
+    return true;
+  }
+
   bool WalkUpFromCallExpr(CallExpr *Call) {
     if (const auto *Fn =
             dyn_cast_or_null<FunctionDecl>(Call->getCalleeDecl())) {
       Fn = Fn->getCanonicalDecl();
       if (const auto *Ref =
               dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit())) {
         Index[Fn].OtherRefs.erase(Ref);
       }
       Index[Fn].Calls.insert(Call);
     }
     return true;
   }
 
 private:
   struct IndexEntry {
     std::unordered_set<const CallExpr *> Calls;
     std::unordered_set<const DeclRefExpr *> OtherRefs;
+    std::unordered_set<const CXXConstructExpr *> CtorRefs;
   };
-
+  SourceManager &mgr;
   std::unordered_map<const FunctionDecl *, IndexEntry> Index;
 };
 
 UnusedParametersCheck::~UnusedParametersCheck() = default;
 
 UnusedParametersCheck::UnusedParametersCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
void UnusedParametersCheck::warnOnUnusedParameter(
   }
 
   // Fix all redeclarations.
   for (const FunctionDecl *FD : Function->redecls())
     if (FD->param_size())
       MyDiag << removeParameter(Result, FD, ParamIndex);
 
   // Fix all call sites.
-  for (const CallExpr *Call : Indexer->getFnCalls(Function))
+  for (const CallExpr *Call : Indexer->getFnCalls(Function)) {
+    if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
+      MyDiag << removeArgument(Result, Call, ParamIndex);
+  }
+  for (const CXXConstructExpr *Call : Indexer->getCtorCalls(Function)) {
     if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
       MyDiag << removeArgument(Result, Call, ParamIndex);
   }
+}
 
 void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("function");
   if (!Function->hasWrittenPrototype() || Function->isTemplateInstantiation())
     return;
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Function))
     if (Method->isLambdaStaticInvoker())
       return;

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

// CHECK-FIXES: static void h(int  /*i*/[]) {;}
   static void s(void (*fn)()) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning
 // CHECK-FIXES: static void s(void (* /*fn*/)()) {;}
 };
 
 namespace {
 class C {
 public:
 // CHECK-FIXES: C() {}
   C(int i) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning
 
   void f(int i);
 // CHECK-FIXES: void f();
   void g(int i) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:14: warning
 // CHECK-FIXES: void g() {;}
   void h(int i) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:14: warning

 // CHECK-MESSAGES: :[[@LINE-1]]:15: warning
 // CHECK-FIXES: void C::f() {;}
 
 template <typename T>
 void useFunction(T t);
 
 void someMoreCallSites() {
   C c(42);
+// CHECK-FIXES: C c();
   c.f(1);
 // CHECK-FIXES: c.f();
   c.g(1);
 // CHECK-FIXES: c.g();
 
   useFunction(&C::h);
   useFunction(&C::s);
   useFunction(&C::u);