Skip to content

Fix crash when emitting force-unwrap-and-call expression for optional ObjC methods #60399

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

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

kavon
Copy link
Member

@kavon kavon commented Aug 4, 2022

Due to some changes in how the AST is constructed by CSApply to handle
global actors from @preconcurrency declarations, the AST for a force-call to
an optional ObjC method that is part of a global-actor qualified protocol, such as:

import Foundation

@mainactor
@objc protocol P {
  @objc optional func generateMaybe() async
}

extension P {
  func test() async -> Void {
        await self.generateMaybe!()
    }
}

now contains a function conversion in between the forced-value operation and the dynamic-ref:

(force_value_expr type='@mainactor () async -> ()'
  (optional_evaluation_expr implicit type='(@mainactor () async -> ())?'
    (inject_into_optional implicit type='(@mainactor () async -> ())?'
      (function_conversion_expr implicit type='@mainactor () async -> ()'   <<<<< new!
        (bind_optional_expr implicit type='() async -> ()'
          (dynamic_member_ref_expr type='(() async -> ())?' ... )))))

For compatibility with how ObjC does message sends to these optional methods, in Swift there
is a difference between how something like a.method!() and the following are compiled:

let meth = a.method!
meth()

The former will directly call the optional method and let the "unknown selector" error be emitted
if it's not implemented. But the latter will query the dynamic method and inject that result into an
Optional<...> to then be forced, yielding a "force-unwrap nil" error. It's a subtle difference but
those two scenarios lead to different code paths in SILGen.

Now, this patch fixes the issue by enhancing the recognition of these direct call scenarios so that it
can look past these function conversions. Because that recognition already tries to ignore implicit
conversions, this is not something new. The problem was that we weren't considering implicit conversions
between the optional-evaluation and bind-optional expressions.

This patch is intentionally precise about what kind of function conversions can be skipped, because I
do not know if all of them are OK to blindly skip or not. We might need to expand that in the future.

Resolves rdar://97646309

…c method

Due to some changes in how the AST is constructed by CSApply to handle
global actors from @preconcurrency declarations, the AST for a force-call to
an optional ObjC method that is part of a global-actor qualified protocol, such as:

```
import Foundation

@mainactor
@objc protocol P {
  @objc optional func generateMaybe() async
}

extension P {
  func test() async -> Void {
        await self.generateMaybe!()
    }
}
```

now contains a function conversion in between the forced-value operation and the dynamic-ref:

```
(force_value_expr type='@mainactor () async -> ()'
  (optional_evaluation_expr implicit type='(@mainactor () async -> ())?'
    (inject_into_optional implicit type='(@mainactor () async -> ())?'
      (function_conversion_expr implicit type='@mainactor () async -> ()'   <<<<< new!
        (bind_optional_expr implicit type='() async -> ()'
          (dynamic_member_ref_expr type='(() async -> ())?' ... )))))
```

For compatibility with how ObjC does message sends to these optional methods, in Swift there
is a difference between how something like `a.method!()` and the following are compiled:

```
let meth = a.method!
meth()
```

The former will directly call the optional method and let the "unknown selector" error be emitted
if it's not implemented. But the latter will query the dynamic method and inject that result into an
`Optional<...>` to then be forced, yielding a "force-unwrap nil" error. It's a subtle difference but
those two scenarios lead to different code paths in SILGen.

Now, this patch fixes the issue by enhancing the recognition of these direct call scenarios so that it
can look past these function conversions. Because that recognition already tries to ignore implicit
conversions, this is not something new. The problem was that we weren't considering implicit conversions
between the optional-evaluation and bind-optional expressions.

This patch is intentionally precise about what kind of function conversions can be skipped, because I
do not know if all of them are OK to blindly skip or not. We might need to expand that in the future.

Resolves rdar://97646309
@kavon
Copy link
Member Author

kavon commented Aug 4, 2022

@swift-ci please test

@kavon
Copy link
Member Author

kavon commented Aug 5, 2022

@swift-ci please test

@kavon kavon requested a review from jckarter August 5, 2022 02:12
@kavon
Copy link
Member Author

kavon commented Aug 10, 2022

@swift-ci please smoke test

@kavon kavon merged commit b0fb7c5 into swiftlang:main Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants