[Clang] Fix dependency computation for pack indexing expression #91933

cor3ntin · 2024-05-13T08:02:35Z

Given foo...[idx] if idx is value dependent, the expression is type dependent.

Fixes #91885
Fixes #91884

llvmbot · 2024-05-13T08:03:11Z

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

Given foo...[idx] if idx is value dependent, the expression is type dependent.

Fixes #91885
Fixes #91884

Full diff: https://github.com/llvm/llvm-project/pull/91933.diff

2 Files Affected:

(modified) clang/lib/AST/ComputeDependence.cpp (+3)
(modified) clang/test/SemaCXX/cxx2c-pack-indexing.cpp (+14)

diff --git a/clang/lib/AST/ComputeDependence.cpp b/clang/lib/AST/ComputeDependence.cpp
index bad8e75b2f878..ee56c50d76512 100644
--- a/clang/lib/AST/ComputeDependence.cpp
+++ b/clang/lib/AST/ComputeDependence.cpp
@@ -376,6 +376,9 @@ ExprDependence clang::computeDependence(PackExpansionExpr *E) {
 
 ExprDependence clang::computeDependence(PackIndexingExpr *E) {
   ExprDependence D = E->getIndexExpr()->getDependence();
+  if (D & ExprDependence::Value)
+    D |= ExprDependence::TypeInstantiation;
+
   ArrayRef<Expr *> Exprs = E->getExpressions();
   if (Exprs.empty())
     D |= (E->getPackIdExpression()->getDependence() |
diff --git a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
index a3e5a0931491b..764f6163710bd 100644
--- a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
+++ b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
@@ -194,3 +194,17 @@ void h() {
   // expected-note-re@-2 {{function template specialization '{{.*}}' requested here}}
 }
 }
+
+namespace GH91885 {
+
+void test(auto...args){
+    [&]<int idx>(){
+        using R = decltype( args...[idx] ) ;
+    }.template operator()<0>();
+}
+
+void f( ) {
+    test(1);
+}
+
+}

Given `foo...[idx]` if idx is value dependent, the expression is type dependent. Fixes llvm#91885 Fixes llvm#91884

zyn0217 · 2024-05-13T08:54:08Z

clang/lib/AST/ComputeDependence.cpp

@@ -376,6 +376,9 @@ ExprDependence clang::computeDependence(PackExpansionExpr *E) {

 ExprDependence clang::computeDependence(PackIndexingExpr *E) {
  ExprDependence D = E->getIndexExpr()->getDependence();
+  if (D & ExprDependence::Value)
+    D |= ExprDependence::TypeInstantiation;


Does it mean "if the index expression is value-dependent, then the whole PackIndexingExpr is type-dependent despite its expressions"? For example,

template <int N, int... E> auto foo() { return E...[N]; }

Should E...[N] be treated as type-dependent regardless of the result being always an int?

Probably E->getPackIdExpression()->getDependence() & ExprDependence::TypeInstantiation?

Should E...[N] be treated as type-dependent regardless of the result being always an int?

Yes. (We don't do anything to look at whether all the expressions would have the same type)

NVM, you are right, we can simplify this whole thing, I'll do that shortly

RungeCC · 2024-05-13T13:38:43Z

clang/lib/AST/ComputeDependence.cpp

@@ -377,7 +377,7 @@ ExprDependence clang::computeDependence(PackExpansionExpr *E) {
 ExprDependence clang::computeDependence(PackIndexingExpr *E) {
  ExprDependence D = E->getIndexExpr()->getDependence();
  if (D & ExprDependence::Value)
-    D |= ExprDependence::TypeInstantiation;
+    D |= E->getPackIdExpression()->getDependence() & ExprDependence::Type;


The change seems reasonable to me, however with it we need to alter the way PackIndexingExpr is handled inside the getDecltypeForExpr, since when 1) E is not type-dependent (int... case) and 2) index is instantiation-dependent we shall not directly call getSelectedExpr.

llvm-project/clang/lib/Sema/SemaType.cpp

Lines 9739 to 9748 in ad1a65f

QualType Sema::getDecltypeForExpr(Expr *E) {

if (E->isTypeDependent())

return Context.DependentTy;

Expr *IDExpr = E;

if (auto *ImplCastExpr = dyn_cast<ImplicitCastExpr>(E))

IDExpr = ImplCastExpr->getSubExpr();

if (auto *PackExpr = dyn_cast<PackIndexingExpr>(E))

IDExpr = PackExpr->getSelectedExpr();

llvm-project/clang/include/clang/AST/ExprCXX.h

Lines 4411 to 4415 in ad1a65f

Expr *getSelectedExpr() const {

std::optional<unsigned> Index = getSelectedIndex();

assert(Index && "extracting the indexed expression of a dependant pack");

return getTrailingObjects<Expr *>()[*Index];

}

In short, crash on the following code:

template<int...args> // notice the "int..." here. void test(){ [&]<int idx>(){ using R = decltype( args...[idx] ) ; }.template operator()<0>(); } void f( ) { test<1>(); }

Possibly PackExpr->getExpressions().front() for the type-independent PackExpr with its IndexExpr instantiation-dependent? I'm not sure whether we need an empty-check here.

The change seems reasonable to me, however with it we need to alter the way PackIndexingExpr is handled inside the getDecltypeForExpr, since when 1) E is not type-dependent (int... case) and 2) index is instantiation-dependent we shall not directly call getSelectedExpr.

In that case (no selected expression) the pack is always type dependent. I did add a test though

In that case (no selected expression) the pack is always type dependent.

I don't get it, could you explain the reason? FWIW, new test failed in CI so we may need to reconsider this.

zyn0217 · 2024-05-14T02:47:46Z

clang/lib/Sema/SemaType.cpp

@@ -9806,7 +9806,7 @@ QualType Sema::BuildCountAttributedArrayType(QualType WrappedTy,
 /// that expression, according to the rules in C++11
 /// [dcl.type.simple]p4 and C++11 [expr.lambda.prim]p18.
 QualType Sema::getDecltypeForExpr(Expr *E) {
-  if (E->isTypeDependent())
+  if (E->isInstantiationDependent())


Is it... right? I was assuming this would bring such regressions e.g.

template <class T> const int val = sizeof(T); template <class T> void foo() { decltype(val<T>); // <-- this is instantiation-dependent, but not type-dependent. }

Perhaps we can move it to line 9816 where the PackIndexingExpr is handled?

Perhaps we can move it to line 9816 where the PackIndexingExpr is handled?

I would be slightly concerned about having specific handling of PackIndexingExpr all over the place,
or about decltype behaving differently for pack indexing.

Then again, I'm not sure there is a scenario where it matters.

So here is a thought.
If the pack indexing expression is not type dependent, decltype should just produce the type of that expression.
By that logic we do want to modify decltype so that it does something more clever than to try to use the selected expression.

I'll try to do something in that direction, but it's unclear to me under which set of circumstances we can observe that.

If a pack indexing expression has no selected expression but has a non-tytpe dependent pattern, we can use that type to compute the decltyoe type.

zyn0217

Thanks for the prompt fix! I think this looks good now, but please wait for @erichkeane for the final stamp.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 13, 2024

[Clang] Fix dependency computation for pack indexing expression

4cadff5

Given `foo...[idx]` if idx is value dependent, the expression is type dependent. Fixes llvm#91885 Fixes llvm#91884

cor3ntin force-pushed the fix_pack_indexing2 branch from 066e23d to 4cadff5 Compare May 13, 2024 08:32

zyn0217 reviewed May 13, 2024

View reviewed changes

address review feedback

7c9192c

cor3ntin requested review from AaronBallman and erichkeane May 13, 2024 12:50

RungeCC reviewed May 13, 2024

View reviewed changes

cor3ntin added 3 commits May 13, 2024 15:56

add additional test

320b9fe

Fix tests?

bb6b680

Fix getDecltypeForExpr + cleanup

fdd321f

zyn0217 reviewed May 14, 2024

View reviewed changes

More correct decltype type computation

10246f2

If a pack indexing expression has no selected expression but has a non-tytpe dependent pattern, we can use that type to compute the decltyoe type.

zyn0217 approved these changes May 14, 2024

View reviewed changes

erichkeane approved these changes May 14, 2024

View reviewed changes

cor3ntin merged commit 312f83f into llvm:main May 14, 2024
3 of 4 checks passed

cor3ntin deleted the fix_pack_indexing2 branch May 14, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Fix dependency computation for pack indexing expression #91933

[Clang] Fix dependency computation for pack indexing expression #91933

cor3ntin commented May 13, 2024

llvmbot commented May 13, 2024

zyn0217 May 13, 2024

zyn0217 May 13, 2024

cor3ntin May 13, 2024

cor3ntin May 13, 2024

RungeCC May 13, 2024

RungeCC May 13, 2024

cor3ntin May 13, 2024

RungeCC May 13, 2024

zyn0217 May 14, 2024

cor3ntin May 14, 2024

cor3ntin May 14, 2024

zyn0217 left a comment

	QualType Sema::getDecltypeForExpr(Expr *E) {
	if (E->isTypeDependent())
	return Context.DependentTy;

	Expr *IDExpr = E;
	if (auto *ImplCastExpr = dyn_cast<ImplicitCastExpr>(E))
	IDExpr = ImplCastExpr->getSubExpr();

	if (auto *PackExpr = dyn_cast<PackIndexingExpr>(E))
	IDExpr = PackExpr->getSelectedExpr();

	Expr *getSelectedExpr() const {
	std::optional<unsigned> Index = getSelectedIndex();
	assert(Index && "extracting the indexed expression of a dependant pack");
	return getTrailingObjects<Expr >()[Index];
	}

[Clang] Fix dependency computation for pack indexing expression #91933

[Clang] Fix dependency computation for pack indexing expression #91933

Conversation

cor3ntin commented May 13, 2024

llvmbot commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyn0217 left a comment

Choose a reason for hiding this comment