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-4827] Incorrect result resolving type for Obj-C block #47404

Closed
swift-ci opened this issue May 7, 2017 · 11 comments
Closed

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

swift-ci opened this issue May 7, 2017 · 11 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself crash Bug: A crash, i.e., an abnormal termination of software good first issue Good for newcomers run-time crash Bug → crash: Swift code crashed during execution

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented May 7, 2017

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
Copy link
Contributor

belkadan commented May 8, 2017

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
Copy link
Contributor

belkadan commented May 8, 2017

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
Copy link
Collaborator Author

swift-ci commented May 8, 2017

Comment by Ben A (JIRA)

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

@swift-ci
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

swift-ci commented Jun 2, 2017

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
Copy link
Collaborator Author

Comment by Benjamin Encz (JIRA)

FYI, this issue is still occurring in Swift 3.2.

@belkadan
Copy link
Contributor

Resetting assignee for all Starter Bugs not modified since 2018.

@ChristopherRogers
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Back to Resolved now that #27921 is in.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added the crash Bug: A crash, i.e., an abnormal termination of software label Dec 12, 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 crash Bug: A crash, i.e., an abnormal termination of software good first issue Good for newcomers run-time crash Bug → crash: Swift code crashed during execution
Projects
None yet
Development

No branches or pull requests

4 participants