Skip to content

Small constraint solver cleanups #18970

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 3 commits into from
Aug 25, 2018
Merged

Small constraint solver cleanups #18970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b41a7a
Sema: Simplify ExprRewriter::coerceTupleToTuple()
slavapestov Aug 23, 2018
1c5452d
Sema: Change a few AnyFunctionType => FunctionType
slavapestov Aug 23, 2018
4ed51fb
Sema: Add a FIXME
slavapestov Aug 23, 2018
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
93 changes: 8 additions & 85 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5104,46 +5104,18 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
auto *fromTupleExpr = dyn_cast<TupleExpr>(innerExpr);

/// Check each of the tuple elements in the destination.
bool hasVariadic = false;
unsigned variadicParamIdx = toTuple->getNumElements();
bool anythingShuffled = false;
bool hasInits = false;
SmallVector<TupleTypeElt, 4> toSugarFields;
SmallVector<TupleTypeElt, 4> fromTupleExprFields(
fromTuple->getElements().size());
SmallVector<Expr *, 2> callerDefaultArgs;
ConcreteDeclRef callee =
findCalleeDeclRef(cs, solution, cs.getConstraintLocator(locator));

for (unsigned i = 0, n = toTuple->getNumElements(); i != n; ++i) {
assert(sources[i] != TupleShuffleExpr::DefaultInitialize &&
sources[i] != TupleShuffleExpr::Variadic);

const auto &toElt = toTuple->getElement(i);
auto toEltType = toElt.getType();

// If we're default-initializing this member, there's nothing to do.
if (sources[i] == TupleShuffleExpr::DefaultInitialize) {
anythingShuffled = true;
hasInits = true;
toSugarFields.push_back(toElt);

// Create a caller-side default argument, if we need one.
if (auto defArg = getCallerDefaultArg(cs, dc, expr->getLoc(),
callee, i).first) {
callerDefaultArgs.push_back(defArg);
sources[i] = TupleShuffleExpr::CallerDefaultInitialize;
}
continue;
}

// If this is the variadic argument, note it.
if (sources[i] == TupleShuffleExpr::Variadic) {
assert(!hasVariadic && "two variadic parameters?");
toSugarFields.push_back(toElt);
hasVariadic = true;
variadicParamIdx = i;
anythingShuffled = true;
continue;
}

// If the source and destination index are different, we'll be shuffling.
if ((unsigned)sources[i] != i) {
anythingShuffled = true;
Expand Down Expand Up @@ -5200,51 +5172,6 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
fromElt.getWithType(cs.getType(convertedElt));
}

// Convert all of the variadic arguments to the destination type.
ArraySliceType *arrayType = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own understanding what to double-check - this is dead-code now because parameters in the function type is represented as an array of Param objects and regular tuples can't have variadics, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was dead even before that. This code path is never taken for function arguments. It is only used when we convert a value of tuple type to another tuple type. Applying the solution to call arguments happens in coerceCallArguments() (which uses Params now, but even before that, it was separate because the rules for call arguments are different from tuple values).

What makes this confusing is that both call arguments and tuple conversions use a TupleShuffleExpr, which supports the union of the transformations needed in both cases. Once all this redundant code is gone it will be easier to split up TupleShuffleExpr into two separate Exprs with very little overlap between them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, at the end of all of this, “variadic tuples” and tuples with inouts will no longer be a thing, but it will take us a while to get there. I’ve been mostly focusing on places where we build the old representation; there are more spots where we take it apart, by calling getInput().

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thank you!

if (hasVariadic) {
Type toEltType = toTuple->getElements()[variadicParamIdx].getVarargBaseTy();
for (int fromFieldIdx : variadicArgs) {
const auto &fromElt = fromTuple->getElement(fromFieldIdx);
Type fromEltType = fromElt.getType();

// If the source and destination types match, there's nothing to do.
if (toEltType->isEqual(fromEltType)) {
fromTupleExprFields[fromFieldIdx] = fromElt;
continue;
}

// We need to convert the source element to the destination type.
if (!fromTupleExpr) {
// FIXME: Lame! We can't express this in the AST.
tc.diagnose(expr->getLoc(),
diag::tuple_conversion_not_expressible,
fromTuple, toTuple);
return nullptr;
}

// Actually convert the source element.
auto convertedElt = coerceToType(
fromTupleExpr->getElement(fromFieldIdx),
toEltType,
locator.withPathElement(
LocatorPathElt::getTupleElement(fromFieldIdx)));
if (!convertedElt)
return nullptr;

fromTupleExpr->setElement(fromFieldIdx, convertedElt);

fromTupleExprFields[fromFieldIdx] =
fromElt.getWithType(cs.getType(convertedElt));
}

// Find the appropriate injection function.
if (tc.requireArrayLiteralIntrinsics(expr->getStartLoc()))
return nullptr;
arrayType = cast<ArraySliceType>(
toTuple->getElements()[variadicParamIdx].getType().getPointer());
}

// Compute the updated 'from' tuple type, since we may have
// performed some conversions in place.
Type fromTupleType = TupleType::get(fromTupleExprFields, tc.Context);
Expand All @@ -5256,8 +5183,7 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
}

// Compute the re-sugared tuple type.
Type toSugarType = hasInits? toTuple
: TupleType::get(toSugarFields, tc.Context);
Type toSugarType = TupleType::get(toSugarFields, tc.Context);

// If we don't have to shuffle anything, we're done.
if (!anythingShuffled && fromTupleExpr) {
Expand All @@ -5272,13 +5198,10 @@ Expr *ExprRewriter::coerceTupleToTuple(Expr *expr, TupleType *fromTuple,
// Create the tuple shuffle.
return
cs.cacheType(TupleShuffleExpr::create(tc.Context,
expr, sources,
TupleShuffleExpr::TupleToTuple,
callee,
variadicArgs,
arrayType,
callerDefaultArgs,
toSugarType));
expr, sources,
TupleShuffleExpr::TupleToTuple,
ConcreteDeclRef(), {}, Type(), {},
toSugarType));
}

static Type getMetatypeSuperclass(Type t, TypeChecker &tc) {
Expand Down
10 changes: 7 additions & 3 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,10 @@ matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
paramIdx));
auto argTy = args[argIdx].getType();

// FIXME: This should be revisited. If one of argTy or paramTy
// is a type variable, matchTypes() will add a constraint, and
// when the constraint is later solved, we will have lost the
// value of 'subflags'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we really need to fix this at some point...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be best to just try removing this sub flag altogether, instead of recovering it from the locator or whatever.

if (!haveOneNonUserConversion) {
subflags |= ConstraintSystem::TMF_ApplyingOperatorParameter;
}
Expand Down Expand Up @@ -1403,7 +1407,7 @@ ConstraintSystem::matchExistentialTypes(Type type1, Type type2,

// Conformance to 'Any' always holds.
if (type2->isAny()) {
auto *fnTy = type1->getAs<AnyFunctionType>();
auto *fnTy = type1->getAs<FunctionType>();
if (!fnTy || !fnTy->isNoEscape())
return getTypeMatchSuccess();

Expand Down Expand Up @@ -1602,7 +1606,7 @@ ConstraintSystem::matchTypesBindTypeVar(
// Disallow bindings of noescape functions to type variables that
// represent an opened archetype. If we allowed this it would allow
// the noescape function to potentially escape.
if (auto *fnTy = type->getAs<AnyFunctionType>()) {
if (auto *fnTy = type->getAs<FunctionType>()) {
if (fnTy->isNoEscape() && typeVar->getImpl().getArchetype()) {
if (shouldAttemptFixes()) {
auto *fix = MarkExplicitlyEscaping::create(
Expand Down Expand Up @@ -2136,7 +2140,7 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
// Penalize conversions to Any, and disallow conversions of
// noescape functions to Any.
if (kind >= ConstraintKind::Conversion && type2->isAny()) {
if (auto *fnTy = type1->getAs<AnyFunctionType>()) {
if (auto *fnTy = type1->getAs<FunctionType>()) {
if (fnTy->isNoEscape()) {
if (shouldAttemptFixes()) {
auto &ctx = getASTContext();
Expand Down