[Clang] [Sema] Ensure noexcept(typeid(E)) checks if E throws when needed

[MitalAshok]

3ad31e1 changed it so that not all potentially-evaluated typeids were marked as potentially-throwing, but I forgot to check the subexpression if the null check of the typeid didn't potentially-throw. This adds that check.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

3ad31e1 changed it so that not all potentially-evaluated typeids were marked as potentially-throwing, but I forgot to check the subexpression if the null check of the typeid didn't potentially-throw. This adds that check.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaExceptionSpec.cpp (+7-2)
• (modified) clang/test/SemaCXX/cxx0x-noexcept-expression.cpp (+18)

diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index 67e0c7c63909e..ef1128cedf994 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -1111,13 +1111,18 @@ static CanThrowResult canDynamicCastThrow(const CXXDynamicCastExpr *DC) {
 }
 
 static CanThrowResult canTypeidThrow(Sema &S, const CXXTypeidExpr *DC) {
-  if (DC->isTypeOperand())
+  // Operand is not evaluated, cannot possibly throw
+  if (!DC->isPotentiallyEvaluated())
     return CT_Cannot;
 
   if (DC->isValueDependent())
     return CT_Dependent;
 
-  return DC->hasNullCheck() ? CT_Can : CT_Cannot;
+  // Can throw std::bad_typeid if a nullptr is dereferenced
+  if (DC->hasNullCheck())
+    return CT_Can;
+
+  return S.canThrow(DC->getExprOperand());
 }
 
 CanThrowResult Sema::canThrow(const Stmt *S) {
diff --git a/clang/test/SemaCXX/cxx0x-noexcept-expression.cpp b/clang/test/SemaCXX/cxx0x-noexcept-expression.cpp
index c2b2244c117a0..1e86a31fffcbf 100644
--- a/clang/test/SemaCXX/cxx0x-noexcept-expression.cpp
+++ b/clang/test/SemaCXX/cxx0x-noexcept-expression.cpp
@@ -1,6 +1,10 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s -fexceptions -fcxx-exceptions -Wno-unevaluated-expression
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s -fexceptions -fcxx-exceptions -Wno-unevaluated-expression -fexperimental-new-constant-interpreter
 
+namespace std {
+struct type_info;
+}
+
 void f(); // expected-note {{possible target for call}}
 void f(int); // expected-note {{possible target for call}}
 
@@ -97,3 +101,17 @@ void j() noexcept(0);
 void k() noexcept(1);
 void l() noexcept(2); // expected-error {{noexcept specifier argument evaluates to 2, which cannot be narrowed to type 'bool'}}
 } // namespace P1401
+
+template<bool NoexceptConstructor, bool NoexceptDestructor>
+struct Polymorphic {
+  Polymorphic() noexcept(NoexceptConstructor) {}
+  virtual ~Polymorphic() noexcept(NoexceptDestructor) {}
+};
+
+static_assert(noexcept(typeid(Polymorphic<false, false>{})));  // Not evaluated (not glvalue)
+static_assert(noexcept(typeid((Polymorphic<true, true>&&) Polymorphic<true, true>{})));
+static_assert(!noexcept(typeid((Polymorphic<false, true>&&) Polymorphic<false, true>{})));
+static_assert(!noexcept(typeid((Polymorphic<true, false>&&) Polymorphic<true, false>{})));
+static_assert(!noexcept(typeid(*&(const Polymorphic<true, true>&) Polymorphic<true, true>{})));
+static_assert(!noexcept(typeid(*&(const Polymorphic<false, true>&) Polymorphic<false, true>{})));
+static_assert(!noexcept(typeid(*&(const Polymorphic<true, false>&) Polymorphic<true, false>{})));

[MitalAshok]

CC @Endilll @Sirraide

[Sirraide]

LGTM

[Sirraide]

Do we already have tests for your most recent commit (i.e. dependent expressions)? Because I recall CI passing even when that wasn’t there, but that may have been my imagination.

[MitalAshok]

@Sirraide Yes, the tests did pass because there were no tests for this.

For reference:

• llvm:main currently only handles these two wrong 89da8b3#diff-c35ff1c0977223e8ef2893fd21f3886c57b26346258850d8c879362fd9222381R113-R114 (https://godbolt.org/z/1j9Kc187f)
• The first commit in this PR didn't handle f1, f2 and f3 properly, 77b5b64#diff-c35ff1c0977223e8ef2893fd21f3886c57b26346258850d8c879362fd9222381R124-R141 (It treated them as CT_Cannot since it incorrectly assumed they weren't potentially evaluated because the dependent type isn't a polymorphic class type)
• The third commit fixed that by moving the check afterwards

[Sirraide]

Alright, this does look like we’re handling all cases now (that I can think of at least).

However, I’d like to see some comments added to f1–f3, indicating why they’re dependent, because it took me a bit of thinking to figure that out just now...

[Sirraide]

Thanks, the comments are pretty much exactly what I had in mind.

[Sirraide]

In case anyone ends up being confused by the lack of a release note here: #95718 already added one that still covers everything done in this pr seeing as this is mostly a follow-up to that one.