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

[ObjC Bridging] Consistently bridge block types verbatim #77496

Merged
merged 1 commit into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 18 additions & 8 deletions stdlib/public/runtime/Casting.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1289,9 +1289,8 @@ static id bridgeAnythingNonVerbatimToObjectiveC(OpaqueValue *src,
swift_unknownObjectRetain(result);
return result;
}

// Dig through existential types.
if (auto srcExistentialTy = dyn_cast<ExistentialTypeMetadata>(srcType)) {
else if (auto srcExistentialTy = dyn_cast<ExistentialTypeMetadata>(srcType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function has always been a long if-else chain; I've reformatted it some to make that a bit more explicit.

I've also adjusted the comment layout to hopefully make it clearer:

// Explain first if
if (...) {
  ...
}
// Explain second if
else if (....) {
  ...
}
// Explain third if
else if (...) {
  ...
}

OpaqueValue *srcInnerValue;
const Metadata *srcInnerType;
bool isOutOfLine;
Expand Down Expand Up @@ -1320,10 +1319,9 @@ static id bridgeAnythingNonVerbatimToObjectiveC(OpaqueValue *src,
}
return result;
}

// Handle metatypes.
if (isa<ExistentialMetatypeMetadata>(srcType)
|| isa<MetatypeMetadata>(srcType)) {
// Handle metatypes and existential metatypes
else if (isa<ExistentialMetatypeMetadata>(srcType)
|| isa<MetatypeMetadata>(srcType)) {
const Metadata *srcMetatypeValue;
memcpy(&srcMetatypeValue, src, sizeof(srcMetatypeValue));

Expand All @@ -1342,8 +1340,9 @@ static id bridgeAnythingNonVerbatimToObjectiveC(OpaqueValue *src,
return objc_retain(protocolObj);
}
}
}
// Handle bridgeable types.
} else if (auto srcBridgeWitness = findBridgeWitness(srcType)) {
else if (auto srcBridgeWitness = findBridgeWitness(srcType)) {
// Bridge the source value to an object.
auto srcBridgedObject =
srcBridgeWitness->bridgeToObjectiveC(src, srcType, srcBridgeWitness);
Expand All @@ -1353,13 +1352,24 @@ static id bridgeAnythingNonVerbatimToObjectiveC(OpaqueValue *src,
srcType->vw_destroy(src);

return (id)srcBridgedObject;
}
// Handle Errors.
} else if (auto srcErrorWitness = findErrorWitness(srcType)) {
else if (auto srcErrorWitness = findErrorWitness(srcType)) {
// Bridge the source value to an NSError.
auto flags = consume ? DynamicCastFlags::TakeOnSuccess
: DynamicCastFlags::Default;
return dynamicCastValueToNSError(src, srcType, srcErrorWitness, flags);
}
// Handle functions: "Block" types can be bridged literally
else if (auto fn = dyn_cast<FunctionTypeMetadata>(srcType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new case: If this is a function type with a block convention, then bridge it verbatim.

if (fn->getConvention() == FunctionMetadataConvention::Block) {
id result;
memcpy(&result, src, sizeof(id));
if (!consume)
swift_unknownObjectRetain(result);
return result;
}
}

// Fall back to boxing.
return (id)bridgeAnythingToSwiftValueObject(src, srcType, consume);
Expand Down
76 changes: 76 additions & 0 deletions test/Casting/Cast_Blocks.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Casts.swift - Tests for conversion between types.
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
// -----------------------------------------------------------------------------
///
/// Contains tests for non-trapping type conversions reported by users.
///
// -----------------------------------------------------------------------------
// RUN: %empty-directory(%t)
//
// RUN: %clang %target-cc-options -isysroot %sdk -fobjc-arc %S/Inputs/Cast_Blocks/Cast_Blocks.m -c -o %t/Cast_Blocks.o -g
//
// RUN: %target-build-swift -I %S/Inputs/Cast_Blocks %t/Cast_Blocks.o -swift-version 6 -g -Onone -module-name a %s -o %t/a.swift6.Onone.out
// RUN: %target-codesign %t/a.swift6.Onone.out
// RUN: %target-run %t/a.swift6.Onone.out
//
// RUN: %target-build-swift -I %S/Inputs/Cast_Blocks %t/Cast_Blocks.o -swift-version 6 -g -O -module-name a %s -o %t/a.swift6.O.out
// RUN: %target-codesign %t/a.swift6.O.out
// RUN: %target-run %t/a.swift6.O.out
//
// RUN: %target-build-swift -I %S/Inputs/Cast_Blocks %t/Cast_Blocks.o -swift-version 5 -g -Onone -module-name a %s -o %t/a.swift5.Onone.out
// RUN: %target-codesign %t/a.swift5.Onone.out
// RUN: %target-run %t/a.swift5.Onone.out
//
// RUN: %target-build-swift -I %S/Inputs/Cast_Blocks %t/Cast_Blocks.o -swift-version 5 -g -O -module-name a %s -o %t/a.swift5.O.out
// RUN: %target-codesign %t/a.swift5.O.out
// RUN: %target-run %t/a.swift5.O.out
//
// REQUIRES: executable_test
// REQUIRES: objc_interop
// UNSUPPORTED: use_os_stdlib
// UNSUPPORTED: back_deployment_runtime

import StdlibUnittest
import Foundation
import Cast_Blocks

fileprivate func SwiftThinksObjectIsSwiftValue<T>(_ t: T) -> Bool {
let type = "\(type(of: t))"
return type == "__SwiftValue"
}

let CastsTests = TestSuite("Cast_Blocks")

CastsTests.test("block closures should bridge without __SwiftValue")
{
let x: @convention(block) () -> Void = {}

expectFalse(ObjCThinksObjectIsSwiftValue(x))

expectFalse(SwiftThinksObjectIsSwiftValue(x))

ObjCCanCallBlock(x);
}

// Bug: @convention(block) closure used to be incorrectly wrapped in
// SwiftValue box when bridged to Objective-C as a member of an array
CastsTests.test("block closures in array should bridge without __SwiftValue")
{
let f: @convention(block) () -> Void = {}
let x = ([f] as NSArray)[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test does NSArray bridging explicitly here, but the bug also manifests when a Swift array is implicitly bridged to NSArray when calling Obj-C code.


expectFalse(ObjCThinksObjectIsSwiftValue(x))
expectFalse(SwiftThinksObjectIsSwiftValue(x))
ObjCCanCallBlock(x);
}


runAllTests()
9 changes: 9 additions & 0 deletions test/Casting/Inputs/Cast_Blocks/Cast_Blocks.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#ifndef SWIFT_TEST_CAST_BLOCKS_H
#define SWIFT_TEST_CAST_BLOCKS_H

#import <Foundation/Foundation.h>

BOOL ObjCThinksObjectIsSwiftValue(id obj);
void ObjCCanCallBlock(id block_as_id);

#endif
19 changes: 19 additions & 0 deletions test/Casting/Inputs/Cast_Blocks/Cast_Blocks.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#import <objc/runtime.h>

#import "Cast_Blocks.h"

BOOL ObjCThinksObjectIsSwiftValue(id obj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's important to verify that Objective-C really does see what we expect.

Class cls = object_getClass(obj);
const char *name = class_getName(cls);
if (strcmp(name, "__SwiftValue") == 0) {
return TRUE;
} else {
return FALSE;
}
}

void ObjCCanCallBlock(id block_as_id) {
typedef void(^blockType)(void);
blockType block = (blockType)block_as_id;
block();
}
4 changes: 4 additions & 0 deletions test/Casting/Inputs/Cast_Blocks/module.modulemap
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module Cast_Blocks {
header "Cast_Blocks.h"
export *
}