[clang][Sema] Reject negative tuple sizes

[tbaederr]

Negative sizes don't make sense and trip up the code using UnsignedOrNone.

Fixes #159563

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Negative sizes don't make sense and trip up the code using UnsignedOrNone.

Fixes #159563

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+1-1)
• (modified) clang/test/SemaCXX/builtin-structured-binding-size.cpp (+9)

diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index fb57b43882911..2acd07cc5a6f1 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -1219,7 +1219,7 @@ static IsTupleLike isTupleLike(Sema &S, SourceLocation Loc, QualType T,
     return IsTupleLike::Error;
 
   E = S.VerifyIntegerConstantExpression(E.get(), &Size, Diagnoser);
-  if (E.isInvalid())
+  if (E.isInvalid() || Size.isNegative())
     return IsTupleLike::Error;
 
   return IsTupleLike::TupleLike;
diff --git a/clang/test/SemaCXX/builtin-structured-binding-size.cpp b/clang/test/SemaCXX/builtin-structured-binding-size.cpp
index 53576048754ab..de881a539310c 100644
--- a/clang/test/SemaCXX/builtin-structured-binding-size.cpp
+++ b/clang/test/SemaCXX/builtin-structured-binding-size.cpp
@@ -229,3 +229,12 @@ static_assert(__is_same_as(tag_of_t<S1>, int));
 static_assert(__is_same_as(tag_of_t<int>, int)); // error
 // expected-error@-1 {{constraints not satisfied for alias template 'tag_of_t' [with T = int]}}
 // expected-note@#tag-of-constr {{because substituted constraint expression is ill-formed: type 'int' cannot be decomposed}}
+
+struct Neg {
+  int a;
+};
+template <> struct std::tuple_size<Neg> {
+  static constexpr int value = -1;
+};
+
+int e = __builtin_structured_binding_size(Neg); // expected-error {{type 'Neg' cannot be decomposed}}

[cor3ntin]

We should backport that

[shafik]

You mention UnsignedOrNone in the summary but that is not directly involved in the code you modify. It looks like it is used in GetDecompositionElementCount which uses isTupleLike but the connection is not obvious. It lines up w/ the git bisect but a more detailed explaination would be helpful especially since the PR was very large and likely warrants further review and understanding the issue better would help there.

[tbaederr]

UnsignedOrNone just means we assert there when passing -1. With a full std::optional<unsigned> the value would just underflow and the value we use later would've been wrong. The incorrect handling of negative values has nothing to do with UnsignedOrNone.

[shafik]

Just for clarification isTupleLike already contains a bug by potentially returning a -1 value, this already existed. Because GetDecompositionElementCount now uses UnsignedOrNone this changes a logic error to an assertion.

[mizvekov]

I didn't realize someone else was working on it, but I propose #159677 instead, as the diagnostics produced are more complete.