Skip to content
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

[ConstraintSystem][SE-0249] Key Path Expressions as Functions #26054

Merged
merged 15 commits into from
Aug 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
14 changes: 10 additions & 4 deletions include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -3690,14 +3690,20 @@ class ClosureExpr : public AbstractClosureExpr {
}
};


/// This is a closure of the contained subexpression that is formed
/// when a scalar expression is converted to @autoclosure function type.
/// For example:
/// This is an implicit closure of the contained subexpression that is usually
/// formed when a scalar expression is converted to @autoclosure function type.
/// \code
/// func f(x : @autoclosure () -> Int)
/// f(42) // AutoclosureExpr convert from Int to ()->Int
/// \endcode
///
/// They are also created when key path expressions are converted to function
/// type, in which case, a pair of nested implicit closures are formed:
/// \code
/// { $kp$ in { $0[keyPath: $kp$] } }( \(E) )
/// \endcode
/// This is to ensure side effects of the key path expression (mainly indices in
/// subscripts) are only evaluated once.
class AutoClosureExpr : public AbstractClosureExpr {
BraceStmt *Body;

Expand Down
111 changes: 107 additions & 4 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4199,9 +4199,18 @@ namespace {

// Resolve each of the components.
bool didOptionalChain = false;
auto keyPathTy = cs.getType(E)->castTo<BoundGenericType>();
Type baseTy = keyPathTy->getGenericArgs()[0];
Type leafTy = keyPathTy->getGenericArgs()[1];
bool isFunctionType = false;
Type baseTy, leafTy;
Type exprType = cs.getType(E);
if (auto fnTy = exprType->getAs<FunctionType>()) {
baseTy = fnTy->getParams()[0].getPlainType();
leafTy = fnTy->getResult();
isFunctionType = true;
} else {
auto keyPathTy = exprType->castTo<BoundGenericType>();
baseTy = keyPathTy->getGenericArgs()[0];
leafTy = keyPathTy->getGenericArgs()[1];
}

// Updates the constraint system with the type of the last resolved
// component. We do it this way because we sometimes insert new
Expand Down Expand Up @@ -4406,7 +4415,101 @@ namespace {
// key path.
assert(!baseTy || baseTy->hasUnresolvedType()
|| baseTy->getWithoutSpecifierType()->isEqual(leafTy));
return E;

if (!isFunctionType)
return E;

// If we've gotten here, the user has used key path literal syntax to form
// a closure. The type checker has given E a function type to indicate
// this; we're going to change E's type to KeyPath<baseTy, leafTy> and
// then wrap it in a larger closure expression with the appropriate type.

// baseTy has been overwritten by the loop above; restore it.
baseTy = exprType->getAs<FunctionType>()->getParams()[0].getPlainType();

// Compute KeyPath<baseTy, leafTy> and set E's type back to it.
auto kpDecl = cs.getASTContext().getKeyPathDecl();
auto keyPathTy =
BoundGenericType::get(kpDecl, nullptr, { baseTy, leafTy });
E->setType(keyPathTy);
cs.cacheType(E);

// To ensure side effects of the key path expression (mainly indices in
// subscripts) are only evaluated once, we construct an outer closure,
// which is immediately evaluated, and an inner closure, which it returns.
// The result looks like this:
//
// return "{ $kp$ in { $0[keyPath: $kp$] } }( \(E) )"

auto &ctx = cs.getASTContext();
auto discriminator = AutoClosureExpr::InvalidDiscriminator;

// The inner closure.
//
// let closure = "{ $0[keyPath: $kp$] }"
auto closureTy =
FunctionType::get({ FunctionType::Param(baseTy) }, leafTy);
auto closure = new (ctx)
AutoClosureExpr(E, leafTy, discriminator, cs.DC);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just a regular implicit closure? I believe that by conversion autoclosure is not supposed to have any arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the discriminator. ClosureExprs get numbered during parsing, and AutoClosureExprs get numbered during a walk after type checking. The parsing context is already gone by the time this code is running, so the only way to correctly set the discriminator is via an AutoClosureExpr.

Copy link
Contributor

@xedin xedin Aug 7, 2019

Choose a reason for hiding this comment

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

Interesting, and there is no way to change that behavior to allow implicit closures? It seems a bit like a hack to use autoclosure here, because autoclosure is not supposed to have any parameters and used only in @autoclosure marked positions, so we are kind of stretching its design here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the superclass AbstractClosureExpr is what handles parameters, not either of the subclasses. The main difference between ClosureExpr and AutoClosureExpr is that ClosureExpr is supposed to be for explicit closures parsed from the source code, and AutoClosureExpr is supposed to be for implicitly created closures around an expression.

So, it's true, this is the only place where AutoClosureExpr will currently have parameters. But the design of the classes is such that I think we'd be stretching the design much more to try to use ClosureExpr here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to add a separate sub-class of AbstractClosureExpr which deals with general cases of implicit closures? I'm just trying to think through ideas here since AutoClosureExpr just seems like a specialized use-case to me. Maybe it doesn't really matter that much anyway since the only code which has to deal with that in in SILGen... I can't think of any special cases in CSDiag which deal with expressions like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least comment associated with AutoClosureExpr has to be updated to cover new use-case.

auto param = new (ctx) ParamDecl(
ParamDecl::Specifier::Default, SourceLoc(),
/*argument label*/ SourceLoc(), Identifier(),
/*parameter name*/ SourceLoc(), ctx.getIdentifier("$0"), closure);
param->setType(baseTy);
param->setInterfaceType(baseTy->mapTypeOutOfContext());

// The outer closure.
//
// let outerClosure = "{ $kp$ in \(closure) }"
auto outerClosureTy =
FunctionType::get({ FunctionType::Param(keyPathTy) }, closureTy);
auto outerClosure = new (ctx)
AutoClosureExpr(closure, closureTy, discriminator, cs.DC);
auto outerParam =
new (ctx) ParamDecl(ParamDecl::Specifier::Default, SourceLoc(),
/*argument label*/ SourceLoc(), Identifier(),
/*parameter name*/ SourceLoc(),
ctx.getIdentifier("$kp$"), outerClosure);
outerParam->setType(keyPathTy);
outerParam->setInterfaceType(keyPathTy->mapTypeOutOfContext());

// let paramRef = "$0"
auto *paramRef = new (ctx)
DeclRefExpr(param, DeclNameLoc(E->getLoc()), /*Implicit=*/true);
paramRef->setType(baseTy);
cs.cacheType(paramRef);

// let outerParamRef = "$kp$"
auto outerParamRef = new (ctx)
DeclRefExpr(outerParam, DeclNameLoc(E->getLoc()), /*Implicit=*/true);
outerParamRef->setType(keyPathTy);
cs.cacheType(outerParamRef);

// let application = "\(paramRef)[keyPath: \(outerParamRef)]"
auto *application = new (ctx)
KeyPathApplicationExpr(paramRef,
E->getStartLoc(), outerParamRef, E->getEndLoc(),
leafTy, /*implicit=*/true);
cs.cacheType(application);

// Finish up the inner closure.
closure->setParameterList(ParameterList::create(ctx, {param}));
closure->setBody(application);
closure->setType(closureTy);
cs.cacheType(closure);

// Finish up the outer closure.
outerClosure->setParameterList(ParameterList::create(ctx, {outerParam}));
outerClosure->setBody(closure);
outerClosure->setType(outerClosureTy);
cs.cacheType(outerClosure);

// let outerApply = "\( outerClosure )( \(E) )"
auto outerApply = CallExpr::createImplicit(ctx, outerClosure, {E}, {});
outerApply->setType(closureTy);
cs.cacheExprTypes(outerApply);

return coerceToType(outerApply, exprType, cs.getConstraintLocator(E));
}

KeyPathExpr::Component
Expand Down
27 changes: 10 additions & 17 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ namespace {

// Add the member constraint for a subscript declaration.
// FIXME: weak name!
auto memberTy = CS.createTypeVariable(resultLocator, TVO_CanBindToNoEscape);
auto memberTy = CS.createTypeVariable(memberLocator, TVO_CanBindToNoEscape);

// FIXME: synthesizeMaterializeForSet() wants to statically dispatch to
// a known subscript here. This might be cleaner if we split off a new
Expand Down Expand Up @@ -3013,7 +3013,7 @@ namespace {

case KeyPathExpr::Component::Kind::OptionalChain: {
didOptionalChain = true;

// We can't assign an optional back through an optional chain
// today. Force the base to an rvalue.
auto rvalueTy = CS.createTypeVariable(resultLocator,
Expand All @@ -3027,10 +3027,9 @@ namespace {
auto optionalObjTy = CS.createTypeVariable(resultLocator,
TVO_CanBindToLValue |
TVO_CanBindToNoEscape);

CS.addConstraint(ConstraintKind::OptionalObject, base, optionalObjTy,
resultLocator);

base = optionalObjTy;
break;
}
Expand Down Expand Up @@ -3065,20 +3064,14 @@ namespace {
auto rvalueBase = CS.createTypeVariable(baseLocator,
TVO_CanBindToNoEscape);
CS.addConstraint(ConstraintKind::Equal, base, rvalueBase, locator);

// The result is a KeyPath from the root to the end component.
Type kpTy;
if (didOptionalChain) {
// Optional-chaining key paths are always read-only.
kpTy = BoundGenericType::get(kpDecl, Type(), {root, rvalueBase});
} else {
// The type of key path depends on the overloads chosen for the key
// path components.
auto typeLoc =
CS.getConstraintLocator(locator, ConstraintLocator::KeyPathType);
kpTy = CS.createTypeVariable(typeLoc, TVO_CanBindToNoEscape);
CS.addKeyPathConstraint(kpTy, root, rvalueBase, locator);
}
// The type of key path depends on the overloads chosen for the key
// path components.
auto typeLoc =
CS.getConstraintLocator(locator, ConstraintLocator::KeyPathType);
Type kpTy = CS.createTypeVariable(typeLoc, TVO_CanBindToNoEscape);
CS.addKeyPathConstraint(kpTy, root, rvalueBase, locator);
return kpTy;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSRanking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
++score1;
continue;
}

// FIXME:
// This terrible hack is in place to support equality comparisons of non-
// equatable option types to 'nil'. Until we have a way to constrain a type
Expand Down
Loading