Skip to content
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.

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

[SR-6659] One line closure introduces overload resolution ambiguity #49208

Closed
djehrlich opened this issue Dec 22, 2017 · 9 comments
Closed

[SR-6659] One line closure introduces overload resolution ambiguity #49208

djehrlich opened this issue Dec 22, 2017 · 9 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation type checker Area → compiler: Semantic analysis

Comments

@djehrlich
Copy link

Previous ID SR-6659
Radar None
Original Reporter @djehrlich
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment

ProductName: Mac OS X
ProductVersion: 10.12.6
BuildVersion: 16G1036
“—“
Xcode 9.2
Build version 9C40b

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, DiagnosticsQoI, TypeChecker
Assignee @xedin
Priority Medium

md5: ec421fe090bf11717ae63d3a4bb9473e

Issue Description:

I extended "Sequence" by adding a "reduce" method whose closure takes an extra argument. The extra argument permits short-circuiting of the loop. However, the new method introduced an ambiguity in existing code using the standard library's "reduce" method, but only when existing usages contain only a single line of code in the closure.

Here is the new short-circuiting "reduce" and sample code that exposes the ambiguity (also contained in attached Playground):

extension Sequence {

{{ public func reduce<T>(}}

{{ into initialResult: T,}}

{{ while transform: (inout T, Element, inout Bool}}) throws -> ()

{{ )}}

{{ rethrows -> T}}

{{ {}}

{{ var retval = initialResult}}

{{ var `continue` = true}}

{{ for element in self {}}

{{ try transform(&retval, element, &`continue`)}}

{{ }}{{ guard `continue`}}

{{ else {}}

{{ break}}

{{ }}}

{{ return retval}}

{{ }}}

{{ }}}

}

let values = [(1,"a"),(2,"b"),(3,"c"),(4,"d"),(5,"e")]

// // Ambiguous

let reducedToDictionary1 = values.reduce(

{{ into: [Int:String]()}}

)

{{{ accumulation, element in}}

{{ accumulation[element.0] = element.1}}

{{}}}

// // Not ambiguous

//let reducedToDictionary2 = values.reduce(

// into: [Int:String]()

//)

//{ accumulation, element in

// accumulation[element.0] = element.1

// return

//}

// // Not ambiguous

//let reducedToDictionary3 = values.reduce(

// into: [Int:String]()

//)

//{ accumulation, element in

// if element.0 < 5 {

// accumulation[element.0] = element.1

// }

//}

2 things surprise me about this

  1. Since neither the standard library's "reduce" method nor this new "reduce" method return values, I'm (somewhat) surprised adding "return" fixes the ambiguity.

  2. Why is the compiler looking at the contents of the closure to resolve ambiguity at all? The closures passed to the two reduce methods take different numbers of arguments.

@belkadan
Copy link
Contributor

belkadan commented Jan 2, 2018

Since closures can use anonymous arguments, sometimes the compiler has to look inside to resolve ambiguities. However, it will only do this for a closure that consists of a single expression, to avoid trying to infer types across statement boundaries.

@xedin, what do you think about using the number of arguments when it is provided, though? At least in Swift 4 and 5 mode, where it can't accidentally be a tuple unsplat?

@xedin
Copy link
Member

xedin commented Jan 2, 2018

The problem here is that non of the overloads on "reduce" actually match for

let reducedToDictionary1 = values.reduce(
    into: [Int:String]()
)
{ accumulation, element in
    accumulation[element.0] = element.1
}

because there is no return (and argument label), which doesn't match "reduce" defined by standard library (it expects Result type returned from the trailing closure), and there is no 3rd argument which would match new overload. So it looks like the first call to reduce is legitimately ambiguous...

@xedin
Copy link
Member

xedin commented Jan 2, 2018

I guess we could have a diagnostic which should stay that expected result is not produced or something like that to improve situation here...

@xedin
Copy link
Member

xedin commented Jan 3, 2018

No actually we can't do that, the best would be to tell that there is an argument missing

@djehrlich
Copy link
Author

I'm confused by your comment that "none of the overloads on "reduce" actually match for..."

When uncommented, "reducedToDictionary2" matches and compiles just fine, utilizing the standard library's

reduce(into:_:)

method by way of adding a second line to the trailing closure. It's an example of overload resolution working properly - with the caveat that adding the second line to the trailing closure is weird and shouldn't be necessary.

I'm also confused by your comment: "which doesn't match "reduce" defined by standard library (it expects Result type returned from the trailing closure)". There are two "reduce" methods in the standard library. The trailing closure argument for the

reduce(_:_:)

variant returns Result. That variant of "reduce" shouldn't come into play here.

In all the examples provided, the

reduce(into:_:)

variant is being called. The trailing closure argument of the latter variant returns ().

@xedin
Copy link
Member

xedin commented Jan 3, 2018

Sorry @djehrlich I missed reduce(into:_) for some reason... It seems like "reducedToDictionary1" is supposed to use the overload from standard library. I will investigate further.

@xedin
Copy link
Member

xedin commented Jan 3, 2018

@djehrlich Can you actually try using the latest trunk snapshot (https://swift.org/download/#releases)? It looks like this problem has been solved on master branch, i think it's related to subtyping with inout parameters (something I've recently fixed).

@djehrlich
Copy link
Author

Pavel,

I pulled down "Trunk Development (master)" from https://swift.org/download/#releases and added it as a toolchain to Xcode. You were correct: the problem is solved on this branch. For posterity, I'm attaching screen shots: ReduceIntoAmbiguity1.png & ReduceIntoAmbiguity2.png.

Thanks!

--David

@xedin
Copy link
Member

xedin commented Jan 3, 2018

Thanks, @djehrlich!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation type checker Area → compiler: Semantic analysis
Projects
None yet
Development

No branches or pull requests

3 participants