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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005
+    if (Member->getInit() && Member->getInit()->containsErrors())
+      Constructor->setInvalidDecl();
     if (Member->isBaseInitializer())
----------------
hokein wrote:
> rsmith wrote:
> > rsmith wrote:
> > > This is inappropriate. The "invalid" bit on a declaration indicates whether the declaration is invalid, not whether the definition is invalid.
> > > 
> > > What we should do is add a "contains errors" bit to `Stmt`, and mark the function body as containing errors if it has an initializer that contains an error.
> > As an example of why this distinction matters: the "contains errors" bit indicates whether external users of the declaration should ignore it / suppress errors on it, or whether they can still treat it as a regular declaration. It's not ideal for an error in the body of a function to affect the diagnostic behavior of a caller to that function, since the body is (typically) irrelevant to the validity of that caller.
> Thanks for the suggestions and clarifications!  The "invalid" bit of decl is subtle, I didn't infer it from the code before, thanks for the explanation.
> 
> Setting the decl invalid seemed much safer to us, and would avoid running a lot of unexpected code paths in clang which might violate the assumptions. 
> 
> > What we should do is add a "contains errors" bit to Stmt, and mark the function body as containing errors if it has an initializer that contains an error.
> 
> This sounds promising, but there are no free bit in Stmt at the moment :( (to add the ErrorBit to expr, we have sacrificed the `IsOMPStructuredBlock` bit, it would be easier after the ongoing FPOptions stuff finished, which will free certain bits).
> 
> since this crash is blocking recovery-expr stuff, it is prioritized for us to fix it. Maybe a compromising plan for now is to fix it in `CheckConstexprFunctionDefinition` (just moving the inspecting `InitExpr` code there) -- the crash is from `isPotentialConstantExpr` which is only called in `CheckConstexprFunctionDefinition`, should cover all cases.
I don't think the availability or otherwise of a bit in stmt is the critical factor here. Adding a bit to stmt will be a huge amount of work because (unlike expr/type) stmts don't have dependence, so there's no existing dependence propagation/handling to reuse.

When a ctor has an init expr with an error, I think we ultimately need to choose between:
1. ctor is constant-evaluable. (This seems obviously wrong)
2. ctor is not-constant-evaluable. (This creates spurious "must be a constant expression" diags)
3. ctor is invalid and therefore we never ask. (This creates other spurious diags due to not finding the ctor)
4. ctor-with-errors is handled specially (we find it but don't check it for constant-evaluability).

Adding a stmt bit is 4, and is certainly the best long-term direction. 2 seems like a reasonable short-term solution though. Can we modify the is-constexpr check to return false if errors are present, before asserting there's no dependent code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77041