[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This makes me nervous, marking the constructor as invalid seems much safer. Can you show tests it regresses?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:14359
+        !ErrorsInCtorInitializer &&
         !CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
       FD->setInvalidDecl();
----------------
hokein wrote:
> The crash is in `CheckConstexprFunctionDefinition`, I tried different ways to fixing it:
> 
> 1) mark `FD` invalid when there are any errors in CtorInitailizer -- clang deliberately treats CtorDecl as valid even there are some errors in the initializer to prevent spurious diagnostics (see the cycle delegation in the test as an example), so marking them invalid may affect the quality of diagnostics;
> 
> 2) Fixing it inside `CheckConstexprFunctionDefinition` or `isPotentialConstantExpr`, but it doesn't seem to be a right layer, these functions are expected to be called on a validDecl (or at least after a few sanity checks), and emit diagnostics.
> clang deliberately treats CtorDecl as valid even there are some errors in the initializer
Do you mean currently? (when such errors result in destroying the whole init expr)
If so, it would be nice to preserve this indeed.

I'm worried that we're going to decide that a constexpr constructor is valid and then actually try to evaluate it at compile time.
What happens if we try to evaluate `constexpr int Z = X().Y` in your testcase?

> Fixing it inside CheckConstexprFunctionDefinition

I have a couple of concerns with where it's currently implemented:
 - aren't there lots of other paths we're going to call isPotentialConstantExpr and crash? CheckConstexprFunctionDefinition is large and complicated, init-lists are only a small part.
 - if we treat anything with errors in the ctor as valid (am I reading that right?!) then won't that mask *other* problems where the non-error parts of the definition should cause it to be treated as invalid? e.g. `Foo() : m1(broken_but_maybe_constexpr()), m2(exists_and_not_constexpr()) {}`

Wherever this goes, if we're going to do something subtle like marking diagnostics as valid based on errors in them, it deserves a comment laying out the cases.


================
Comment at: clang/test/SemaCXX/invalid-constructor-init.cpp:17
+  CycleDelegate(int) : Y(foo()) {} // expected-error {{use of undeclared identifier 'foo'}}
+  // no "delegation cycle" diagnostic emitted!
+  CycleDelegate(float) : CycleDelegate(1) {}
----------------
what's the behavior with -fno-recovery-ast?
(It'd be nice to improve it, failing to improve it is OK. regressing is probably bad...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77041/new/

https://reviews.llvm.org/D77041