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

ahoppen · 2016-10-30T15:56:21Z

Problem

In generic contexts, the inout annotation for closure parameters was not inferred anymore, e.g. the following code failed to compile (SR-1976):

func foo<T>(_ closure: (inout T) -> Void) {}
foo({ $0 += 2 }) // error: ambigous use of operator '+='

while the non-generic version

func foo(_ closure: (inout Int) -> Void) {}
foo({ $0 += 2 })

compiles fine.

If the closure included a member access of the inout argument, this even a compiler crash (SR-3053/SR-3073):

struct MyType {
  var number1: Int
}
func set<S, T>(_ closure:(inout S, T) -> ()) {}
set({ $0.number1 = $1 })

Solution

When encountering a subtype constraint (type1 subtype type2) where type1 is an inout type and type2 is not, we introduce a new type variable type3 and check if type2 == inout type3 can hold. This guards against cases like the following where we need the asymetric inout relationship.

func pointerInoutEquality(i: Int, p: UnsafeMutablePointer<Int>?) {
  var i = i
  _ = (p == &i) // <- This is the interesting line
}

Otherwise, we replace the original constraint by the two constraints type2 == inout type3 and removedInOutLayerFromType1 subtype type3 that upgrade type2 to inout.

Some notes

I don't actually feel that CSSimplify is the right place to make the choice if a type needs to be inout or not, but couldn't find a way to do it in CSSolve. Using a disjunction constraint would require having a conjunction constraint as well but I couldn't find such a thing.
Removing the type variable in the end feels fairly ugly but otherwise we get a false warning about not unwrapping UnsafeMutablePointer in the example above.
For backtracking changes to the constraint system, it seems like there used to be ConstraintGraphScope but that didn't work for me and doesn't seem to be used anymore -> we might remove the class

ahoppen · 2016-10-30T19:11:11Z

@swift-ci please test

DougGregor · 2016-10-31T03:59:01Z

lib/Sema/CSSimplify.cpp

@@ -1410,6 +1410,35 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
    case ConstraintKind::ArgumentConversion:
    case ConstraintKind::OperatorArgumentTupleConversion:
    case ConstraintKind::OperatorArgumentConversion:
+      if (type1->is<InOutType>() && !type2->is<InOutType>() && typeVar2) {


typeVar2 will only be non-null when type2 is a type variable, so the !type2->is<InOutType>() check here is redundant.

Yeah, that's true. Good spot

DougGregor · 2016-10-31T04:03:09Z

lib/Sema/CSSimplify.cpp

+
+        TypeMatchOptions subFlags = flags;
+        subFlags -= TMF_GenerateConstraints;
+        // First, see if we can actually replace typeVar2 with an InOutType.


matchTypes() isn't permitted to tentatively do testing this way... when it fails, that branch of the solution space is invalid. In cases where it isn't obvious which way to go, we end up having to introduce a disjunction so the solver itself can explore both paths (and use the scope RAII objects to allow some of them to fail). I wonder if you could tackle this by adding a TVO_CanBeInOut flag to specific type variables, and only allow type variables with that flag set to bind to inout types, similarly to the way we have the TVO_MustBeMaterializable flag.

OK, that was the part I wasn't quite sure about in my approach, looking at the implementation of matchTypes I couldn't see a reason for the CS to get changed if you remove TMF_GenerateConstraints, but you know the part far better than me ;-). I'll have a look at adding a new flag to type variable then

DougGregor · 2016-10-31T04:03:51Z

test/stdlib/UnsafePointerDiagnostics.swift

@@ -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>?) {


This is a bit of an odd test.

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.

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

…ntexts This fixes SR-1976

Having an invalid assignment in a closure could make the type checker fail without providing a diagnostic, eventually crashing the compiler. Now a diagnostic is provided. This resolves SR-3053/SR-3073

ahoppen · 2016-11-01T09:46:37Z

Thanks for the feedback. I now annotate closure parameters with TVO_CanBeInOut and only upgrade a type variable to inout if it has this flag set. This really did make things clearer.

Regarding the UnsafeMutablePointer test case: Should I remove it or keep it in the test suite?

@swift-ci please test

DougGregor

Definitely getting closer and simpler!

DougGregor · 2016-11-02T03:13:36Z

lib/Sema/CSSimplify.cpp

@@ -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()) {


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?

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.

DougGregor · 2016-11-02T03:14:39Z

lib/Sema/CSSimplify.cpp

+        auto inOutType1 = type1->getAs<InOutType>();
+
+        unsigned typeVar3Options = typeVar2->getImpl().getOptions();
+        typeVar3Options &= ~TVO_CanBeInOut;


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).

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.

DougGregor · 2016-11-02T03:17:46Z

lib/Sema/CSSimplify.cpp

+        auto inOutType3 = InOutType::get(typeVar3);
+
+        addConstraint(ConstraintKind::Equal, typeVar2, inOutType3, locator);
+        return matchTypes(inOutType1->getObjectType(), typeVar3, kind, flags,


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?

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.

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.

DougGregor · 2016-11-02T03:18:12Z

test/stdlib/UnsafePointerDiagnostics.swift

@@ -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>?) {


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

slavapestov · 2016-12-08T05:48:16Z

Looks like this was never merged?

ahoppen · 2016-12-08T07:36:28Z

No, it wasn't. Uni work caught up with me last month so I couldn't work on it anymore. I am currently working on #5537 and was hoping to finish this PR sometime next week. Or is the issue very urgent at the moment?

slavapestov · 2016-12-09T02:20:40Z

@ahoppen Not urgent, just making sure we're not dropping user contributions on the floor!

slavapestov · 2017-01-06T22:36:16Z

FWIW @xedin is looking at similar issues.

xedin · 2017-01-06T23:01:37Z

Heh, I wish I've seen this earlier!... Thanks, @slavapestov! This re-enforces my hepothesys that binding instead of argv between type of different materializability is a way to go about it, since potential bindings should be able to create disjunction with and without inout for related bind param, I will try it out tonight and let you guys know.

slavapestov · 2017-01-09T08:52:47Z

@xedin Does your PR supersede this one?

xedin · 2017-01-09T08:56:26Z

@slavapestov yes, it does, mine also covers tests from this one.

[wasm] Revert unnecessary TaskGroup.cpp include directive

DougGregor requested changes Oct 31, 2016

View reviewed changes

ahoppen force-pushed the SR-1976-inout-inference branch from 7c4448b to 8f39c78 Compare November 1, 2016 09:19

ahoppen added 2 commits November 1, 2016 09:29

[TypeChecker] Inference of inout for closure parameters in generic co…

ff2a72d

…ntexts This fixes SR-1976

[TypeChecker] Resolve a crash about ambiguous assignment closure

63c1f8d

Having an invalid assignment in a closure could make the type checker fail without providing a diagnostic, eventually crashing the compiler. Now a diagnostic is provided. This resolves SR-3053/SR-3073

ahoppen force-pushed the SR-1976-inout-inference branch from 8f39c78 to 63c1f8d Compare November 1, 2016 09:37

DougGregor reviewed Nov 2, 2016

View reviewed changes

slavapestov closed this Jan 11, 2017

ahoppen deleted the SR-1976-inout-inference branch June 26, 2017 17:36

MaxDesiatov pushed a commit that referenced this pull request Sep 7, 2023

Merge pull request #5533 from swiftwasm/katei/revert-task-group-cpp-diff

b952d11

[wasm] Revert unnecessary TaskGroup.cpp include directive

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

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

Uh oh!

Conversation

ahoppen commented Oct 30, 2016

Problem

Solution

Some notes

Uh oh!

ahoppen commented Oct 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Nov 1, 2016

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Dec 8, 2016

Uh oh!

ahoppen commented Dec 8, 2016

Uh oh!

slavapestov commented Dec 9, 2016

Uh oh!

slavapestov commented Jan 6, 2017

Uh oh!

xedin commented Jan 6, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

Uh oh!