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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: rsmith.
sammccall added a comment.

In D121824#3400720 <https://reviews.llvm.org/D121824#3400720>, @hokein wrote:

> 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:

Agree about the inconsistency. I think ideally we'd move in the direction of *not* marking such things invalid. Here's the quote from Richard:

In D76831#1973053 <https://reviews.llvm.org/D76831#1973053>, @rsmith wrote:

> Generally I think we should be moving towards finer-grained "invalid" / "contains errors" bits, so that we can preserve as much of the AST as possible and produce accurate diagnostics for independent errors wherever possible. To that end, I think generally the "invalid" bit on a declaration should concern *only* the "declaration" part of that declaration (how it's seen from other contexts that don't "look too closely"). So in particular:
>
> - The initializer of a variable should play no part in its "invalid" bit. If the initializer expression is marked as "contains errors", then things that care about the initializer (in particular, constant evaluation and any diagnostics that look into the initializer) may need to bail out, but we should be able to form a `DeclRefExpr` referring to that variable -- even if (say) the variable is `constexpr`. Exception: if the variable has a deduced type and the type can't be deduced due to an invalid initializer, then the declaration should be marked invalid.
> - The body of a function should play no part in its "invalid" bit. (This came up in a different code review recently.)

But practically, it's at least as important to make changes that make diagnostics better and not worse, and don't introduce new crashes. This is hard to do while keeping scope limited, so I'm OK with bending principle too.

> 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.

+1



================
Comment at: clang/lib/Sema/TreeTransform.h:14708
   } else if (Op == OO_Arrow) {
+    if (First->getType()->isDependentType())
+      return ExprError();
----------------
hokein wrote:
> 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`.
>  
I agree with this FWIW: in principle it makes sense to have RecoveryExpr produced in template instantiation, in practice we probably haven't weakened the assumptions in template instantiation enough to do so safely, in the way that we have for "regular" sema.

We could try to do so in an ongoing way, but at least for syntax based tools like clangd the payoff won't be large as long as we keep mostly ignoring template instantiations.

That said, the current form of the patch is simple and fixes the crash in an obvious way, if this really is a more general problem then we'll see it again and have more data to generalize.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121824