[PATCH] D121824: [clang] Do not crash on arrow operator on dependent type.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry, I thought this crash is fixed as a bonus effort by improving the AST for reference-type var-decl.

Beyond the crash cased by the dependent-type `RecoveryExpr` built in `Sema::BuildDeclarationNameExpr`, I think there is another issue which is worth fixing: whether we should mark the reference-type var-decl valid if it doesn't have an initializer (either the initializer is not provided in the source code or too broken to preserve). The current AST behavior is inconsistent:

  int &a; // var-decl a is invalid without an initializer
  int &b = undefine[1111]; // var-decl b is valid without an initializer
  int &c = undefine; // var-decl c is valid with a recovery-expr initializer

>From the AST point of view, there is no difference between `a` and `b`, they both don't have an initializer, but their valid bits are different. IMO marking b invalid is an improvement, the valid bit is consistent for refer-type var-decls without initializers; it avoids the bogus `requires an initializer` diagnostic during template instantiations; as a bonus, it seems to "fix" the crash (at least for the original testcase).

Another option would be to mark `a` valid regardless of the initializer. Pros: preserve more AST nodes for `a`; the valid bit are consistent among three cases. Cons: unknown? whether it'll break some subtle invariants in clang; the bogus `requires an initializer` diagnostic is still there.

But yeah, this is a separate issue, we should fix the crash first.



================
Comment at: clang/lib/Sema/TreeTransform.h:14708
   } else if (Op == OO_Arrow) {
+    if (First->getType()->isDependentType())
+      return ExprError();
----------------
I'm not sure this is the only place triggering the crash, it looks like that we're fixing a symptom.

While here, `First` refers to a dependent-type `RecoveryExpr` (which is built from the code path: `TransformDeclRefExpr` -> `RebuildDeclRefExpr`-> `Sema::BuildDeclarationNameExpr`). So I think we have a high chance that the `RecoveryExpr` will spread multiple places in the `TreeTransform` and cause similar violations in other places.

A safe fix will be to not build the `RecoveryExpr` in `TreeTransform::TransformDeclRefExpr` -- `Sema::BuildDeclarationNameExpr` has a `AcceptInvalidDecl` parameter (by default it is false), we could reuse it to control whether we should build a `RecoveryExpr`.
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121824