Skip to content

[ConstraintSystem] Form argument constraints instead of using `matchT… #19699

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.

Sign up for GitHub

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 1 commit into from
Oct 4, 2018
Merged

[ConstraintSystem] Form argument constraints instead of using `matchT… #19699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 57 additions & 17 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,14 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
/// a subscript and offer a fixit if the inputs are compatible.
bool diagnoseSubscriptMisuse(ApplyExpr *callExpr);

/// Try to diagnose common errors involving implicitly non-escaping parameters
/// of function type, giving more specific and simpler diagnostics, attaching
/// notes on the parameter, and offering fixits to insert @escaping. Returns
/// true if it detects and issues an error, false if it does nothing.
bool diagnoseNonEscapingParameterToEscaping(Expr *expr, Type srcType,
Type dstType,
ContextualTypePurpose dstPurpose);

bool visitExpr(Expr *E);
bool visitIdentityExpr(IdentityExpr *E);
bool visitTryExpr(TryExpr *E);
Expand Down Expand Up @@ -1441,12 +1449,14 @@ bool FailureDiagnosis::diagnoseGeneralConversionFailure(Constraint *constraint){
return true;
}

if (srcFT->isNoEscape() && !destFT->isNoEscape()) {
diagnose(expr->getLoc(), diag::noescape_functiontype_mismatch,
fromType, toType)
.highlight(expr->getSourceRange());
auto destPurpose = CTP_Unused;
if (constraint->getKind() == ConstraintKind::ArgumentConversion ||
constraint->getKind() == ConstraintKind::OperatorArgumentConversion)
destPurpose = CTP_CallArgument;

if (diagnoseNonEscapingParameterToEscaping(anchor, fromType, toType,
destPurpose))
return true;
}
}

// If this is a callee that mismatches an expected return type, we can emit a
Expand Down Expand Up @@ -2344,9 +2354,8 @@ static bool addTypeCoerceFixit(InFlightDiagnostic &diag, ConstraintSystem &CS,
/// of function type, giving more specific and simpler diagnostics, attaching
/// notes on the parameter, and offering fixits to insert @escaping. Returns
/// true if it detects and issues an error, false if it does nothing.
static bool tryDiagnoseNonEscapingParameterToEscaping(
Expr *expr, Type srcType, Type dstType, ContextualTypePurpose dstPurpose,
ConstraintSystem &CS) {
bool FailureDiagnosis::diagnoseNonEscapingParameterToEscaping(
Expr *expr, Type srcType, Type dstType, ContextualTypePurpose dstPurpose) {
assert(expr);
// Need to be referencing a parameter of function type
auto declRef = dyn_cast<DeclRefExpr>(expr);
Expand Down Expand Up @@ -2684,8 +2693,8 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
}

// Try for better/more specific diagnostics for non-escaping to @escaping
if (tryDiagnoseNonEscapingParameterToEscaping(expr, exprType, contextualType,
CTP, CS))
if (diagnoseNonEscapingParameterToEscaping(expr, exprType, contextualType,
CTP))
return true;

// Don't attempt fixits if we have an unsolved type variable, since
Expand Down Expand Up @@ -5482,21 +5491,52 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
}
}

auto isFailingConstraintRelevant = [&]() -> bool {
auto *constraint = CS.failedConstraint;
if (!constraint)
return false;

auto *locator = constraint->getLocator();
return locator && locator->getAnchor() == callExpr;
};

// If there is a failing constraint associated with current constraint
// system which points to the argument/parameter mismatch, let's use
// that information while re-typechecking argument expression, this
// makes it a lot easier to determine contextual mismatch.
if (CS.failedConstraint && !hasTrailingClosure) {
if (isFailingConstraintRelevant() && !hasTrailingClosure) {
auto *constraint = CS.failedConstraint;
if (constraint->getKind() == ConstraintKind::ApplicableFunction) {
if (auto *locator = constraint->getLocator()) {
if (locator->getAnchor() == callExpr) {
auto calleeType = CS.simplifyType(constraint->getSecondType());
if (auto *fnType = calleeType->getAs<FunctionType>())
argType = AnyFunctionType::composeInput(fnType->getASTContext(),
fnType->getParams(),
auto calleeType = CS.simplifyType(constraint->getSecondType());
if (auto *fnType = calleeType->getAs<FunctionType>())
argType = AnyFunctionType::composeInput(fnType->getASTContext(),
fnType->getParams(),
/*canonicalVararg=*/false);
} else if (constraint->getKind() == ConstraintKind::ArgumentConversion ||
constraint->getKind() ==
ConstraintKind::OperatorArgumentConversion) {
using PathEltKind = ConstraintLocator::PathElementKind;
// Dig up type variable which represents the overload choice that fit
// this call expression after simplifying `ApplicableFunction` constraint.
for (auto *typeVar : CS.getTypeVariables()) {
auto *locator = typeVar->getImpl().getLocator();
auto path = locator->getPath();

// Check whether this type variable in anchored at current
// expression and path ends with `apply function`, which means
// that it's related to `ApplicableFunction` constraint.
if (locator->getAnchor() != callExpr || path.empty() ||
path.back().getKind() != PathEltKind::ApplyFunction)
continue;

if (auto type = typeVar->getImpl().getFixedType(nullptr)) {
fnType = type;
if (auto *FT = fnType->getAs<AnyFunctionType>())
argType = AnyFunctionType::composeInput(FT->getASTContext(),
FT->getParams(),
/*canonicalVararg=*/false);
}
break;
}
}
}
Expand Down
8 changes: 1 addition & 7 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,10 +816,6 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
parameterBindings))
return cs.getTypeMatchFailure(locator);

// Check the argument types for each of the parameters.
ConstraintSystem::TypeMatchOptions subflags =
ConstraintSystem::TMF_GenerateConstraints;

// If this application is part of an operator, then we allow an implicit
// lvalue to be compatible with inout arguments. This is used by
// assignment operators.
Expand Down Expand Up @@ -848,9 +844,7 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
getApplyArgToParam(argIdx,
paramIdx));
auto argTy = argsWithLabels[argIdx].getOldType();
Copy link
Contributor

@slavapestov slavapestov Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A possible next step here: if this is an InOutType, the argument conversion immediately simplifies down to a BindParam. You could instead directly emit a BindParam here, bypassing the existence of InOutType altogether. This allows you to replace the getOldType() call with getPlainType()

auto result = cs.matchTypes(argTy, paramTy, subKind, subflags, loc);
if (result.isFailure())
return result;
cs.addConstraint(subKind, argTy, paramTy, loc, /*isFavored=*/false);
}
}

Expand Down
3 changes: 2 additions & 1 deletion test/attr/attr_noescape.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func takesNoEscapeClosure(_ fn : () -> Int) {
// expected-note@-5{{parameter 'fn' is implicitly non-escaping}} {{34-34=@escaping }}
// expected-note@-6{{parameter 'fn' is implicitly non-escaping}} {{34-34=@escaping }}
// expected-note@-7{{parameter 'fn' is implicitly non-escaping}} {{34-34=@escaping }}
// expected-note@-8{{parameter 'fn' is implicitly non-escaping}} {{34-34=@escaping }}
takesNoEscapeClosure { 4 } // ok

_ = fn() // ok
Expand All @@ -56,7 +57,7 @@ func takesNoEscapeClosure(_ fn : () -> Int) {
takesGenericClosure(4) { fn() } // ok.

_ = [fn] // expected-error {{converting non-escaping value to 'Element' may allow it to escape}}
_ = [doesEscape(fn)] // expected-error {{'(() -> Int) -> ()' is not convertible to '(@escaping () -> Int) -> ()'}}
_ = [doesEscape(fn)] // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

_ = [1 : fn] // expected-error {{converting non-escaping value to 'Value' may allow it to escape}}
_ = [1 : doesEscape(fn)] // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
_ = "\(doesEscape(fn))" // expected-error {{passing non-escaping parameter 'fn' to function expecting an @escaping closure}}
Expand Down