[SR-9839] Fixes ambiguity in convention function argument inference #30022

LucianoPAlmeida · 2020-02-23T17:24:45Z

Resolves the ambiguity issue between @\convention function types where the solver finds two possibilities in this case () -> Void and @convention(block) () -> Void but couldn't disambiguate them.

Resolves SR-9839 and SR-9840

cc @xedin @hamishknight

xedin

I'd suggest a different approach - let's detect this situation in matchFunctionTypes and increase score for SK_FunctionConversion because passing () -> Void to @convention(block) () -> Void parameter requires an implicit conversion, so it's always better to pick a type which doesn't require one.

I think this is a better approach because instead of asking ranking algorithm to find where the difference is solver would record exact spot and type of difference.

xedin · 2020-02-24T08:41:30Z

Even better idea might be to join bindings based on representation if it's allowed, so instead of two bindings we'd just have one - @convention(block) () -> Void for the argument. See addPotentialBinding in https://github.com/apple/swift/blob/master/lib/Sema/CSBindings.cpp#L175. That's where such logic would go - https://github.com/apple/swift/blob/master/lib/AST/TypeJoinMeet.cpp#L298

LucianoPAlmeida · 2020-02-24T14:53:20Z

Hey @xedin Thank you for the review :)
Increase the score on was the first approach I've attempted but couldn't quite make it work... I didn't know exactly how to identify the situation where to increase the score or not, so I thought it maybe simpler just adjust on ranking to choose one when comparing solutions ...

Even better idea might be to join bindings based on representation if it's allowed, so instead of two bindings we'd just have one - @convention(block) () -> Void for the argument. See addPotentialBinding in https://github.com/apple/swift/blob/master/lib/Sema/CSBindings.cpp#L175. That's where such logic would go - https://github.com/apple/swift/blob/master/lib/AST/TypeJoinMeet.cpp#L298

Awesome, I'm at work right now, so I'll take a look and change this later tonight :)

xedin · 2020-02-24T17:26:46Z

Sounds good!

xedin · 2020-02-24T17:29:24Z

I think it does make sense to join regular Swift convention to @convention(block) if type variable has to be a supertype of @convention(block) or any other convention of that matter.

LucianoPAlmeida · 2020-02-25T03:15:08Z

@xedin Just a quick question, as I just got back to this ... this TypeJoin::visitFunctionType should be already been called because there is a supertype/subtype correlation between both potential bind types in this case? Or would we add logic specifically for functions checking supertype/subtype relation in addPotentialBindings and attempt Type::join if true? I've just placed a breakpoint into TypeJoin::visitFunctionType and it isn't called in this case ... Am I getting that right?

xedin · 2020-02-25T03:33:03Z

Hm, I guess it's not going to be called because one of the types is subtype and another is a supertype... I guess there is no other choice but to make it work with score in matchFunctionTypes...

LucianoPAlmeida · 2020-02-25T04:21:53Z

Hm, I guess it's not going to be called because one of the types is subtype and another is a supertype... I guess there is no other choice but to make it work with score in matchFunctionTypes...

Humm ... that makes sense, but I'm still not sure how to go about to decide when to increase the scores or not, here are the matchings when solving the system for this case when both attempting type variable $T2 := () -> Void and attempting type variable $T2 := @convention(block) () -> Void

($T2 potentially_incomplete bindings={(supertypes of) () -> Void; (subtypes of) @convention(block) () -> Void})
  Initial bindings: $T2 := () -> Void, $T2 := @convention(block) () -> Void
  (attempting type variable $T2 := () -> Void
(lldb) p func1->dump()
(function_type escaping
  (input=function_params num_params=0)
  (output=type_alias_type decl=Swift.(file).Void underlying='()))
(lldb) p func2->dump()
(function_type representation=block escaping
  (input=function_params num_params=0)
  (output=type_alias_type decl=Swift.(file).Void underlying='()))
(lldb) p kind
(swift::constraints::ConstraintKind) $0 = ArgumentConversion
    (found solution 0 0 0 0 0 0 0 0 0 0 0 0)
  )
  (attempting type variable $T2 := @convention(block) () -> Void
(lldb) p func1->dump()
(function_type escaping
  (input=function_params num_params=0)
  (output=type_alias_type decl=Swift.(file).Void underlying='()))
(lldb) p func2->dump()
(function_type representation=block escaping
  (input=function_params num_params=0)
  (output=type_alias_type decl=Swift.(file).Void underlying='()))
(lldb) p kind
(swift::constraints::ConstraintKind) $1 = ArgumentConversion
(lldb)

So right now, I can't really see how I would increase the score to prefer an type over the other ...

LucianoPAlmeida · 2020-02-25T04:24:41Z

Also, @xedin Thank you for the help here, really appreciate :)

xedin · 2020-02-25T04:39:17Z

That's curious since it doesn't matter what we attempt for $T2 it seems like we just end up with the same comparison, do you know why is that?

LucianoPAlmeida · 2020-02-25T04:44:51Z

That's curious since it doesn't matter what we attempt for $T2 it seems like we just end up with the same comparison, do you know why is that?

No idea, looking into it right now :)

LucianoPAlmeida · 2020-02-25T05:09:12Z

One thing I've noted is that I was expecting to get into the matchFunctionTypes from a Bind constraint, but seems that the bind constraint on TypeVariableBinding::attempt succeeds(without a matchFunctionTypes call) for and it tries to simplify() so then we got into matchingFunctionTypes by argument conversion constraints, which I think are:

Active Constraints:
  @lvalue () -> Void arg conv $T2 [[locator@0x11d01aa88 [Call@/Users/lucianoalmeida/Documents/Programming/swift-lang/swift/test/expr/closure/sr9839.swift:11:8 -> apply argument -> comparing call argument #0 to parameter #0]]];

Inactive Constraints:
  $T2 arg conv @convention(block) () -> Void [[locator@0x11d01ac18 [Call@/Users/lucianoalmeida/Documents/Programming/swift-lang/swift/test/expr/closure/sr9839.swift:11:1 -> apply argument -> comparing call argument #0 to parameter #0]]];

But even for the arg conv it still confusing to me ...

xedin · 2020-02-25T05:46:47Z

Bind would just assign a type for $T2 and argument conversions is when matchFunctionTypes would be used. The more I think about this, the more it seems like a legit ambiguity because either type would require an implicit conversion, either for id or foo...

LucianoPAlmeida · 2020-02-25T15:17:56Z

Seems like this also fixes SR-9840 :)

LucianoPAlmeida · 2020-02-25T15:28:50Z

Bind would just assign a type for $T2 and argument conversions is when matchFunctionTypes would be used. The more I think about this, the more it seems like a legit ambiguity because either type would require an implicit conversion, either for id or foo...

Yeah, the problem is that the two types are both viable solutions(not sure if that's what you mean by "legit ambiguity") ... e.g.

func foo(_ x: @escaping @convention(block) () -> Void) {}
// If we ignore type inference, and the solver choose any of the solutions, they are both applicable
func f( _ a: @escaping () -> Void, _ b: @escaping @convention(block) () -> Void ) {
  foo(a) // Ok
  foo(b) // Ok
}

We can note this here:

Opened types:
  locator@0x11b00e250 [DeclRef@/Users/lucianoalmeida/Documents/Programming/swift-lang/swift/test/expr/closure/sr9839.swift:11:8] opens τ_0_0 -> $T2
  ($T2 potentially_incomplete bindings={(supertypes of) () -> Void; (subtypes of) @convention(block) () -> Void})
  Initial bindings: $T2 := () -> Void, $T2 := @convention(block) () -> Void
  (attempting type variable $T2 := () -> Void
    (found solution 0 0 0 0 0 0 0 0 0 0 0 0)
  )
  (attempting type variable $T2 := @convention(block) () -> Void
    (found solution 0 0 0 0 0 0 0 0 0 0 0 0)
  )

xedin · 2020-02-25T16:45:47Z

Sorry, I mean it's really ambiguous and not just a bug because @convention(block) () -> Void fits when implicitly converted to () -> Void for id and () -> Void fits when implicitly converted to @convention(block) () -> Void for foo, we can't really claim that either implicit conversion is better.

I think we should just make it diagnose ambiguous and be done.

LucianoPAlmeida · 2020-02-25T17:46:24Z

Sorry, I mean it's really ambiguous and not just a bug because @convention(block) () -> Void fits when implicitly converted to () -> Void for id and () -> Void fits when implicitly converted to @convention(block) () -> Void for foo, we can't really claim that either implicit conversion is better.

I think we should just make it diagnose ambiguous and be done.

Humm, I see ... I thought that since both types could be implicit converted to one another what I called equivalent(not sure if that's the term here) it wouldn't really be a problem to just choose one of them :)
But right, I'll just revert the code and this will be just the test cases and we could be close this? :)

xedin · 2020-02-25T18:02:17Z

I think it would have to be more than just test-cases because at least original example from SR is not diagnosed as ambiguity at the moment...

LucianoPAlmeida · 2020-02-25T18:34:46Z

I think it would have to be more than just test-cases because at least original example from SR is not diagnosed as ambiguity at the moment...

It is for SR-9839, ... right now here is the diagnostic

/Users/lucianoalmeida/Documents/Programming/swift-lang/swift/test/expr/closure/closures.swift:497:1: error: unexpected error produced: type of expression is ambiguous without more context
SR9839(id(qux)) 
^

xedin · 2020-02-25T18:38:19Z

Yeah, that's the problem, I think that diagnostic should point out exactly what is ambiguous here :)

LucianoPAlmeida · 2020-02-25T19:23:21Z

Yeah, that's the problem, I think that diagnostic should point out exactly what is ambiguous here :)

Oh right, I'm on it :)

include/swift/AST/DiagnosticsSema.def

LucianoPAlmeida · 2020-02-25T23:06:01Z

@xedin One thing that still got me thinking is the fact that although this is really ambiguous, it may not be too obvious to the user.
Mostly because

func foo(_ x: @escaping @convention(block) () -> Void) {}

func id<T>(_ x: T) -> T {
  return x
}

var qux: () -> Void = {}

foo(qux)
let p = id(qux) // This will be infered as ()->Void
foo(p) // Works fine ... 

foo(id(qux)) // This will error, but for the user, this may not be too obvious and 
             // confusing given that for them, this is just a one-liner shorten form 
             // to write the same of above. So maybe they would expect the same semantics

So my question is if there is a way to prioritize the first bind e.g. ()->Void so the semantics will be the same?
Which are the problems we can get by choosing always the first binding? Or any situation where it would change the semantics of the code.

Anyways it is just a thought, and I want to understand better(especially the downsides) to know if we could improve the situation :)

LucianoPAlmeida · 2020-02-27T02:50:19Z

@xedin I was thinking in being more specific with those diagnostics and provide something like type ()->Void comes from argument conversion id(qux) and type ... comes from ... but I think that in order to do this we would have to track down the source locator for the type variable bindings in the constraint system.

xedin · 2020-02-28T08:00:03Z

I promise to take a look at this on Friday. Sorry, I got distracted by other things this week :/

xedin · 2020-02-28T08:41:48Z

@LucianoPAlmeida We could try one more way to disambiguate this. Similar to my suggestion to use join while inferring bindings, we could alternatively increase the score for @convention(block) binding in TypeVariableBinding::attempt if type variable represents a generic parameter and binding is a "subtype".

The difference between foo(p) and foo(id(qux)) is that there are no type variables involved in first example, all types are known upfront; but in the latter case id is a generic function so its generic parameter T has to be inferred based on constraints associated with it which is going to be two argument conversions:

from () -> Void to T; and (inferred from argument type passed to id)
from T to @convention(block) () -> Void (T is used a result type of id and passed as an argument to parameter of foo)

And there is going to be a solution for each type. That's why I was suggesting either try to join the types (which is unfortunately impossible) or increase a score for one of the cases.

LucianoPAlmeida · 2020-02-28T10:52:13Z

I promise to take a look at this on Friday. Sorry, I got distracted by other things this week :/

No worries @xedin :) These things take time and you have other things 👍
As always, thank you for taking the time to look at this :))

lib/Sema/ConstraintSystem.h

lib/Sema/CSBindings.cpp

LucianoPAlmeida · 2020-04-03T23:55:59Z

@xedin Can you check on this one when you have some time? :) Just to know if this approach is ok and we just have to tweak details or should we go the other way and just diagnose this ambiguous ...

xedin · 2020-04-07T02:46:54Z

Sorry it took me awhile to come back to this, was trying to see whether I could come up with anything else given time... After looking at the code again I still feel like what we are trying to do is a hack and we should try to diagnose it better instead. WDYT?

LucianoPAlmeida · 2020-04-07T13:17:19Z

Sorry it took me awhile to come back to this, was trying to see whether I could come up with anything else given time...

@xedin
No worries! Thanks for taking a look :)

After looking at the code again I still feel like what we are trying to do is a hack and we should try to diagnose it better instead. WDYT?

Agree, I kinda felt the same and also that this situation is too specific to worth making those hacks to only this specific case to work.
But great, let's do this then 👍
Thank you for the help :)

xedin · 2020-04-07T17:48:20Z

No worries, thank you for seeing it through!

LucianoPAlmeida · 2020-04-08T02:31:24Z

Right, diagnostic implemented \o/
cc @xedin

lib/Sema/ConstraintSystem.cpp

include/swift/AST/DiagnosticsSema.def

…with no fixes

lib/Sema/ConstraintSystem.cpp

…neric parameter bindings with no fixes

xedin · 2020-04-08T20:55:48Z

Alright, let's give it a try.

xedin · 2020-04-08T20:55:53Z

@swift-ci please smoke test

xedin

Let's ! Thank you, @LucianoPAlmeida!

LucianoPAlmeida · 2020-04-08T23:58:06Z

\o/ Thanks @xedin!

LucianoPAlmeida requested a review from xedin February 23, 2020 17:24

xedin requested changes Feb 24, 2020

View reviewed changes

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from d19ad1e to 481c58b Compare February 25, 2020 21:25

theblixguy reviewed Feb 25, 2020

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

LucianoPAlmeida requested a review from xedin February 25, 2020 21:37

xedin requested changes Mar 9, 2020

View reviewed changes

lib/Sema/ConstraintSystem.h Outdated Show resolved Hide resolved

lib/Sema/CSBindings.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida requested a review from xedin March 13, 2020 01:04

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch 5 times, most recently from a361989 to 2fd2b86 Compare March 14, 2020 01:06

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from 2fd2b86 to 45796be Compare March 20, 2020 01:16

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from 45796be to 76c163e Compare April 6, 2020 23:06

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from 76c163e to bfda020 Compare April 8, 2020 02:24

xedin requested changes Apr 8, 2020

View reviewed changes

lib/Sema/ConstraintSystem.cpp Outdated Show resolved Hide resolved

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

LucianoPAlmeida added 2 commits April 8, 2020 17:16

[ConstraintSystem] Diagnose ambiguity for generic parameter bindings …

2704b6d

…with no fixes

[test] Adding SR-9839 test cases

4c8f1c8

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from 34c4578 to 4c8f1c8 Compare April 8, 2020 20:17

LucianoPAlmeida requested a review from xedin April 8, 2020 20:19

xedin reviewed Apr 8, 2020

View reviewed changes

lib/Sema/ConstraintSystem.cpp Outdated Show resolved Hide resolved

[ConstraintSystem] Checking for SK_Fix when diagnose ambiguity for ge…

426a2f8

…neric parameter bindings with no fixes

LucianoPAlmeida force-pushed the SR-9839-convention-function-conversions-fail branch from c45009f to 426a2f8 Compare April 8, 2020 20:51

xedin approved these changes Apr 8, 2020

View reviewed changes

xedin merged commit deb58e0 into swiftlang:master Apr 8, 2020

LucianoPAlmeida deleted the SR-9839-convention-function-conversions-fail branch April 8, 2020 23:44

LucianoPAlmeida mentioned this pull request Feb 23, 2020

[SR-9839] @convention function conversions unreliably fail when type variables are introduced #52253

Closed

[SR-9839] Fixes ambiguity in convention function argument inference #30022

[SR-9839] Fixes ambiguity in convention function argument inference #30022

Uh oh!

Conversation

LucianoPAlmeida commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Feb 24, 2020

Uh oh!

LucianoPAlmeida commented Feb 24, 2020

Uh oh!

xedin commented Feb 24, 2020

Uh oh!

xedin commented Feb 24, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

xedin commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

Uh oh!

LucianoPAlmeida commented Feb 25, 2020

Uh oh!

LucianoPAlmeida commented Feb 27, 2020

Uh oh!

xedin commented Feb 28, 2020

Uh oh!

xedin commented Feb 28, 2020

Uh oh!

LucianoPAlmeida commented Feb 28, 2020

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Apr 3, 2020

Uh oh!

xedin commented Apr 7, 2020

Uh oh!

LucianoPAlmeida commented Apr 7, 2020

Uh oh!

xedin commented Apr 7, 2020

Uh oh!

LucianoPAlmeida commented Apr 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xedin commented Apr 8, 2020

Uh oh!

xedin commented Apr 8, 2020

LucianoPAlmeida commented Feb 23, 2020 •

edited

Loading

LucianoPAlmeida commented Feb 25, 2020 •

edited

Loading