[SR-4827] Incorrect result resolving type for Obj-C block

[swift-ci]

Previous ID	SR-4827
Radar	rdar://problem/23666040
Original Reporter	benasher44 (JIRA User)
Type	Bug
Status	Resolved
Resolution	Done

Environment

macOS 10.12.4
Swift 3.1
Xcode 8.3.2

Also:
macOS 10.12.5
Swift 3.2
Xcode 9.0 beta (9M136h)

Additional Detail from JIRA

Votes	3
Component/s	Compiler
Labels	Bug, ClangImporter, RunTimeCrash, StarterBug
Assignee	benasher44 (JIRA)
Priority	Medium

md5: 5590c8a0ad8e21dbbb40a05d1c250d51

is duplicated by:

• SR-8330 Obj-C methods returning @objc-marked swift classes are not visible in swift code in frameworks
• SR-8410 Swift Classes with Custom Objective C Class Names Not Visible to Swift
• SR-8653 Renaming protocol using @objc(RenamedProtocol) makes method unavailable
• SR-11337 Problem with @objc and renaming
• SR-11339 Problems when naming classes/protocols using @objc

Issue Description:

Consider the following files:

SampleClass.h

#import <Foundation/Foundation.h>

@class SomeFactory;

@interface SampleClass: NSObject

+ (void)performWithSample:(SampleClass *)sample completion:(void(^)(SampleClass *, SomeFactory *))completionBlock;

@end

@interface OtherSampleClass: NSObject

@property (nonatomic, strong) SomeFactory *factory;

@end

SampleClass.m

#import "SampleClass.h"
#import "Test-Swift.h"

@implementation SampleClass

+ (void)performWithSample:(SampleClass *)sample completion:(void(^)(SampleClass *, SomeFactory *))completionBlock {
    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^{
        SomeFactory *f = [[SomeFactory alloc] init];
        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^{
            completionBlock(sample, f);
        });
    });
}

@end


@implementation OtherSampleClass

@end

test.swift

import Foundation

@objc(SomeFactory)
final class _ObjCSomeFactory: NSObject {
}

final class SomeFactory {
}

final class SomethingElse {
    let os = OtherSampleClass()
    func foo() {
        let s = SampleClass()
        SampleClass.perform(withSample: s) { (sample, factory) in
            print("\(type(of: factory))")
            self.os.factory = factory
            print("\(type(of: factory))")
        }
    }
}

func main() {
    let s = SomethingElse()
    s.foo()
}

main()

// Ensure program doesn't end before async blocks finish
let g = DispatchGroup()
g.enter()
g.wait()

This can be run using the following commands:

swiftc -c test.swift -import-objc-header Test-Bridging-Header.h -F/System/Library/Frameworks/Foundation.framework -emit-objc-header-path Test-Swift.h
swiftc -c test.swift -import-objc-header Test-Bridging-Header.h -F/System/Library/Frameworks/Foundation.framework
clang SampleClass.m -o SampleClass.o -c
swiftc -o test SampleClass.o test.o
./test

The program runs and prints the following:

Optional<SomeFactory>
Optional<SomeFactory>
Segmentation fault: 11 ./test

The type being printed here is incorrect. This should be printing Optional<_ObjCSomeFactory>. The bad type-checker result can be further exposed by annotating the block like so with more type information:

…
SampleClass.perform(withSample: s) { (sample: SampleClass?, factory: _ObjCSomeFactory?) -> Void in
…

Updating the code like this causes a compiler error:

error: cannot convert value of type '(SampleClass?, _ObjCSomeFactory?) -> Void' to expected argument type '((SampleClass?, SomeFactory?) -> Void)!'

By adding these type annotations, it's exposed that the block in SampleClass.h has been resolved to have a parameter of type SomeFactory?, when it should be _ObjCSomeFactory?.

As for the segfault (in the actual program that I derived this example from) Instruments showed that it was crashing because the closure executed by SampleClass in SomethingElse.foo ends up over-releasing factory when the closure is cleaned up (in _dispatch_call_block_and_release). This seems like it might be related to the bad type resolution?

We stared at this code for a long time trying to figure out where there could possible be a memory management bug here, and then we arrived at the fact that the types, as Swift understands them, in the closure that's implemented in Swift, but defined and called in Obj-C, are incorrect. Those types being understood as correct seems like an important prerequisite for Swift getting the memory management correct in this scenario.

[belkadan]

I was wondering when someone was going to catch us on this. Currently there's no way to get from the Objective-C class name "SomeClass" back to the Swift class "_ObjCSomeClass"—there's no connection between the two without looking at every class in the program for the one with the given name. But that's clearly the right thing to do if you really need to write code like this.

[belkadan]

That's basically going to be the implementation strategy if anyone wants to take this:

1. Do a regular lookup, the way we always do.
2. If the lookup succeeds, check the Objective-C name to make sure it matches.
3. Look through every top-level class / protocol in the module to see if it has a matching ObjC name. (Nested classes just lose on this one; they're not supposed to be accessible from Objective-C.)

I'm tagging this as a StarterBug for someone to add step 2; don't feel like you have to do step 3 as well. That will at least prevent the miscompile.

[swift-ci]

Comment by Ben A (JIRA)

@belkadan awesome thanks for the explanation and notes for someone who'd work on this!

[swift-ci]

Comment by Leon Li (JIRA)

Hey Jordan & Ben. I would really like to fix this bug. As it is my first time looking at Swift's source code, it would be really good if you can give me more detailed instructions like which file and/or function I should start looking at and what are the related classes/data structures. Cheers!

[belkadan]

Sure![]( I do suggest just aiming to do the Step 2 part first before moving on to Step 3. The relevant code here is in ImportDecl.cpp under {{resolveSwiftDeclImpl}}; what you'd want to do is see if the Swift declaration's attributes (getAttrs) include an ObjCAttr with a custom name, and then check if that custom name is different from what we looked up. If so, that's not the thing we're looking for)

When you've got something working, you can modify one of the test cases inside test/ClangImporter/MixedSource/, and test (see docs/Testing.rst), then open a PR on the Swift repo. Feel free to tag me as a reviewer when you get there! (And of course to ask questions along the way.)

[swift-ci]

Comment by Leon Li (JIRA)

Thank you Jordan for the instructions. I was able to reproduce the project with a simple Xcode project. Now I am trying to run a specific test case to reproduce the problem. I don't really know how to do this. Ideally it would be good if I can write an unit test case in Xcode which I can run within the IDE. I looked at 'test/ClangImporter/MixedSource/' and not sure which test I should run. And how I can run those tests within Xcode?

Many thanks!

[swift-ci]

Comment by Benjamin Encz (JIRA)

FYI, this issue is still occurring in Swift 3.2.

[belkadan]

Resetting assignee for all Starter Bugs not modified since 2018.

[ChristopherRogers]

I've submitted a pull request for this. It pretty much follows the implementation steps that Jordan explained. This particular bug could be resolved by just excluding Swift declarations that weren't being exposed to Obj-C.

#27682

[belkadan]

We had to revert this for now because we found a case where people were relying on the old behavior (NS_SWIFT_NAME with nested names). I'll follow up with a test case shortly.

[belkadan]

Back to Resolved now that #27921 is in.