Skip to content

[ConstraintSystem] Inference of inout for closure parameters in generic contexts #5533

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

Closed
wants to merge 2 commits into from
Closed

[ConstraintSystem] Inference of inout for closure parameters in generic contexts #5533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff2a72d
[TypeChecker] Inference of inout for closure parameters in generic co…
ahoppen Oct 31, 2016
63c1f8d
[TypeChecker] Resolve a crash about ambiguous assignment closure
ahoppen Oct 31, 2016
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
5 changes: 4 additions & 1 deletion lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3294,8 +3294,11 @@ namespace {
//
// FIXME: This is also computed when the constraint system is set up.
auto destTy = cs.computeAssignDestType(expr->getDest(), expr->getLoc());
if (!destTy)
if (!destTy) {
cs.TC.diagnose(expr->getLoc(), diag::type_of_expression_is_ambiguous)
.highlight(expr->getSourceRange());
return nullptr;
}
expr->getDest()->propagateLValueAccessKind(AccessKind::Write);

// Convert the source to the simplified destination type.
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1889,7 +1889,7 @@ namespace {

// Otherwise, create a fresh type variable.
Type ty = CS.createTypeVariable(CS.getConstraintLocator(locator),
/*options=*/0);
TVO_CanBeInOut);

param->overwriteType(ty);
}
Expand Down
16 changes: 16 additions & 0 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1410,6 +1410,22 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentTupleConversion:
case ConstraintKind::OperatorArgumentConversion:
if (type1->is<InOutType>() && typeVar2 &&
typeVar2->getImpl().canBeInOut()) {
Copy link
Member

Choose a reason for hiding this comment

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

When type1 is an InOutType, we have two cases for typeVar2: either it allows binding to inout (in which case we do the binding, as it's doing below), or it does not allow binding to inout (in which case we should immediately fail). The immediate-failure case might not be strictly necessary, but it would be nice to early-exit with the failure here rather than go through the entirety of matchTypes.

Also, could this happen the other way, where type2 is an InOutType and typeVar1 needs to be checked for canBeInOut?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's correct. I wasn't quite sure if the inout to UnsafeMutablePointer conversion could still happen but just verified that it can't, so early exit should be possible.

I couldn't come up with any scenario that needs a conversion the other way round but can't disprove it either.

// Upgrade type2 to an InOutType
auto inOutType1 = type1->getAs<InOutType>();

unsigned typeVar3Options = typeVar2->getImpl().getOptions();
typeVar3Options &= ~TVO_CanBeInOut;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add TVO_MustBeMaterializable to typeVar3Options, because one cannot have an inout of a non-materializable type. (I'm not sure I understand why we need the new type variable at all... see below).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I must admit I didn't quite understand what TVO_MustBeMaterializable was for and thought that passing the options on was a sensible thing to do.

auto typeVar3 = createTypeVariable(getConstraintLocator(locator),
typeVar3Options);
auto inOutType3 = InOutType::get(typeVar3);

addConstraint(ConstraintKind::Equal, typeVar2, inOutType3, locator);
return matchTypes(inOutType1->getObjectType(), typeVar3, kind, flags,
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessarily complex, with the introduction of a new type variable that is immediately bound. The effect of this code is, basically, binding typeVar2 to InOutType::get(inOutType1->getObjectType()), i.e., type1. Why can't we directly allow that type binding once we've checked that the type variable can bind to an InOutType?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also thought about directly binding typeVar2 to type1 (which is an InOutType) (that is call setFixedType(typeVar2, type1)). It works on the test cases I have, but I thought that by doing this, we lose the flexibility of the subtype (or lower) constraint that we actually need to enforce. I thought there may be constraint systems that are solvable when typeVar1 is a proper subtype of type2 but type2 still needs to be upgraded to an InOutType. However, I couldn't come up with an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if that were the case this is going to bind the extra typevar anyway in the course of solving the system. I think the new TV Option can be removed in favor of a materializable constraint and a fixed binding.

locator);
}

// We couldn't solve this constraint. If only one of the types is a type
// variable, perhaps we can do something with it below.
if (typeVar1 && typeVar2) {
Expand Down
26 changes: 20 additions & 6 deletions lib/Sema/ConstraintSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,11 @@ namespace constraints {
/// \brief A handle that holds the saved state of a type variable, which
/// can be restored.
class SavedTypeVariableBinding {
/// \brief The type variable and type variable options.
llvm::PointerIntPair<TypeVariableType *, 3> TypeVarAndOptions;
/// \brief The type variable.
TypeVariableType *TypeVar;

/// \brief The type variable options.
unsigned TypeVarOptions : 4;

/// \brief The parent or fixed type.
llvm::PointerUnion<TypeVariableType *, TypeBase *> ParentOrFixed;
Expand All @@ -76,8 +79,8 @@ class SavedTypeVariableBinding {
/// \brief Restore the state of the type variable to the saved state.
void restore();

TypeVariableType *getTypeVariable() { return TypeVarAndOptions.getPointer(); }
unsigned getOptions() { return TypeVarAndOptions.getInt(); }
TypeVariableType *getTypeVariable() { return TypeVar; }
unsigned getOptions() { return TypeVarOptions; }
};

/// \brief A set of saved type variable bindings.
Expand Down Expand Up @@ -126,7 +129,10 @@ enum TypeVariableOptions {
TVO_PrefersSubtypeBinding = 0x02,

/// Whether the variable must be bound to a materializable type.
TVO_MustBeMaterializable = 0x04
TVO_MustBeMaterializable = 0x04,

// Whether the variable can be of an inout type
TVO_CanBeInOut = 0x08,
};

/// \brief The implementation object for a type variable used within the
Expand All @@ -138,7 +144,7 @@ enum TypeVariableOptions {
/// which it is assigned.
class TypeVariableType::Implementation {
/// Type variable options.
unsigned Options : 3;
unsigned Options : 4;

/// \brief The locator that describes where this type variable was generated.
constraints::ConstraintLocator *locator;
Expand Down Expand Up @@ -178,6 +184,14 @@ class TypeVariableType::Implementation {
bool mustBeMaterializable() const {
return Options & TVO_MustBeMaterializable;
}

bool canBeInOut() const {
return Options & TVO_CanBeInOut;
}

unsigned getOptions() const {
return Options;
}

/// \brief Retrieve the type variable associated with this implementation.
TypeVariableType *getTypeVariable() {
Expand Down
4 changes: 3 additions & 1 deletion lib/Sema/TypeCheckConstraints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void TypeVariableType::Implementation::print(llvm::raw_ostream &OS) {
}

SavedTypeVariableBinding::SavedTypeVariableBinding(TypeVariableType *typeVar)
: TypeVarAndOptions(typeVar, typeVar->getImpl().Options),
: TypeVar(typeVar), TypeVarOptions(typeVar->getImpl().Options),
ParentOrFixed(typeVar->getImpl().ParentOrFixed) { }

void SavedTypeVariableBinding::restore() {
Expand Down Expand Up @@ -2683,6 +2683,8 @@ void ConstraintSystem::print(raw_ostream &out) {
tv->getImpl().print(out);
if (tv->getImpl().canBindToLValue())
out << " [lvalue allowed]";
if (tv->getImpl().canBeInOut())
out << " [inout allowed]";
if (tv->getImpl().mustBeMaterializable())
out << " [must be materializable]";
auto rep = getRepresentative(tv);
Expand Down
27 changes: 27 additions & 0 deletions test/expr/closure/inference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,30 @@ var nestedClosuresWithBrokenInference = { f: Int in {} }
// expected-error@-2 {{consecutive statements on a line must be separated by ';'}} {{44-44=;}}
// expected-error@-3 {{expected expression}}
// expected-error@-4 {{use of unresolved identifier 'f'}}

// SR-1976/SR-3073: Inference of inout

func sr1976<T>(_ closure: (inout T) -> Void) {
}

sr1976({ $0 += 2 })

// SR-3073: UnresolvedDotExpr in single expression closure

func sr3073<S, T>(_ closure:(inout S, T) -> ()) {}

sr3073({ $0.number1 = $1 }) //expected-error {{type of expression is ambiguous without more context}}

struct SR3073Lense<Whole, Part> {
let set: (inout Whole, Part) -> ()
}

struct SR3073 {
var number1: Int

func lenses() {
let _: SR3073Lense<SR3073, Int> = SR3073Lense(
set: { $0.number1 = $1 }
)
}
}
5 changes: 5 additions & 0 deletions test/stdlib/UnsafePointerDiagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,8 @@ func unsafeRawBufferPointerConversions(
_ = UnsafeMutableRawBufferPointer(start: orp, count: 1) // expected-error {{cannot convert value of type 'UnsafeRawPointer?' to expected argument type 'UnsafeMutableRawPointer?'}}
_ = UnsafeRawBufferPointer(start: orp, count: 1)
}

func pointerInoutEquality(i: Int, p: UnsafeMutablePointer<Int>?) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of an odd test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept having issues with this kind of expression (found code of this kind in swiftpm or stdlib somewhere) so I thought I might as well add it to the test suite.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but please add an SR- reference so we can more easily look up why the test exists.

var i = i
_ = (p == &i)
}