[PATCH] D92298: [AST][RecoveryAST] Preserve type for member call expr if argments are not matched.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D92298#2439325 <https://reviews.llvm.org/D92298#2439325>, @sammccall wrote:

> The thrust of this change is great, I'm just having trouble being sure there are no bad side-effects.
>
> ---
>
> I almost left a comment here saying we might want to recover here... before noticing that we recover all the way up at ParsePostfixExpressionSuffix().
>
> It may be worth a comment on the function implementation that RecoveryExpr fallback is done at a higher level.
> But on this note, I think we have to consider the effect on *other* callsites.
> For example `Sema::tryExprAsCall` checks if the result is non-null in order to produce "function must be callled, did you mean to call with no arguments"... but now I think we're returning non-null even if args are required. The fix there is to check containsErrors too.
>
> There are a bunch of "internal" uses to Sema::BuildCallExpr (which calls this function), like coroutines, decomposition, pseudo-object etc that might need to be checked.
>
> Finally BuildRecoveryCallExpr in SemaOverload itself calls BuildCallExpr, and maybe we can end up with a RecoveryExpr wrapping a RecoveryExpr.
>
> ---
>
> I'm not sure what the most principled thing to do here is. Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type. Checking for containsErrors() where needed seems fragile. Adding a new state to ExprResult (null/error/Expr/QualType) is a pretty big change that may not be useful in general.
>
> I'm not opposed to this patch if we carefully audit all callers but this doesn't feel great. Maybe an ugly "allowRecovery" flag to BuildCallExpr is the right compromise, at least it forces us to be explicit.

This is a good point, thanks for raising it.

> Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type.

This looks reasonable. Currently we already perform the recovery when a CallExpr node is failed to build (in ParseExpr.cpp), but the type is always dependent. We could add some heuristics there to propagate the type as the Fn is known (but we might end up with duplicated logics as those in `SemaOverload.cpp`)

so yeah, adding a flag to BuildCallExpr and other related methods seems like a right compromise, it doesn't hurt the existing diagnostics while allowing us to preserve the type, and https://reviews.llvm.org/D80109 also needs that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92298