Generic function taking closure with inout parameter returns erroneous results #6586

xedin · 2017-01-05T09:57:09Z

Resolves SR-3520.
Resolves SR-1976.
Resolves SR-3073.
Resolves SR-3479.

xedin · 2017-01-05T10:00:51Z

This is still work in process, but solves crasher related to the ticket and improves diagnostics related to result types of closures with anonymous parameters...

I'm going to check why doesn't following type-check correctly right now (I'm pretty sure it has to do with how constraint graph contraction works related to parameter binding):

func sr3520_2<T>(_ item: T, _ update: (inout T) -> Void) {
  var x = item
  update(&x)
}
var i = 42
sr3520_2(i) { $0 += 3 }

xedin · 2017-01-06T20:06:05Z

I've figured out what the problem is last night but i don't know yet how to solve it properly - the reason why it's failing to type-check is because update: parameter is (inout <tv_0>) (where <tv_0> must be materializable) and anonymous closure is going to allocate argument as just (<tv_1>) (which might be inout or might not be depending on the solver path) and according to parameter simplification rules following constraint is going to be created to relating parameter and argument (inout <tv_0>) arg conv (tv_1>) (keep in mind that tv_0 must be materializable because it's open generic) so now CG.optimize() is going to kick-in and merge equivalence classes of tv_0 and tv_1 which yields constraint system unsolvable when tv_1 actually has to be inout, because tv_0 must be materializable, but works in other cases e.g.

func f0<T, U>(_ t: T, _ f: (inout T) -> U) -> U {
  var t2 = t
  return f(&t2)
}

struct X2 {
  func g() -> Float { return 0 }
}

_ = f0(X2(), {$0.g()})

since first argument of closure doesn't have to be inout in this case, everything works perfectly fine. There is also rule in CG.optimize which prohibits such contraction if there is BindParam that binds to strict inout is involved but that's never true in either case.

I've tried to check materializability requirements on both sides and skip contraction if they are different but that breaks a bunch of other cases...

Edit: I've incorrectly stated that constraint kind is 'arg conv' where it's actually 'subtype', because closure parameter bindings are constructed in context of function call, which means them subtype constraints, see ConstraintSystem::matchFunctionTypes.

slavapestov · 2017-01-06T21:10:25Z

@xedin We were discussing this problem yesterday because it also came up with #6608. @DougGregor and I had the idea that there should be one type variable for the internal type of each parameter (which is an lvalue type for inouts, an array type for varargs, etc) -- and another type variable for the external type of the parameter list (which need not be materializable). A constraint binds the outer type variable with a tuple type of inner type variables, enforcing the necessary conversions. What do you think?

xedin · 2017-01-06T21:24:53Z

@slavapestov That's what I was kind of thinking as well (but less general)...

I thought that maybe if one side of the contractable constraint must be materializable but the other isn't instead of contracting constraint it would make more sense to create a separate binding which is going to reverse the relationship e.g. -> (inout t0) arg conv (t1) becomes t1 bind (inout t0) this way instead of making type variables equivalent (which I think is wrong) it would establish other type of dependency relationship which is more appropriate.

All that said, I'm not yet sure if that's going to work at all, since I haven't tried it but in theory it should enable t1 to be bound properly and doesn't prevent related 'bind param' constraints from being contracted...

jtbandes · 2017-01-06T21:28:16Z

Perhaps @cwillmor would have thoughts on this; although no longer a maintainer I think he was the original author of BindParam constraint stuff...

…t return types

xedin · 2017-01-08T06:43:04Z

@slavapestov @DougGregor I've pushed a fix which deals with inout T0 subtype T1 constraint contraction by ConstraintGraph.optimize which breaks system solving because it also contracts bind param bindings which are not supposed to be inout but rather lvalue. The change from subtype to fixed binding made early allows for correct 'bind param' contraction/solving because left-hand side of the constraint is going to be currently marked as 'inout' which marks right-hand side as 'lvalue'.

Please take a look and let me know what you think, I've added comments and tests for all related issues SR-3520, SR-1976, SR-3073, SR-3479.

slavapestov · 2017-01-09T05:08:31Z

Awesome. So the vararg problem is not related after all?

slavapestov · 2017-01-09T05:08:39Z

@swift-ci Please smoke test

xedin · 2017-01-09T05:56:49Z

Yeah, vararg is unrelated, it has to do with contraction of subtype constraints, generated by sub-declarations of trailing closures to be precise :)

slavapestov · 2017-01-09T07:59:25Z

lib/Sema/CSSimplify.cpp

+      // (if inout containts type var, see ConstraintGraph::contractEdges()),
+      // since right-hand side type variable must not be materializable
+      // it can simply get left-hand side as a fixed binding, otherwise fail.
+      if (type1->is<InOutType>() && type1->hasTypeVariable() && typeVar2) {


Maybe this should be type1->getInOutObjectType()->isTypeVariableOrMember()?

Sure, let me change it to check for type variable directly instead of presence of one.

Convert subtype constraints with inout on one side and type variable on the other into fixed binding. Constriants like `inout T0 subtype T1` where (T0 must be materializable) are created when closures are part of the generic function parameters e.g. `func foo<T>(_ t: T, (inout T) -> Void) {}` so when such function gets called e.g. ``` var x = 42 foo(x) { $0 = 0 } ``` it's going to try and map closure parameters type (inout T0) - where is opened generic parameter T - to argument type (T1), which can be 'inout' but it's uncertain at this stage, but since closure 'declaration' `{ $0 = 0 }` is wrapped inside of a function call, it has to 'map' parameters to arguments instead of converting them, see `ConstraintSystem::matchFunctionTypes`. Resolves: SR-3520, SR-1976, SR-3073, SR-3479.

xedin · 2017-01-09T08:24:12Z

@slavapestov Force pushed the change for type variable checking, thanks!

slavapestov · 2017-01-09T08:24:49Z

@swift-ci Please smoke test

slavapestov · 2017-01-09T08:26:11Z

@swift-ci Please smoke test Linux

slavapestov · 2017-01-09T09:12:35Z

@swift-ci Please smoke test Linux

xedin · 2017-01-09T09:40:25Z

@slavapestov previous Linux build is still going, just got to run tests, I took a long time to compile llvm/clang and other pieces...

slavapestov · 2017-01-09T09:46:21Z

@swift-ci Please smoke test Linux

xedin · 2017-01-09T09:56:41Z

\o/

xedin · 2017-01-09T20:28:58Z

@slavapestov Thanks again!

xedin · 2017-01-09T20:40:28Z

@slavapestov Will you resolve the tickets or should I?

rudkx · 2017-01-09T21:28:22Z

@xedin I went ahead and resolved them, thanks!

xedin · 2017-01-09T21:35:00Z

@rudkx Awesome, thank you!

timvermeulen · 2017-01-24T19:47:32Z

This didn't make Swift 3.1?

[Diagnostics] Improve diagnostics for closures with erroneous implici…

dd7360e

…t return types

xedin force-pushed the SR_3520 branch from b177c35 to a5d8a60 Compare January 8, 2017 06:29

xedin changed the title ~~[WIP] SR-3520: Generic function taking closure with inout parameter returns erroneous results~~ Generic function taking closure with inout parameter returns erroneous results Jan 8, 2017

slavapestov reviewed Jan 9, 2017

View reviewed changes

slavapestov self-assigned this Jan 9, 2017

xedin force-pushed the SR_3520 branch from a5d8a60 to 0f716e6 Compare January 9, 2017 08:23

slavapestov merged commit c04c6c9 into swiftlang:master Jan 9, 2017

Generic function taking closure with inout parameter returns erroneous results #6586

Generic function taking closure with inout parameter returns erroneous results #6586

Uh oh!

Conversation

xedin commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Jan 5, 2017

Uh oh!

xedin commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Jan 6, 2017

Uh oh!

xedin commented Jan 6, 2017

Uh oh!

jtbandes commented Jan 6, 2017

Uh oh!

xedin commented Jan 8, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

slavapestov Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

xedin Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

slavapestov commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

rudkx commented Jan 9, 2017

Uh oh!

xedin commented Jan 9, 2017

Uh oh!

timvermeulen commented Jan 24, 2017

Uh oh!

Uh oh!

xedin commented Jan 5, 2017 •

edited

Loading

xedin commented Jan 6, 2017 •

edited

Loading