https://github.com/yuxuanchen1997 updated https://github.com/llvm/llvm-project/pull/70567
>From 77b9cba0aaa1157cc323f2f3ef7b1cef536ef147 Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Fri, 27 Oct 2023 16:37:40 -0700 Subject: [PATCH 1/6] [Clang] Coroutines: warn against unused parameters in C++ coroutines with -Wunused-parameters --- clang/include/clang/AST/DeclBase.h | 18 +++++++++++- clang/lib/Sema/SemaCoroutine.cpp | 6 ++++ clang/lib/Sema/SemaDecl.cpp | 5 +++- .../warn-unused-parameters-coroutine.cpp | 28 +++++++++++++++++++ 4 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 978e4255e877ec2..dc78ee37d16bea2 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -306,6 +306,11 @@ class alignas(8) Decl { /// are regarded as "referenced" but not "used". unsigned Referenced : 1; + /// Whether the last reference to this declaration happened in Coroutine + /// Parameter moves. Otherwise the reference caused by such moves would + /// prevent a warning against unused parameters in all coroutines. + unsigned LastReferenceInCoroutineParamMoves : 1; + /// Whether this declaration is a top-level declaration (function, /// global variable, etc.) that is lexically inside an objc container /// definition. @@ -609,11 +614,22 @@ class alignas(8) Decl { /// Whether any declaration of this entity was referenced. bool isReferenced() const; + bool isLastReferenceInCoroutineParamMoves() const { + return LastReferenceInCoroutineParamMoves; + } + + void setLastReferenceInCoroutineParamMoves(bool V = true) { + LastReferenceInCoroutineParamMoves = true; + } + /// Whether this declaration was referenced. This should not be relied /// upon for anything other than debugging. bool isThisDeclarationReferenced() const { return Referenced; } - void setReferenced(bool R = true) { Referenced = R; } + void setReferenced(bool R = true) { + Referenced = R; + LastReferenceInCoroutineParamMoves = false; + } /// Whether this declaration is a top-level declaration (function, /// global variable, etc.) that is lexically inside an objc container diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 38ac406b14adadf..3af42cae9ba0420 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -1965,9 +1965,15 @@ bool Sema::buildCoroutineParameterMoves(SourceLocation Loc) { if (PD->getType()->isDependentType()) continue; + bool PDRefBefore = PD->isReferenced(); + ExprResult PDRefExpr = BuildDeclRefExpr(PD, PD->getType().getNonReferenceType(), ExprValueKind::VK_LValue, Loc); // FIXME: scope? + + if (!PDRefBefore) + PD->setLastReferenceInCoroutineParamMoves(); + if (PDRefExpr.isInvalid()) return false; diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 824267acbb1c04e..6ce4871152a4e78 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15092,7 +15092,10 @@ void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) { return; for (const ParmVarDecl *Parameter : Parameters) { - if (!Parameter->isReferenced() && Parameter->getDeclName() && + if (Parameter->isReferenced() && !Parameter->isLastReferenceInCoroutineParamMoves()) + continue; + + if (Parameter->getDeclName() && !Parameter->hasAttr<UnusedAttr>() && !Parameter->getIdentifier()->isPlaceholder()) { Diag(Parameter->getLocation(), diag::warn_unused_parameter) diff --git a/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp b/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp new file mode 100644 index 000000000000000..0fd26ea4a21be79 --- /dev/null +++ b/clang/test/SemaCXX/warn-unused-parameters-coroutine.cpp @@ -0,0 +1,28 @@ +// RUN: %clang_cc1 -fsyntax-only -Wunused-parameter -verify -std=c++20 %s + +#include "Inputs/std-coroutine.h" + +struct awaitable { + bool await_ready() noexcept; + void await_resume() noexcept; + void await_suspend(std::coroutine_handle<>) noexcept; +}; + +struct task : awaitable { + struct promise_type { + task get_return_object() noexcept; + awaitable initial_suspend() noexcept; + awaitable final_suspend() noexcept; + void unhandled_exception() noexcept; + void return_void() noexcept; + }; +}; + +task foo(int a) { // expected-warning{{unused parameter 'a'}} + co_return; +} + +task bar(int a, int b) { // expected-warning{{unused parameter 'b'}} + a = a + 1; + co_return; +} >From c094a4fa29142b33cb15c41f5544395d3e87473c Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Sat, 28 Oct 2023 12:42:01 -0700 Subject: [PATCH 2/6] fix small bug: setLastReferenceInCoroutineParamMoves should respect param --- clang/include/clang/AST/DeclBase.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index dc78ee37d16bea2..ff519d14602014b 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -619,7 +619,7 @@ class alignas(8) Decl { } void setLastReferenceInCoroutineParamMoves(bool V = true) { - LastReferenceInCoroutineParamMoves = true; + LastReferenceInCoroutineParamMoves = V; } /// Whether this declaration was referenced. This should not be relied >From cdf0368d8aef4badaeb70274c465df28744184ee Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Sat, 28 Oct 2023 12:46:30 -0700 Subject: [PATCH 3/6] fix clang format --- clang/lib/Sema/SemaDecl.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 6ce4871152a4e78..2485f05accd74dc 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15092,11 +15092,11 @@ void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) { return; for (const ParmVarDecl *Parameter : Parameters) { - if (Parameter->isReferenced() && !Parameter->isLastReferenceInCoroutineParamMoves()) + if (Parameter->isReferenced() && + !Parameter->isLastReferenceInCoroutineParamMoves()) continue; - if (Parameter->getDeclName() && - !Parameter->hasAttr<UnusedAttr>() && + if (Parameter->getDeclName() && !Parameter->hasAttr<UnusedAttr>() && !Parameter->getIdentifier()->isPlaceholder()) { Diag(Parameter->getLocation(), diag::warn_unused_parameter) << Parameter->getDeclName(); >From fe9a5adc60a1719da7110f133ae2bf0d69f459ce Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Tue, 31 Oct 2023 10:06:34 -0700 Subject: [PATCH 4/6] revert clang changes --- clang/include/clang/AST/DeclBase.h | 18 +----------------- clang/lib/Sema/SemaCoroutine.cpp | 6 ------ clang/lib/Sema/SemaDecl.cpp | 7 ++----- 3 files changed, 3 insertions(+), 28 deletions(-) diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index ff519d14602014b..978e4255e877ec2 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -306,11 +306,6 @@ class alignas(8) Decl { /// are regarded as "referenced" but not "used". unsigned Referenced : 1; - /// Whether the last reference to this declaration happened in Coroutine - /// Parameter moves. Otherwise the reference caused by such moves would - /// prevent a warning against unused parameters in all coroutines. - unsigned LastReferenceInCoroutineParamMoves : 1; - /// Whether this declaration is a top-level declaration (function, /// global variable, etc.) that is lexically inside an objc container /// definition. @@ -614,22 +609,11 @@ class alignas(8) Decl { /// Whether any declaration of this entity was referenced. bool isReferenced() const; - bool isLastReferenceInCoroutineParamMoves() const { - return LastReferenceInCoroutineParamMoves; - } - - void setLastReferenceInCoroutineParamMoves(bool V = true) { - LastReferenceInCoroutineParamMoves = V; - } - /// Whether this declaration was referenced. This should not be relied /// upon for anything other than debugging. bool isThisDeclarationReferenced() const { return Referenced; } - void setReferenced(bool R = true) { - Referenced = R; - LastReferenceInCoroutineParamMoves = false; - } + void setReferenced(bool R = true) { Referenced = R; } /// Whether this declaration is a top-level declaration (function, /// global variable, etc.) that is lexically inside an objc container diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 3af42cae9ba0420..38ac406b14adadf 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -1965,15 +1965,9 @@ bool Sema::buildCoroutineParameterMoves(SourceLocation Loc) { if (PD->getType()->isDependentType()) continue; - bool PDRefBefore = PD->isReferenced(); - ExprResult PDRefExpr = BuildDeclRefExpr(PD, PD->getType().getNonReferenceType(), ExprValueKind::VK_LValue, Loc); // FIXME: scope? - - if (!PDRefBefore) - PD->setLastReferenceInCoroutineParamMoves(); - if (PDRefExpr.isInvalid()) return false; diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 2485f05accd74dc..824267acbb1c04e 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15092,11 +15092,8 @@ void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) { return; for (const ParmVarDecl *Parameter : Parameters) { - if (Parameter->isReferenced() && - !Parameter->isLastReferenceInCoroutineParamMoves()) - continue; - - if (Parameter->getDeclName() && !Parameter->hasAttr<UnusedAttr>() && + if (!Parameter->isReferenced() && Parameter->getDeclName() && + !Parameter->hasAttr<UnusedAttr>() && !Parameter->getIdentifier()->isPlaceholder()) { Diag(Parameter->getLocation(), diag::warn_unused_parameter) << Parameter->getDeclName(); >From 5b8bdca83cfbeaf09477aedf22b861a988e02f16 Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Tue, 31 Oct 2023 20:54:19 -0700 Subject: [PATCH 5/6] teach DiagnoseUnusedParameters how to diagnose coroutines --- clang/include/clang/Sema/Sema.h | 11 ++++++- clang/lib/Sema/SemaDecl.cpp | 54 ++++++++++++++++++++++++++++++--- 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 91a4211a5cf5cce..b81fadfff03ba8a 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -3165,7 +3165,16 @@ class Sema final { /// Diagnose any unused parameters in the given sequence of /// ParmVarDecl pointers. - void DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters); + /// + /// Normally, we check if the parameter decls have the Referenced bit set. + /// C++ Coroutines, however, are a special case due to the existences of + /// parameter moves (See Sema::buildCoroutineParameterMoves), the parameters + /// are always referenced in coroutines. Therefore, in the case of coroutines, + /// CoroutineBodyRefs must be passed to correctly diagnose parameter usages + /// as written by the user. + void DiagnoseUnusedParameters( + ArrayRef<ParmVarDecl *> Parameters, + llvm::SmallSet<ParmVarDecl *, 4> *CoroutineBodyRefs = nullptr); /// 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 diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 824267acbb1c04e..55fe6c0bb939446 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -25,6 +25,7 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/NonTrivialTypeVisitor.h" #include "clang/AST/Randstruct.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtCXX.h" #include "clang/Basic/Builtins.h" #include "clang/Basic/HLSLRuntime.h" @@ -45,6 +46,7 @@ #include "clang/Sema/ScopeInfo.h" #include "clang/Sema/SemaInternal.h" #include "clang/Sema/Template.h" +#include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" #include "llvm/TargetParser/Triple.h" @@ -15085,14 +15087,26 @@ ParmVarDecl *Sema::BuildParmVarDeclForTypedef(DeclContext *DC, return Param; } -void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) { +void Sema::DiagnoseUnusedParameters( + ArrayRef<ParmVarDecl *> Parameters, + llvm::SmallSet<ParmVarDecl *, 4> *CoroutineBodyRefs) { // Don't diagnose unused-parameter errors in template instantiations; we // will already have done so in the template itself. if (inTemplateInstantiation()) return; + auto isReferenced = [&](const ParmVarDecl *Parameter) { + if (CoroutineBodyRefs) { + if (CoroutineBodyRefs->count(Parameter)) + return true; + } else if (Parameter->isReferenced()) + return true; + + return false; + }; + for (const ParmVarDecl *Parameter : Parameters) { - if (!Parameter->isReferenced() && Parameter->getDeclName() && + if (!isReferenced(Parameter) && Parameter->getDeclName() && !Parameter->hasAttr<UnusedAttr>() && !Parameter->getIdentifier()->isPlaceholder()) { Diag(Parameter->getLocation(), diag::warn_unused_parameter) @@ -15805,6 +15819,28 @@ static void diagnoseImplicitlyRetainedSelf(Sema &S) { << FixItHint::CreateInsertion(P.first, "self->"); } +namespace { +struct StmtReferenceVisitor : public RecursiveASTVisitor<StmtReferenceVisitor> { + const ASTContext &Context; + StmtReferenceVisitor(const ASTContext &Ctx) : Context(Ctx) {} + + llvm::SmallSet<ParmVarDecl *, 4> UsedParams; + + bool VisitStmt(Stmt *S) { + ValueDecl *D = nullptr; + if (auto E = dyn_cast<DeclRefExpr>(S)) + D = E->getDecl(); + + if (D) + if (auto PVD = dyn_cast<ParmVarDecl>(D)) + UsedParams.insert(PVD); + + return true; + } +}; + +} // end anonymous namespace + Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, bool IsInstantiation) { FunctionScopeInfo *FSI = getCurFunction(); @@ -15882,8 +15918,18 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, // Don't diagnose unused parameters of defaulted, deleted or naked // functions. if (!FD->isDeleted() && !FD->isDefaulted() && !FD->hasSkippedBody() && - !FD->hasAttr<NakedAttr>()) - DiagnoseUnusedParameters(FD->parameters()); + !FD->hasAttr<NakedAttr>()) { + + if (auto CBS = dyn_cast<CoroutineBodyStmt>(Body)) { + auto BodyAsWritten = CBS->getBody(); + assert(BodyAsWritten && + "Coroutine body CompoundStmt should exist!"); + StmtReferenceVisitor SRV(Context); + SRV.TraverseStmt(BodyAsWritten); + DiagnoseUnusedParameters(FD->parameters(), &SRV.UsedParams); + } else + DiagnoseUnusedParameters(FD->parameters()); + } DiagnoseSizeOfParametersAndReturnValue(FD->parameters(), FD->getReturnType(), FD); >From 1906bbd46dd1b9845b60d7a8c1c68bf9f66b33a1 Mon Sep 17 00:00:00 2001 From: Yuxuan Chen <y...@meta.com> Date: Tue, 31 Oct 2023 23:52:16 -0700 Subject: [PATCH 6/6] address comments --- clang/include/clang/Sema/Sema.h | 21 ++++++++++------- clang/lib/Sema/SemaDecl.cpp | 42 ++++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index b81fadfff03ba8a..ba413fbac8348cb 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -3163,18 +3163,23 @@ class Sema final { /// 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. + /// Diagnose any unused parameters in the given sequence of ParmVarDecl + /// pointers. This function does not work with Coroutines parameters. Use + /// DiagnoseUnusedParametersInCoroutine instead. + void DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters); + + /// Diagnose any unused parameters in the given sequence of ParmVarDecl + /// pointers and a set of user referenced parameters inside the coroutine. /// - /// Normally, we check if the parameter decls have the Referenced bit set. - /// C++ Coroutines, however, are a special case due to the existences of + /// Normally, having the Referenced bit set suffices to show that parameters + /// are used. C++ Coroutines, however, are special due to the existence of /// parameter moves (See Sema::buildCoroutineParameterMoves), the parameters /// are always referenced in coroutines. Therefore, in the case of coroutines, - /// CoroutineBodyRefs must be passed to correctly diagnose parameter usages - /// as written by the user. - void DiagnoseUnusedParameters( + /// this variant of the function must be called with CoroutineBodyRefs that + /// contain the actual referenced parameters as written by the user. + void DiagnoseUnusedParametersInCoroutine( ArrayRef<ParmVarDecl *> Parameters, - llvm::SmallSet<ParmVarDecl *, 4> *CoroutineBodyRefs = nullptr); + const llvm::SmallSet<ParmVarDecl *, 4> &CoroutineBodyRefs); /// 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 diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 55fe6c0bb939446..3ace0424bea5a67 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15087,22 +15087,32 @@ ParmVarDecl *Sema::BuildParmVarDeclForTypedef(DeclContext *DC, return Param; } -void Sema::DiagnoseUnusedParameters( - ArrayRef<ParmVarDecl *> Parameters, - llvm::SmallSet<ParmVarDecl *, 4> *CoroutineBodyRefs) { +void Sema::DiagnoseUnusedParameters(ArrayRef<ParmVarDecl *> Parameters) { // Don't diagnose unused-parameter errors in template instantiations; we // will already have done so in the template itself. if (inTemplateInstantiation()) return; - auto isReferenced = [&](const ParmVarDecl *Parameter) { - if (CoroutineBodyRefs) { - if (CoroutineBodyRefs->count(Parameter)) - return true; - } else if (Parameter->isReferenced()) - return true; + for (const ParmVarDecl *Parameter : Parameters) { + if (!Parameter->isReferenced() && Parameter->getDeclName() && + !Parameter->hasAttr<UnusedAttr>() && + !Parameter->getIdentifier()->isPlaceholder()) { + Diag(Parameter->getLocation(), diag::warn_unused_parameter) + << Parameter->getDeclName(); + } + } +} - return false; +void Sema::DiagnoseUnusedParametersInCoroutine( + ArrayRef<ParmVarDecl *> Parameters, + const llvm::SmallSet<ParmVarDecl *, 4> &CoroutineBodyRefs) { + // Don't diagnose unused-parameter errors in template instantiations; we + // will already have done so in the template itself. + if (inTemplateInstantiation()) + return; + + auto isReferenced = [&](const ParmVarDecl *Parameter) -> bool { + return CoroutineBodyRefs.count(Parameter); }; for (const ParmVarDecl *Parameter : Parameters) { @@ -15110,7 +15120,7 @@ void Sema::DiagnoseUnusedParameters( !Parameter->hasAttr<UnusedAttr>() && !Parameter->getIdentifier()->isPlaceholder()) { Diag(Parameter->getLocation(), diag::warn_unused_parameter) - << Parameter->getDeclName(); + << Parameter->getDeclName(); } } } @@ -15852,7 +15862,8 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy(); sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr; - if (getLangOpts().Coroutines && FSI->isCoroutine()) + bool IsCoroutine = FSI->isCoroutine(); + if (getLangOpts().Coroutines && IsCoroutine) CheckCompletedCoroutineBody(FD, Body); { @@ -15920,13 +15931,16 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body, if (!FD->isDeleted() && !FD->isDefaulted() && !FD->hasSkippedBody() && !FD->hasAttr<NakedAttr>()) { - if (auto CBS = dyn_cast<CoroutineBodyStmt>(Body)) { + if (IsCoroutine) { + auto *CBS = dyn_cast<CoroutineBodyStmt>(Body); + assert(CBS && "Coroutine should have a coroutine body statement!"); auto BodyAsWritten = CBS->getBody(); assert(BodyAsWritten && "Coroutine body CompoundStmt should exist!"); StmtReferenceVisitor SRV(Context); SRV.TraverseStmt(BodyAsWritten); - DiagnoseUnusedParameters(FD->parameters(), &SRV.UsedParams); + DiagnoseUnusedParametersInCoroutine(FD->parameters(), + SRV.UsedParams); } else DiagnoseUnusedParameters(FD->parameters()); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits