[clang] 8222107 - [AST] Preserve the type in RecoveryExprs for broken function calls.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Mon May 11 00:09:50 PDT 2020
Author: Haojian Wu
Date: 2020-05-11T08:46:18+02:00
New Revision: 8222107aa9249aada81334c922a2d284042242ed
URL: https://github.com/llvm/llvm-project/commit/8222107aa9249aada81334c922a2d284042242ed
DIFF: https://github.com/llvm/llvm-project/commit/8222107aa9249aada81334c922a2d284042242ed.diff
LOG: [AST] Preserve the type in RecoveryExprs for broken function calls.
RecoveryExprs are modeled as dependent type to prevent bogus diagnostics
and crashes in clang.
This patch allows to preseve the type for broken calls when the
RecoveryEprs have a known type, e.g. a broken non-overloaded call, a
overloaded call when the all candidates have the same return type, so
that more features (code completion still work on "take2args(x).^") still
work.
However, adding the type is risky, which may result in more clang code being
affected leading to new crashes and hurt diagnostic, and it requires large
effort to minimize the affect (update all sites in clang to handle errorDepend
case), so we add a new flag (off by default) to allow us to develop/test
them incrementally.
This patch also has some trivial fixes to suppress diagnostics (to prevent regressions).
Tested:
all existing tests are passed (when both "-frecovery-ast", "-frecovery-ast-type" flags are flipped on);
Reviewed By: sammccall
Subscribers: rsmith, arphaman, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D79160
Added:
Modified:
clang/include/clang/AST/Expr.h
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Sema/Sema.h
clang/lib/AST/ComputeDependence.cpp
clang/lib/AST/Expr.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaOverload.cpp
clang/test/AST/ast-dump-recovery.cpp
clang/test/CodeCompletion/member-access.cpp
clang/test/Index/getcursor-recovery.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 6c83bc6c649d..0ca4941789e7 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -6097,21 +6097,25 @@ class TypoExpr : public Expr {
/// subexpressions of some expression that we could not construct and source
/// range covered by the expression.
///
-/// For now, RecoveryExpr is type-, value- and instantiation-dependent to take
-/// advantage of existing machinery to deal with dependent code in C++, e.g.
-/// RecoveryExpr is preserved in `decltype(<broken-expr>)` as part of the
+/// By default, RecoveryExpr is type-, value- and instantiation-dependent to
+/// take advantage of existing machinery to deal with dependent code in C++,
+/// e.g. RecoveryExpr is preserved in `decltype(<broken-expr>)` as part of the
/// `DependentDecltypeType`. In addition to that, clang does not report most
/// errors on dependent expressions, so we get rid of bogus errors for free.
/// However, note that unlike other dependent expressions, RecoveryExpr can be
/// produced in non-template contexts.
+/// In addition, we will preserve the type in RecoveryExpr when the type is
+/// known, e.g. preserving the return type for a broken non-overloaded function
+/// call, a overloaded call where all candidates have the same return type.
///
/// One can also reliably suppress all bogus errors on expressions containing
/// recovery expressions by examining results of Expr::containsErrors().
class RecoveryExpr final : public Expr,
private llvm::TrailingObjects<RecoveryExpr, Expr *> {
public:
- static RecoveryExpr *Create(ASTContext &Ctx, SourceLocation BeginLoc,
- SourceLocation EndLoc, ArrayRef<Expr *> SubExprs);
+ static RecoveryExpr *Create(ASTContext &Ctx, QualType T,
+ SourceLocation BeginLoc, SourceLocation EndLoc,
+ ArrayRef<Expr *> SubExprs);
static RecoveryExpr *CreateEmpty(ASTContext &Ctx, unsigned NumSubExprs);
ArrayRef<Expr *> subExpressions() {
@@ -6136,8 +6140,8 @@ class RecoveryExpr final : public Expr,
}
private:
- RecoveryExpr(ASTContext &Ctx, SourceLocation BeginLoc, SourceLocation EndLoc,
- ArrayRef<Expr *> SubExprs);
+ RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
+ SourceLocation EndLoc, ArrayRef<Expr *> SubExprs);
RecoveryExpr(EmptyShell Empty, unsigned NumSubExprs)
: Expr(RecoveryExprClass, Empty), NumExprs(NumSubExprs) {}
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 02f67636d03d..8073031e5975 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -149,6 +149,7 @@ LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template t
LANGOPT(DoubleSquareBracketAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")
COMPATIBLE_LANGOPT(RecoveryAST, 1, 0, "Preserve expressions in AST when encountering errors")
+COMPATIBLE_LANGOPT(RecoveryASTType, 1, 0, "Preserve the type in recovery expressions")
BENIGN_LANGOPT(ThreadsafeStatics , 1, 1, "thread-safe static initializers")
LANGOPT(POSIXThreads , 1, 0, "POSIX thread support")
diff --git a/clang/include/clang/Driver/CC1Options.td b/clang/include/clang/Driver/CC1Options.td
index 85407ce7eee3..7e0038e04b01 100644
--- a/clang/include/clang/Driver/CC1Options.td
+++ b/clang/include/clang/Driver/CC1Options.td
@@ -565,6 +565,10 @@ def frecovery_ast : Flag<["-"], "frecovery-ast">,
HelpText<"Preserve expressions in AST rather than dropping them when "
"encountering semantic errors">;
def fno_recovery_ast : Flag<["-"], "fno-recovery-ast">;
+def frecovery_ast_type : Flag<["-"], "frecovery-ast-type">,
+ HelpText<"Preserve the type for recovery expressions when possible "
+ "(experimental)">;
+def fno_recovery_ast_type : Flag<["-"], "fno-recovery-ast-type">;
let Group = Action_Group in {
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 66294c34a608..d4c8e483a961 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3890,7 +3890,8 @@ class Sema final {
/// Attempts to produce a RecoveryExpr after some AST node cannot be created.
ExprResult CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
- ArrayRef<Expr *> SubExprs);
+ ArrayRef<Expr *> SubExprs,
+ QualType T = QualType());
ObjCInterfaceDecl *getObjCInterfaceDecl(IdentifierInfo *&Id,
SourceLocation IdLoc,
diff --git a/clang/lib/AST/ComputeDependence.cpp b/clang/lib/AST/ComputeDependence.cpp
index 0f0f79ea1857..56c3b91c483d 100644
--- a/clang/lib/AST/ComputeDependence.cpp
+++ b/clang/lib/AST/ComputeDependence.cpp
@@ -485,9 +485,13 @@ ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx) {
}
ExprDependence clang::computeDependence(RecoveryExpr *E) {
+ // Mark the expression as value- and instantiation- dependent to reuse
+ // existing suppressions for dependent code, e.g. avoiding
+ // constant-evaluation.
// FIXME: drop type+value+instantiation once Error is sufficient to suppress
// bogus dianostics.
- auto D = ExprDependence::TypeValueInstantiation | ExprDependence::Error;
+ auto D = toExprDependence(E->getType()->getDependence()) |
+ ExprDependence::ValueInstantiation | ExprDependence::Error;
for (auto *S : E->subExpressions())
D |= S->getDependence();
return D;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 36083d3227c7..2a0e0425ef1f 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4681,25 +4681,25 @@ QualType OMPArraySectionExpr::getBaseOriginalType(const Expr *Base) {
return OriginalTy;
}
-RecoveryExpr::RecoveryExpr(ASTContext &Ctx, SourceLocation BeginLoc,
+RecoveryExpr::RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
SourceLocation EndLoc, ArrayRef<Expr *> SubExprs)
- : Expr(RecoveryExprClass, Ctx.DependentTy, VK_LValue, OK_Ordinary),
- BeginLoc(BeginLoc), EndLoc(EndLoc), NumExprs(SubExprs.size()) {
-#ifndef NDEBUG
+ : Expr(RecoveryExprClass, T, VK_LValue, OK_Ordinary), BeginLoc(BeginLoc),
+ EndLoc(EndLoc), NumExprs(SubExprs.size()) {
+ assert(!T.isNull());
for (auto *E : SubExprs)
assert(E != nullptr);
-#endif
llvm::copy(SubExprs, getTrailingObjects<Expr *>());
setDependence(computeDependence(this));
}
-RecoveryExpr *RecoveryExpr::Create(ASTContext &Ctx, SourceLocation BeginLoc,
+RecoveryExpr *RecoveryExpr::Create(ASTContext &Ctx, QualType T,
+ SourceLocation BeginLoc,
SourceLocation EndLoc,
ArrayRef<Expr *> SubExprs) {
void *Mem = Ctx.Allocate(totalSizeToAlloc<Expr *>(SubExprs.size()),
alignof(RecoveryExpr));
- return new (Mem) RecoveryExpr(Ctx, BeginLoc, EndLoc, SubExprs);
+ return new (Mem) RecoveryExpr(Ctx, T, BeginLoc, EndLoc, SubExprs);
}
RecoveryExpr *RecoveryExpr::CreateEmpty(ASTContext &Ctx, unsigned NumSubExprs) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index e3a57d654207..c5166ad41160 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2890,6 +2890,8 @@ static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK,
Diags.Report(diag::warn_fe_concepts_ts_flag);
Opts.RecoveryAST =
Args.hasFlag(OPT_frecovery_ast, OPT_fno_recovery_ast, false);
+ Opts.RecoveryASTType =
++ Args.hasFlag(OPT_frecovery_ast_type, OPT_fno_recovery_ast_type, false);
Opts.HeinousExtensions = Args.hasArg(OPT_fheinous_gnu_extensions);
Opts.AccessControl = !Args.hasArg(OPT_fno_access_control);
Opts.ElideConstructors = !Args.hasArg(OPT_fno_elide_constructors);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 73920c03e0d5..eba672d54cf8 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16426,7 +16426,7 @@ FieldDecl *Sema::CheckFieldDecl(DeclarationName Name, QualType T,
}
QualType EltTy = Context.getBaseElementType(T);
- if (!EltTy->isDependentType()) {
+ if (!EltTy->isDependentType() && !EltTy->containsErrors()) {
if (RequireCompleteSizedType(Loc, EltTy,
diag::err_field_incomplete_or_sizeless)) {
// Fields of incomplete type force their record to be invalid.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 871f3703e6d8..948880e1c136 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -18950,7 +18950,7 @@ bool Sema::IsDependentFunctionNameExpr(Expr *E) {
}
ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
- ArrayRef<Expr *> SubExprs) {
+ ArrayRef<Expr *> SubExprs, QualType T) {
// FIXME: enable it for C++, RecoveryExpr is type-dependent to suppress
// bogus diagnostics and this trick does not work in C.
// FIXME: use containsErrors() to suppress unwanted diags in C.
@@ -18960,5 +18960,8 @@ ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
if (isSFINAEContext())
return ExprError();
- return RecoveryExpr::Create(Context, Begin, End, SubExprs);
+ if (T.isNull() || !Context.getLangOpts().RecoveryASTType)
+ // We don't know the concrete type, fallback to dependent type.
+ T = Context.DependentTy;
+ return RecoveryExpr::Create(Context, T, Begin, End, SubExprs);
}
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index c400d47dd2bd..1b00b2b18572 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -12776,6 +12776,42 @@ bool Sema::buildOverloadedCallSet(Scope *S, Expr *Fn,
return false;
}
+// Guess at what the return type for an unresolvable overload should be.
+static QualType chooseRecoveryType(OverloadCandidateSet &CS,
+ OverloadCandidateSet::iterator *Best) {
+ llvm::Optional<QualType> Result;
+ // Adjust Type after seeing a candidate.
+ auto ConsiderCandidate = [&](const OverloadCandidate &Candidate) {
+ if (!Candidate.Function)
+ return;
+ QualType T = Candidate.Function->getCallResultType();
+ if (T.isNull())
+ return;
+ if (!Result)
+ Result = T;
+ else if (Result != T)
+ Result = QualType();
+ };
+
+ // Look for an unambiguous type from a progressively larger subset.
+ // e.g. if types disagree, but all *viable* overloads return int, choose int.
+ //
+ // First, consider only the best candidate.
+ if (Best && *Best != CS.end())
+ ConsiderCandidate(**Best);
+ // Next, consider only viable candidates.
+ if (!Result)
+ for (const auto &C : CS)
+ if (C.Viable)
+ ConsiderCandidate(C);
+ // Finally, consider all candidates.
+ if (!Result)
+ for (const auto &C : CS)
+ ConsiderCandidate(C);
+
+ return Result.getValueOr(QualType());
+}
+
/// FinishOverloadedCallExpr - given an OverloadCandidateSet, builds and returns
/// the completed call expression. If overload resolution fails, emits
/// diagnostics and returns ExprError()
@@ -12865,8 +12901,11 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
}
}
- // Overload resolution failed.
- return ExprError();
+ // Overload resolution failed, try to recover.
+ SmallVector<Expr *, 8> SubExprs = {Fn};
+ SubExprs.append(Args.begin(), Args.end());
+ return SemaRef.CreateRecoveryExpr(Fn->getBeginLoc(), RParenLoc, SubExprs,
+ chooseRecoveryType(*CandidateSet, Best));
}
static void markUnaddressableCandidatesUnviable(Sema &S,
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
index f0325b32dc14..3fce48a7a34e 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -1,12 +1,13 @@
-// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -frecovery-ast -ast-dump %s | FileCheck -strict-whitespace %s
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -frecovery-ast -frecovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -fcxx-exceptions -std=gnu++17 -fno-recovery-ast -ast-dump %s | FileCheck --check-prefix=DISABLED -strict-whitespace %s
int some_func(int *);
// CHECK: VarDecl {{.*}} invalid_call
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
-// CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} 'some_func'
-// CHECK-NEXT: `-IntegerLiteral {{.*}} 123
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT: `-RecoveryExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} 'some_func'
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 123
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
int invalid_call = some_func(123);
@@ -14,14 +15,15 @@ int ambig_func(double);
int ambig_func(float);
// CHECK: VarDecl {{.*}} ambig_call
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
-// CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} 'ambig_func'
-// CHECK-NEXT: `-IntegerLiteral {{.*}} 123
+// CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT: `-RecoveryExpr {{.*}} 'int' contains-errors
+// CHECK-NEXT: |-UnresolvedLookupExpr {{.*}} 'ambig_func'
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 123
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
int ambig_call = ambig_func(123);
// CHECK: VarDecl {{.*}} unresolved_call1
-// CHECK-NEXT:`-RecoveryExpr {{.*}} contains-errors
+// CHECK-NEXT:`-RecoveryExpr {{.*}} '<dependent type>' contains-errors
// CHECK-NEXT: `-UnresolvedLookupExpr {{.*}} 'bar'
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
int unresolved_call1 = bar();
diff --git a/clang/test/CodeCompletion/member-access.cpp b/clang/test/CodeCompletion/member-access.cpp
index 6d7cf6ac7cd7..6fe4d1f6a274 100644
--- a/clang/test/CodeCompletion/member-access.cpp
+++ b/clang/test/CodeCompletion/member-access.cpp
@@ -273,3 +273,12 @@ void testOverloadOperator() {
// RUN: --implicit-check-not="[#char#]operator=("
// CHECK-OPER: [#int#]operator=(
+struct S { int member; };
+S overloaded(int);
+S overloaded(double);
+void foo() {
+ // No overload matches, but we have recovery-expr with the correct type.
+ overloaded().
+}
+// RUN: not %clang_cc1 -fsyntax-only -frecovery-ast -frecovery-ast-type -code-completion-at=%s:281:16 %s -o - | FileCheck -check-prefix=CHECK-RECOVERY %s
+// CHECK-RECOVERY: [#int#]member
diff --git a/clang/test/Index/getcursor-recovery.cpp b/clang/test/Index/getcursor-recovery.cpp
index 29966f26c824..69b1403e8edb 100644
--- a/clang/test/Index/getcursor-recovery.cpp
+++ b/clang/test/Index/getcursor-recovery.cpp
@@ -2,15 +2,24 @@ int foo(int, int);
int foo(int, double);
int x;
-void testTypedRecoveryExpr() {
- // Inner foo() is a RecoveryExpr, outer foo() is an overloaded call.
- foo(x, foo(x));
+void testTypedRecoveryExpr1() {
+ // Inner bar() is an unresolved overloaded call, outer foo() is an overloaded call.
+ foo(x, bar(x));
}
-// RUN: c-index-test -cursor-at=%s:7:3 %s -Xclang -frecovery-ast | FileCheck -check-prefix=OUTER-FOO %s
+// RUN: c-index-test -cursor-at=%s:7:3 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=OUTER-FOO %s
// OUTER-FOO: OverloadedDeclRef=foo[2:5, 1:5]
-// RUN: c-index-test -cursor-at=%s:7:7 %s -Xclang -frecovery-ast | FileCheck -check-prefix=OUTER-X %s
+// RUN: c-index-test -cursor-at=%s:7:7 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=OUTER-X %s
// OUTER-X: DeclRefExpr=x:3:5
-// RUN: c-index-test -cursor-at=%s:7:10 %s -Xclang -frecovery-ast | FileCheck -check-prefix=INNER-FOO %s
-// INNER-FOO: OverloadedDeclRef=foo[2:5, 1:5]
-// RUN: c-index-test -cursor-at=%s:7:14 %s -Xclang -frecovery-ast | FileCheck -check-prefix=INNER-X %s
+// RUN: c-index-test -cursor-at=%s:7:10 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=INNER-FOO %s
+// INNER-FOO: OverloadedDeclRef=bar
+// RUN: c-index-test -cursor-at=%s:7:14 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=INNER-X %s
// INNER-X: DeclRefExpr=x:3:5
+
+void testTypedRecoveryExpr2() {
+ // Inner foo() is a RecoveryExpr (with int type), outer foo() is a valid "foo(int, int)" call.
+ foo(x, foo(x));
+}
+// RUN: c-index-test -cursor-at=%s:20:3 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=TEST2-OUTER %s
+// TEST2-OUTER: DeclRefExpr=foo:1:5
+// RUN: c-index-test -cursor-at=%s:20:10 %s -Xclang -frecovery-ast -Xclang -frecovery-ast-type | FileCheck -check-prefix=TEST2-INNER %s
+// TEST2-INNER: OverloadedDeclRef=foo[2:5, 1:5]
More information about the cfe-commits
mailing list