Skip to content

[TypeChecker] Always emit a fallback error if type-check failed witho… #21517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/swift/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,10 @@ class ASTContext final {
bool IsError;
};

/// Check whether current context has any errors associated with
/// ill-formed protocol conformances which haven't been produced yet.
bool hasDelayedConformanceErrors() const;

/// Add a delayed diagnostic produced while type-checking a
/// particular protocol conformance.
void addDelayedConformanceDiag(NormalProtocolConformance *conformance,
Expand Down
1 change: 1 addition & 0 deletions include/swift/AST/Builtins.def
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,7 @@ BUILTIN_SANITIZER_OPERATION(TSanInoutAccess, "tsanInoutAccess", "")
BUILTIN_TYPE_CHECKER_OPERATION(TypeJoin, type_join)
BUILTIN_TYPE_CHECKER_OPERATION(TypeJoinInout, type_join_inout)
BUILTIN_TYPE_CHECKER_OPERATION(TypeJoinMeta, type_join_meta)
BUILTIN_TYPE_CHECKER_OPERATION(TriggerFallbackDiagnostic, trigger_fallback_diagnostic)

#undef BUILTIN_TYPE_CHECKER_OPERATION

Expand Down
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -2767,6 +2767,10 @@ ERROR(type_of_expression_is_ambiguous,none,
ERROR(specific_type_of_expression_is_ambiguous,none,
"expression type %0 is ambiguous without more context", (Type))

ERROR(failed_to_produce_diagnostic,Fatal,
"failed to produce diagnostic for expression; "
"please file a bug report with your project", ())


ERROR(missing_protocol,none,
"missing protocol %0", (Identifier))
Expand Down
13 changes: 13 additions & 0 deletions lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1999,6 +1999,19 @@ LazyGenericContextData *ASTContext::getOrCreateLazyGenericContextData(
lazyLoader);
}

bool ASTContext::hasDelayedConformanceErrors() const {
for (const auto &entry : getImpl().DelayedConformanceDiags) {
auto &diagnostics = entry.getSecond();
if (std::any_of(diagnostics.begin(), diagnostics.end(),
[](const ASTContext::DelayedConformanceDiag &diag) {
return diag.IsError;
}))
return true;
}

return false;
}

void ASTContext::addDelayedConformanceDiag(
NormalProtocolConformance *conformance,
DelayedConformanceDiag fn) {
Expand Down
11 changes: 10 additions & 1 deletion lib/AST/Builtins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,12 @@ static ValueDecl *getTypeJoinMetaOperation(ASTContext &Context, Identifier Id) {
return builder.build(Id);
}

static ValueDecl *getTriggerFallbackDiagnosticOperation(ASTContext &Context,
Identifier Id) {
// () -> Void
return getBuiltinFunction(Id, {}, Context.TheEmptyTupleType);
}

static ValueDecl *getCanBeObjCClassOperation(ASTContext &Context,
Identifier Id) {
// <T> T.Type -> Builtin.Int8
Expand Down Expand Up @@ -1876,7 +1882,10 @@ ValueDecl *swift::getBuiltinValueDecl(ASTContext &Context, Identifier Id) {
return getTypeJoinInoutOperation(Context, Id);

case BuiltinValueKind::TypeJoinMeta:
return getTypeJoinMetaOperation(Context, Id);
return getTypeJoinMetaOperation(Context, Id);

case BuiltinValueKind::TriggerFallbackDiagnostic:
return getTriggerFallbackDiagnosticOperation(Context, Id);
}

llvm_unreachable("bad builtin value!");
Expand Down
4 changes: 4 additions & 0 deletions lib/Sema/CSDiag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1892,6 +1892,10 @@ Expr *FailureDiagnosis::typeCheckChildIndependently(
// the context is missing).
TypeCheckExprOptions TCEOptions = TypeCheckExprFlags::DisableStructuralChecks;

// Make sure that typechecker knows that this is an attempt
// to diagnose a problem.
TCEOptions |= TypeCheckExprFlags::SubExpressionDiagnostics;

// Don't walk into non-single expression closure bodies, because
// ExprTypeSaver and TypeNullifier skip them too.
TCEOptions |= TypeCheckExprFlags::SkipMultiStmtClosures;
Expand Down
18 changes: 18 additions & 0 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,11 @@ namespace {
if (typeOperation != TypeOperation::None)
return CS.getASTContext().TheAnyType;

// If this is `Builtin.trigger_fallback_diagnostic()`, fail
// without producing any diagnostics, in order to test fallback error.
if (isTriggerFallbackDiagnosticBuiltin(expr, CS.getASTContext()))
return Type();

// Open a member constraint for constructor delegations on the
// subexpr type.
if (CS.TC.getSelfForInitDelegationInConstructor(CS.DC, expr)) {
Expand Down Expand Up @@ -3129,6 +3134,19 @@ namespace {
return tv;
}

static bool isTriggerFallbackDiagnosticBuiltin(UnresolvedDotExpr *UDE,
ASTContext &Context) {
auto *DRE = dyn_cast<DeclRefExpr>(UDE->getBase());
if (!DRE)
return false;

if (DRE->getDecl() != Context.TheBuiltinModule)
return false;

auto member = UDE->getName().getBaseName().userFacingName();
return member.equals("trigger_fallback_diagnostic");
}

enum class TypeOperation { None,
Join,
JoinInout,
Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,8 @@ ConstraintSystem::solveImpl(Expr *&expr,
if (auto generatedExpr = generateConstraints(expr))
expr = generatedExpr;
else {
if (listener)
listener->constraintGenerationFailed(expr);
return SolutionKind::Error;
}

Expand Down
129 changes: 113 additions & 16 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "swift/AST/NameLookup.h"
#include "swift/AST/ParameterList.h"
#include "swift/AST/PrettyStackTrace.h"
#include "swift/AST/ProtocolConformance.h"
#include "swift/AST/SubstitutionMap.h"
#include "swift/AST/TypeCheckerDebugConsumer.h"
#include "swift/Basic/Statistic.h"
Expand Down Expand Up @@ -1914,6 +1915,11 @@ Expr *ExprTypeCheckListener::appliedSolution(Solution &solution, Expr *expr) {
return expr;
}

void ExprTypeCheckListener::preCheckFailed(Expr *expr) {}
void ExprTypeCheckListener::constraintGenerationFailed(Expr *expr) {}
void ExprTypeCheckListener::applySolutionFailed(Solution &solution,
Expr *expr) {}

void ParentConditionalConformance::diagnoseConformanceStack(
DiagnosticEngine &diags, SourceLoc loc,
ArrayRef<ParentConditionalConformance> conformances) {
Expand Down Expand Up @@ -1941,20 +1947,115 @@ bool GenericRequirementsCheckListener::diagnoseUnsatisfiedRequirement(
return false;
}

/// Sometimes constraint solver fails without producing any diagnostics,
/// that leads to crashes down the line in AST Verifier or SILGen
/// which, as a result, are much harder to figure out.
///
/// This class is intended to guard against situations like that by
/// keeping track of failures of different type-check phases, and
/// emitting fallback fatal error if any of them fail without producing
/// error diagnostic, and there were no errors emitted or scheduled to be
/// emitted previously.
class FallbackDiagnosticListener : public ExprTypeCheckListener {
TypeChecker &TC;
TypeCheckExprOptions Options;
ExprTypeCheckListener *BaseListener;

public:
FallbackDiagnosticListener(TypeChecker &TC, TypeCheckExprOptions options,
ExprTypeCheckListener *base)
: TC(TC), Options(options), BaseListener(base) {}

bool builtConstraints(ConstraintSystem &cs, Expr *expr) override {
return BaseListener ? BaseListener->builtConstraints(cs, expr) : false;
}

Expr *foundSolution(Solution &solution, Expr *expr) override {
return BaseListener ? BaseListener->foundSolution(solution, expr) : expr;
}

Expr *appliedSolution(Solution &solution, Expr *expr) override {
return BaseListener ? BaseListener->appliedSolution(solution, expr) : expr;
}

void preCheckFailed(Expr *expr) override {
if (BaseListener)
BaseListener->preCheckFailed(expr);
maybeProduceFallbackDiagnostic(expr);
}

void constraintGenerationFailed(Expr *expr) override {
if (BaseListener)
BaseListener->constraintGenerationFailed(expr);
maybeProduceFallbackDiagnostic(expr);
}

void applySolutionFailed(Solution &solution, Expr *expr) override {
if (BaseListener)
BaseListener->applySolutionFailed(solution, expr);

if (hadAnyErrors())
return;

// If solution involves invalid or incomplete conformances that's
// a probable cause of failure to apply it without producing an error,
// which is going to be diagnosed later, so let's not produce
// fallback diagnostic in this case.
if (llvm::any_of(
solution.Conformances,
[](const std::pair<ConstraintLocator *, ProtocolConformanceRef>
&conformance) -> bool {
auto &ref = conformance.second;
return ref.isConcrete() && ref.getConcrete()->isInvalid();
}))
return;

maybeProduceFallbackDiagnostic(expr);
}

private:
bool hadAnyErrors() const { return TC.Context.Diags.hadAnyError(); }

void maybeProduceFallbackDiagnostic(Expr *expr) const {
if (Options.contains(TypeCheckExprFlags::SubExpressionDiagnostics) ||
Options.contains(TypeCheckExprFlags::SuppressDiagnostics))
return;

// Before producing fatal error here, let's check if there are any "error"
// diagnostics already emitted or waiting to be emitted. Because they are
// a better indication of the problem.
if (!(hadAnyErrors() || TC.Context.hasDelayedConformanceErrors()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm confused as to why we have both the TC.Context.hasDelayedConformanceErrors() check and the specific isInvalid() checks above. Doesn't this one suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the situation I came up with was - solution application failed without diagnostic and it was unrelated to invalid conformances in the current expression, but we already had errors like that record by previous expressions so instead of producing fatal error here, let's just keep going since when delayed conformances are processed they are going to produce one or more errors anyway...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay!

TC.diagnose(expr->getLoc(), diag::failed_to_produce_diagnostic);
}
};

#pragma mark High-level entry points
Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
TypeLoc convertType,
ContextualTypePurpose convertTypePurpose,
TypeCheckExprOptions options,
ExprTypeCheckListener *listener,
ConstraintSystem *baseCS) {
FallbackDiagnosticListener diagListener(*this, options, listener);
return typeCheckExpressionImpl(expr, dc, convertType, convertTypePurpose,
options, diagListener, baseCS);
}

Type TypeChecker::typeCheckExpressionImpl(Expr *&expr, DeclContext *dc,
TypeLoc convertType,
ContextualTypePurpose convertTypePurpose,
TypeCheckExprOptions options,
ExprTypeCheckListener &listener,
ConstraintSystem *baseCS) {
FrontendStatsTracer StatsTracer(Context.Stats, "typecheck-expr", expr);
PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);

// First, pre-check the expression, validating any types that occur in the
// expression and folding sequence expressions.
if (preCheckExpression(expr, dc))
if (preCheckExpression(expr, dc)) {
listener.preCheckFailed(expr);
return Type();
}

// Construct a constraint system from this expression.
ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;
Expand Down Expand Up @@ -2009,10 +2110,9 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
convertTo = getOptionalType(expr->getLoc(), var);
}

// Attempt to solve the constraint system.
SmallVector<Solution, 4> viable;
if (cs.solve(expr, convertTo, listener, viable,
allowFreeTypeVariables))
// Attempt to solve the constraint system.
if (cs.solve(expr, convertTo, &listener, viable, allowFreeTypeVariables))
return Type();

// If the client allows the solution to have unresolved type expressions,
Expand All @@ -2026,29 +2126,26 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,

auto result = expr;
auto &solution = viable[0];
if (listener) {
result = listener->foundSolution(solution, result);
if (!result)
return Type();
}
result = listener.foundSolution(solution, result);
if (!result)
return Type();

// Apply the solution to the expression.
result = cs.applySolution(
solution, result, convertType.getType(),
options.contains(TypeCheckExprFlags::IsDiscarded),
options.contains(TypeCheckExprFlags::SkipMultiStmtClosures));

if (!result) {
listener.applySolutionFailed(solution, expr);
// Failure already diagnosed, above, as part of applying the solution.
return Type();
}

// If there's a listener, notify it that we've applied the solution.
if (listener) {
result = listener->appliedSolution(solution, result);
if (!result) {
return Type();
}
}
// Notify listener that we've applied the solution.
result = listener.appliedSolution(solution, result);
if (!result)
return Type();

if (getLangOpts().DebugConstraintSolver) {
auto &log = Context.TypeCheckerDebug->getStream();
Expand Down
28 changes: 28 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,14 @@ enum class TypeCheckExprFlags {
/// If set, a conversion constraint should be specified so that the result of
/// the expression is an optional type.
ExpressionTypeMustBeOptional = 0x200,

/// FIXME(diagnostics): Once diagnostics are completely switched to new
/// framework, this flag could be removed as obsolete.
///
/// If set, this is a sub-expression, and it is being re-typechecked
/// as part of the expression diagnostics, which is attempting to narrow
/// down failure location.
SubExpressionDiagnostics = 0x400,
};

using TypeCheckExprOptions = OptionSet<TypeCheckExprFlags>;
Expand Down Expand Up @@ -381,6 +389,18 @@ class ExprTypeCheckListener {
/// failure.
virtual Expr *appliedSolution(constraints::Solution &solution,
Expr *expr);

/// Callback invoked if expression is structurally unsound and can't
/// be correctly processed by the constraint solver.
virtual void preCheckFailed(Expr *expr);

/// Callback invoked if constraint system failed to generate
/// constraints for a given expression.
virtual void constraintGenerationFailed(Expr *expr);

/// Callback invoked if application of chosen solution to
/// expression has failed.
virtual void applySolutionFailed(constraints::Solution &solution, Expr *expr);
};

/// A conditional conformance that implied some other requirements. That is, \c
Expand Down Expand Up @@ -1330,7 +1350,15 @@ class TypeChecker final : public LazyResolver {
TypeCheckExprOptions(), listener);
}

private:
Type typeCheckExpressionImpl(Expr *&expr, DeclContext *dc,
TypeLoc convertType,
ContextualTypePurpose convertTypePurpose,
TypeCheckExprOptions options,
ExprTypeCheckListener &listener,
constraints::ConstraintSystem *baseCS);

public:
/// Type check the given expression and return its type without
/// applying the solution.
///
Expand Down
10 changes: 10 additions & 0 deletions test/Sema/rdar38885760.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// RUN: %empty-directory(%t)
//
// RUN: not %target-swift-frontend -typecheck %s -parse-stdlib 2>%t/fallback_diagnostic.txt
// RUN: %FileCheck %s --check-prefix FALLBACK-DIAGNOSTIC <%t/fallback_diagnostic.txt
//
// FALLBACK-DIAGNOSTIC: error: failed to produce diagnostic for expression; please file a bug report with your project

import Swift

Builtin.trigger_fallback_diagnostic()