Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Aug 30, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
32a0c8e
[AST] Strip off parens around an ApplyExpr's function
theblixguy Aug 25, 2019
f02a5f3
[Test] Add a test case
theblixguy Aug 25, 2019
ff7e8b1
[CSSimplify] If we're trying to assign the result of an IUO method wr…
theblixguy Aug 26, 2019
4fd2a4c
[CS] Move the check to CSGen
theblixguy Aug 27, 2019
76c29af
[Test] Adds a test case
theblixguy Aug 27, 2019
0660764
[CS] Move the check to resolveOverload()
theblixguy Aug 27, 2019
b33c520
[CS] Extract the check into a separate lambda
theblixguy Aug 27, 2019
6797148
[CS] Only check for the presence of a paren expr
theblixguy Aug 27, 2019
32cfd5b
[CS] Only check if the decl has a function type
theblixguy Aug 27, 2019
28d0c08
[CSApply] shouldForceUnwrapResult() should return false when we don't…
theblixguy Aug 28, 2019
a2daa11
Merge branch 'master' into fix/SR-10492
theblixguy Aug 28, 2019
532498b
[CSApply] Use the choiceLocator, not locator when checking if a disju…
theblixguy Aug 28, 2019
b32386b
[CS] If locator points to a function call, then compare the fn and se…
theblixguy Aug 29, 2019
fc9fef2
[CS] Be more defensive in isIUOWrappedInParens() check
theblixguy Aug 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2272,6 +2272,17 @@ namespace {
auto *choiceLocator = cs.getConstraintLocator(locator.withPathElement(
ConstraintLocator::ImplicitlyUnwrappedDisjunctionChoice));

// We won't have a disjunction choice if we have an IUO function call
// wrapped in parens (i.e. '(s.foo)()'), because we only create a
// single constraint to bind to an optional type. So, we can just return
// false here as there's no need to force unwrap.
if (!solution.DisjunctionChoices.count(choiceLocator)) {
auto type = choice.getDecl()->getInterfaceType();
assert((type && type->is<AnyFunctionType>()) &&
"expected a function type");
return false;
}

return solution.getDisjunctionChoice(choiceLocator);
}

Expand Down
27 changes: 26 additions & 1 deletion lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2190,9 +2190,34 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
openedFullType,
refType};

// If we have something like '(s.foo)()', where 's.foo()' returns an IUO,
// then we need to only create a single constraint that binds the
// type to an optional.
auto isIUOCallWrappedInParens = [&]() {
auto decl = choice.getDecl();
auto type = decl ? decl->getInterfaceType() : nullptr;
if (!type || !type->is<AnyFunctionType>())
return false;

auto expr = locator->getAnchor();
if (!expr)
return false;

if (isa<CallExpr>(expr)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@theblixguy theblixguy Sep 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will give that a try tomorrow 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

return false;
}

auto parentExpr = getParentExpr(expr);
if (parentExpr && isa<ParenExpr>(parentExpr))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;

return false;
};

// In some cases we already created the appropriate bind constraints.
if (!bindConstraintCreated) {
if (choice.isImplicitlyUnwrappedValueOrReturnValue()) {
if (choice.isImplicitlyUnwrappedValueOrReturnValue() &&
!isIUOCallWrappedInParens()) {
// Build the disjunction to attempt binding both T? and T (or
// function returning T? and function returning T).
buildDisjunctionForImplicitlyUnwrappedOptional(boundType, refType,
Expand Down
23 changes: 23 additions & 0 deletions test/Constraints/iuo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,26 @@ var y: Int = 2

let r = sr6988(x: x, y: y)
let _: Int = r

// SR-10492

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

let sr_10492_s = SR_10492_S()
let sr_10492_int1: Int = (sr_10492_s.foo)() // expected-error {{value of optional type 'Int?' must be unwrapped to a value of type 'Int'}}
// expected-note@-1 {{coalesce}}{{44-44= ?? <#default value#>}}
// expected-note@-2 {{force-unwrap}}{{44-44=!}}
let sr_10492_int2: Int? = (sr_10492_s.foo)() // Okay


class SR_10492_C1 {
init!() {}
}

class SR_10492_C2 {
init(_ foo: SR_10492_C1) {}
}

let bar = SR_10492_C2(SR_10492_C1()) // Okay
8 changes: 8 additions & 0 deletions validation-test/compiler_crashers_2_fixed/sr10492.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: not %target-swift-frontend -emit-silgen %s

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

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