[AST] Paren'd reference to an IUO function crashes the compiler in SILGen

[theblixguy]

A paren'd reference to an IUO function crashes the compiler in SILGenApply::visitImplicitlyUnwrappedFunctionConversionExpr():

struct S {
  func foo() -> Int! { return 0 }
}

let s = S()
let x: Int = (s.foo)()

To resolve this, only generate a single constraint to bind it to an optional type, instead of creating a disjunction of optional and underlying types.

Resolves SR-10492
Resolves rdar://problem/50169820

[xedin]

From -debug-constraints print out I can see that the problem here might be related to CSApply inserting ImplicitlyUnwrappedFunctionConversionExpr when it shouldn't. It seems like a better fix would be to adjust CSApply rather then strip sugar like that from AST.

[jrose-apple]

Arguably this is incorrect behavior; the parens imply that the expression should be evaluated separately, which means it would lose its IUO-ness.

[xedin]

@jrose-apple I guess we agree on this point, it shouldn't result in ImplicitlyUnwrappedFunctionConversionExpr the way it currently does. Do I understand your comment correctly?

[jrose-apple]

Sorry, that was mostly aimed at @theblixguy; I suggest that the example in the PR description should error out rather than compile successfully.

[xedin]

I agree, it might unfortunately have to happen in CSApply, I'm not sure if we can make solver not ignore parens in this case without breaking something else...

[theblixguy]

Okay! @xedin in finishApply(), if we encounter that expression wrapped in a parens then we throw an error?

[xedin]

Yeah, and return nullptr; but give it a try to rejct that case in the solver first, maybe it's going to work out.

[jrose-apple]

No, it's not an error; it's a decay.

let x: Int = (s.foo)() // error
let x: Int? = (s.foo)() // okay

[theblixguy]

No, it's not an error; it's a decay.

let x: Int = (s.foo)() // error
let x: Int? = (s.foo)() // okay

Hmm, in that case maybe matchTypes would be better?

[theblixguy]

Hmm, interestingly the solver is now generating two solutions for the call that are exactly the same, minus the fix and obviously its choosing the one without the fix. Is there something I am missing or should I move the check elsewhere? @xedin

What if we change CSGen so we can make the ApplyExpr's result type be optional if the IUO func is wrapped in parens?

EDIT: I tried it and it worked, but again maybe there is a better place for this check. I also tried a few ways to generate constraints for the function's return type, but it didn't work the way I expected it to be. One place I haven't tried is matchTypesBindTypeVar()... we could create a special locator for the parens and then detect that to create a fix, but that might be too much work.

[xedin]

LGTM!

[xedin]

@swift-ci please smoke test

[xedin]

@swift-ci please test source compatibility

[hamishknight]

Arguably the more general problem here is that we can we can still treat function references wrapped in parens as being applied references, as opposed to unapplied references that are then later applied. Another example of this is with argument labels:

func foo(x: Int) {}
(foo)(x: 5)

Arguably the user shouldn't be allowed to use the argument label x: in the call here, they should have to say (foo)(5) instead – just like if they had said { foo }()(5).

If we started classifying the reference to foo in the above example as an unapplied reference, then I believe we'd also get the desired IUO behaviour. This would technically be source breaking, but I doubt many people are writing code like this.

[jrose-apple]

I agree with all of the above except the "I doubt many people are writing code like this" is scary.

[xedin]

Yeah, I agree with that in principle but it seems like impact might be underestimated...

[theblixguy]

@swift-ci please smoke test

[theblixguy]

Hmm... still can't invoke CI 🤷‍♂ cc @shahmishal

[shahmishal]

@theblixguy Can you please try again?

[theblixguy]

@swift-ci please smoke test

[theblixguy]

Thanks @shahmishal, it seems to be working now 🎉

[theblixguy]

Hmm, shouldForceUnwrapResult() hits an assert in getDisjunctionChoice() because it doesn't have a disjunction choice when we're assigning the result to a variable of optional type. Probably best to return false in shouldForceUnwrapResult() when we don't have a disjunction choice.

[hamishknight]

@jrose-apple Fair enough, I could have worded that better :) I didn't mean to completely dismiss source compatibility concerns, they are certainly there. Anecdotally, I don't think I've ever seen Swift code out in the wild that relies on this behaviour (from ~3 years of answering Stack Overflow questions), but that's probably not a representative sample. But given the current behaviour isn't causing active harm, I'm also fine with it staying as-is.

Also, investigating further, if the proposed change included default parameter values, the source compatibility impact would be greater as there's currently a slightly broader set of places where they can be used:

struct S {
  init() throws {}
  func foo(x: Int = 0) {}
}

let s = try S()
(s.foo)()            // ✅
(s.foo)(x: 5)        // ✅

(try S().foo)()      // ✅
(try S().foo)(x: 5)  // ❌

(try! S().foo)()     // ❌
(try! S().foo)(x: 5) // ❌

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test

[theblixguy]

Hmm there's one test failure on macOS:

/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/Interpreter/NewtypeLeak.swift:27:20: error: value of optional type 'ObjCLifetimeTracked?' must be unwrapped to a value of type 'ObjCLifetimeTracked'

I don't know what a "newtype" is. Looking at the definition for NSMyObject, the init seems to return an IUO. I think the current check is getting confused when looking at MyObject(ObjCLifetimeTracked()) because ObjCLifetimeTracked() is wrapped in parens? Let me investigate.

Reproduced:

class Foo {
  init!() {}
}

class Bar {
  init(_ foo: Foo) {}
}

let bar = Bar(Foo())

[theblixguy]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test macOS platform

[theblixguy]

I don’t know why those two sourcekit tests are failing on macOS:

error response (Connection Interrupted): Connection interrupted
sourcekit: [1:connection-event-handler:9475: 0.0000] Connection interruptsourcekit: [1:updateSemanticEditorDelay:9475: 0.0006] disabling semantic editor for 10 secondssourcekit: [1:pingService:9475: 0.0006] pinging serviceerror response (Connection Interrupted): Connection interrupted
FileCheck error: '-' is empty.

I’ll trigger CI again in a few hours.

[theblixguy]

@swift-ci please clean smoke test macOS platform

[theblixguy]

Alright, let's see if the problem is fixed now (apparently the anchor was null when sourcekit was running those tests, so the cast to CallExpr was causing sourcekit to crash).

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test Linux platform

[theblixguy]

@swift-ci please test source compatibility

[hamishknight]

Sorry, I've just thought of a few cases that aren't quite correctly handled :/

[hamishknight]

What about SubscriptExpr and KeyPathExpr anchors? I believe we'll incorrectly reject the following examples:

struct S {
  subscript() -> Int! { 0 }
}

func takesKP(_ kp: KeyPath<S, Int>) {}
takesKP(\S.[])

func takesInt(_ x: Int) {}
takesInt(S()[])

Adding specific exceptions for these cases feels pretty brittle, perhaps we should follow a similar pattern to markDirectCallee where only callees that have defined setFunctionRefKind and getFunctionRefKind are considered?

Though IMO that's still not an ideal solution – in the long run, if we don't make the source breaking change of having markDirectCallee not look through parentheses, we may want to turn FunctionRefKind into a class and store an additional flag on it to answer the question "does this reference decay to a function value such that it doesn't produce an IUO?".

[xedin]

I think these cases seem to be a different issue. None of the locators, associated with member overload set, in these examples are going to be anchored in ParenExpr. That said, I think we indeed need to come up with a better solution than what we currently have, creating new disjunctions in multiple distinct places is not robust enough.

[hamishknight]

@xedin Just to clarify – these are cases that compiled before this change, and no longer compile after. The parent ParenExpr comes from their use as arguments in a function call, so we incorrectly consider them to be "IUO calls wrapped in parens".

[xedin]

Ah, right because it's checking for parent to be a ParentExpr, for some reason I thought that it's checking for anchor to be a paren. Looks like we need to revert and find a better alternative.

[theblixguy]

You're right, those cases do not compile:

1. cannot convert value of type 'KeyPath<S, Int?>' to expected argument type 'KeyPath<S, Int>'
2. value of optional type 'Int?' must be unwrapped to a value of type 'Int'

[theblixguy]

So, perhaps the check could be:

1. Is this a call, keypath or subscript expression? If yes, return false.
2. Is the fn and semanticFn equal? If yes, return false.
3. Return true

Do you find anything wrong with this logic?

[hamishknight]

@theblixguy One potential problem with comparing getFn() with getSemanticFn() is that they won't be equal for cases like this:

func foo() -> Int! { 0 }
let x: Int = foo.self()

which (I think?) we want to allow. In addition, they will be equal for cases like this:

import Foundation

class C {
  @objc func foo() -> String! { "" }
}

func bar(_ x: AnyObject) {
  let str: String = (x.foo)!()
}

which we want to reject. I think what we'll want to do is check whether there is a ParenExpr between the call's getFn() and its "direct callee" (as determined by markDirectCallee).

If we turned FunctionRefKind into a class that stores an additional flag for "does this reference decay to a function value such that it doesn't produce an IUO?", I believe we could do this logic directly in markDirectCallee by setting the flag if we had to look through a paren.

[theblixguy]

Okay, I will give that a try tomorrow 👍

[xedin]

Sorry it took me a bit to get back to this today, just wanted to mention that I concur with what @hamishknight is proposing.

[jrose-apple]

func foo() -> Int! { 0 }
let x: Int = foo.self()

I'd reject this one myself, but also probably zero people are relying it, even more than the parentheses.

[hamishknight]

What if the paren isn't an immediate parent? For example, I believe we'll incorrectly allow the following to typecheck (and then crash in SILGen like example in the bug report):

struct S {
  func foo() -> Int! { 0 }
}
let s = S()

let x1: Int = (S.foo(s))() // Paren surrounds a parent CallExpr of a double application.
let x2: Int = (s.foo.self)() 
let x3: Int = (s.foo as () -> Int)()