[clang] [Clang] Warn against unused parameters in C++ coroutines with `-Wunused-parameters` (PR #70567)
Yuxuan Chen via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 31 20:54:52 PDT 2023
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 <ych at meta.com>
Date: Fri, 27 Oct 2023 16:37:40 -0700
Subject: [PATCH 1/5] [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 <ych at meta.com>
Date: Sat, 28 Oct 2023 12:42:01 -0700
Subject: [PATCH 2/5] 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 <ych at meta.com>
Date: Sat, 28 Oct 2023 12:46:30 -0700
Subject: [PATCH 3/5] 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 <ych at meta.com>
Date: Tue, 31 Oct 2023 10:06:34 -0700
Subject: [PATCH 4/5] 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 <ych at meta.com>
Date: Tue, 31 Oct 2023 20:54:19 -0700
Subject: [PATCH 5/5] 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);
More information about the cfe-commits
mailing list