Turn on ‘as’ bridging on Linux. #16022
Conversation
millenomi
changed the title
|
The Foundation counterpart is swiftlang/swift-corelibs-foundation#1526. |
test/stdlib/KeyPath.swift
Outdated
| expectTrue(fields[0] as! AnyObject === subject.a) | ||
| expectTrue(fields[1] as! AnyObject === subject.b) | ||
| expectTrue(fields[2] as! AnyObject === subject.c) | ||
| // expectTrue(fields[0] as! AnyObject === subject.a) |
There was a problem hiding this comment.
These are failing without _SwiftValue (bullet 2 in the description); the intent is to reenable them before finishing.
| @@ -800,7 +798,7 @@ static bool _dynamicCastToExistential(OpaqueValue *dest, | |||
| // A fallback to use if we don't have a more specialized approach | |||
| // for a non-class type. | |||
| auto fallbackForNonClass = [&] { | |||
| #if SWIFT_OBJC_INTEROP | |||
| #if SWIFT_OBJC_INTEROP // TODO | |||
There was a problem hiding this comment.
The // TODOs are for the implementation of _SwiftValue (the second bullet in the PR description).
There was a problem hiding this comment.
This is done and the TODOs need to be removed.
|
@millenomi Should we consider renaming |
|
I believe we should. We can investigate it as a last step in this patch or as a separate PR, given how extensively the name appears in the codebase. Are we bound by any SE document to use that particular name, or is it free for arbitrary change? |
|
I think since the names in question are underscored they're not public API and are not part of the SE process. |
lib/Sema/CSApply.cpp
Outdated
| @@ -8120,6 +8117,7 @@ bool ConstraintSystem::applySolutionFix(Expr *expr, | |||
| case CheckedCastKind::Coercion: | |||
| case CheckedCastKind::BridgingCoercion: | |||
| llvm_unreachable("Coercions handled in other disjunction branch"); | |||
| return false; | |||
There was a problem hiding this comment.
This isn't necessary (or useful). llvm_unreachable exits the process.
There was a problem hiding this comment.
I don't remember the circumstances for this. It will be removed.
|
cc @phausler |
|
This new push adds preliminary Tests that were disabled above need to be re-enabled after this. |
|
(As per comment: memory management is likely incorrect, since the new branch is not heeding the casting flags yet.) |
|
cc @slavapestov @DougGregor @jckarter to sanity check before we commit to this path? |
|
This is also introducing a linker error in my local tests that I'm not quite sure how to approach; making the symbol public doesn't seem to change things: Since Casting.cpp.o, at a minimum, refers to other |
|
I have found the spot in Stdlib.cpp where to add the non-Swift stub for those tests. |
|
@swift-ci please test |
|
(I'm not sure I can kick off tests on this repository.) |
|
Build failed |
|
Build failed |
| @@ -800,7 +798,7 @@ static bool _dynamicCastToExistential(OpaqueValue *dest, | |||
| // A fallback to use if we don't have a more specialized approach | |||
| // for a non-class type. | |||
| auto fallbackForNonClass = [&] { | |||
| #if SWIFT_OBJC_INTEROP | |||
| #if SWIFT_OBJC_INTEROP // TODO | |||
There was a problem hiding this comment.
This is done and the TODOs need to be removed.
| @@ -158,16 +158,6 @@ print(allToAll(type(of: C()), AnyObject.self)) // CHECK-NEXT: true | |||
| // Bridging | |||
| print(allToAll(0, AnyObject.self)) // CHECK-NEXT: true | |||
|
|
|||
| // This will get bridged using _SwiftValue. | |||
There was a problem hiding this comment.
These need to be readded in, but I need some help figuring out how to address the NSCopying one — if it is enough to import Foundation on Linux so that Foundation can then extend _SwiftValue with NSCopying.
There was a problem hiding this comment.
(This will require making Linux's _SwiftValue public as a Foundation SPI.)
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
There was a problem hiding this comment.
Overall the changes look great, much reduction of #ifdefs, many unified code paths, very cross platform! 😍 the direction of this change looks good to me. I cannot speak to the full impact I. The compiler and the memory management questions but the rest of the changes seem quite reasonable.
|
This needs to build with the corresponding Foundation patch. |
|
Please test with the following PR: @swift-ci please smoke test |
|
Please test with the following PR: @swift-ci please test source compatibility |
|
SwiftPM failed to build without an error message. It may be a transient issue; retrying while also kicking it off locally to check. |
|
BTW, this also will enable a handful more tests to run on Linux! |
|
@compnerd Could you give the patch a once-over? I'm trying to diagnose the failures but many of them look to be memory corruption or management issues that
Any input is appreciated. (cc: @jrose-apple @slavapestov @jckarter) |
|
Please test with the following PR: @swift-ci please smoke test |
|
Please test with the following PR: @swift-ci please smoke test |
|
Please test with the following PR: @swift-ci please test source compatibility |
|
Please test with the following PR: @swift-ci please smoke test |
|
This last push includes a likely fix for the tests that takes into account the new ObjC-like SIL generated for casts on native. |
|
NSError tests are failing. |
|
Please test with the following PR: @swift-ci please smoke test |
|
Please test with the following PR: @swift-ci please test source compatibility |
|
Something is wrong — it seems like it's trying to smoke test with a previous version of the Foundation patch (which contained fatalError("TODO").) |
|
Maybe I started the test too early; starting another in case. |
|
Please test with the following PR: @swift-ci please smoke test |
1 similar comment
|
Please test with the following PR: @swift-ci please smoke test |
millenomi
changed the title
|
Major update:
|
|
Please test with the following PR: @swift-ci please smoke test |
|
Test failures aren't patch-related: https://bugs.swift.org/browse/SR-7716 |
|
Please test with the following PR: @swift-ci please test source compatibility |
|
Please test with the following PR: @swift-ci please smoke test |
|
I reverted this because it broke in CI. Please take a look. |
|
Please wait for a reviewer from Anna's team as well before merging this. |
|
@jrose-apple I will fix the 14.04 issue and repost this patch. Is there anyone you would recommend look at this patch, or who would know who? |
|
Thank you. I will tag people again once the 14.04 build issue is figured out and I reopen the PR. |
| @@ -10,6 +10,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #if _runtime(_ObjC) | |||
There was a problem hiding this comment.
Do we try to build the ObjectiveC module on non-ObjC-interop platforms? I didn't see a CMakeLists.txt change to that effect.
There was a problem hiding this comment.
I can revert this if it’s unneeded. Part of the debugging cycle for this patch was building it on Darwin simulating the Linux situation so that I could iterate on it with Xcode; it may be a leftover from that.
| @usableFromInline | ||
| internal class _SwiftValue { | ||
| @usableFromInline | ||
| let value: Any |
There was a problem hiding this comment.
You could avoid double-indirecting using a small class hierarchy:
open class _SwiftValue {}
internal class _SwiftValueOf<T>: _SwiftValue {
let value: T
init(_ value: T) { self.value = value }
}
@_silgen_name("_swift_makeSwiftValue")
func _makeSwiftValue<T>(_ value: T) -> _SwiftValue { return _SwiftValueOf(value) }
This might also let you represent null as a singleton subclass instead of as a wrapper around a specific kind of Optional. Maybe you could also make it so that Foundation's preexisting _SwiftValue and NSNull are Swift._SwiftValue subclasses, so that they're substitutable for stdlib _SwiftValues, instead of the _NSSwiftValue/_SwiftValueFlavor hacks too.
There was a problem hiding this comment.
We cannot reparent NSNull, and I would be loathe to reparent _SwiftValue for the reasons outlined below. Unless we reparent NSObject, which is possible but would be really weird.
| } else { | ||
| return _SwiftValue(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a plan to retire Foundation._SwiftValue once this is all done?
There was a problem hiding this comment.
The problem is that _SwiftValue is an NSObject subclass on Apple platforms, and we probably can't undo that.
There was a problem hiding this comment.
How does that become important in practice? We don't promise that on Darwin even, we "just haven't gotten around" to making it an independent root class.
There was a problem hiding this comment.
It becomes important when it interacts with CF API. Right now, calling CFCopy() will go through the NSCopying conformance that all SwiftValues implicitly have to, and that is still possible on Linux as well (you can toll-free bridge swift-corelibs-foundation’s NSArray/Dictionary to CFArray/DictionaryRef the same way you can on Darwin, and these may contain _SwiftValues and be set up to copy.)
There was a problem hiding this comment.
(It is also secondarily important for two reasons:
- It allows Linux to respond to
is/as? NSObject/NSCopyingthe same way Darwin does, which can be a subtle issue in codebases where that question is posed in code that doesn’t know it may be receiving a boxed value; - Existing boxing code in s-c-f already assumes that boxed values can be freely casted to NSObject (and uses its Hashable conformance to eg use them as dictionary keys); the churn was pretty big and would have introduced a number of forced casts and uses of AnyHashable, so I went instead with the solution that also he the property of being the closest match to Darwin.
There was a problem hiding this comment.
At a high level, I don't think changing the boxing behavior dynamically based on the presence of Foundation is workable—what happens when a program that doesn't use Foundation dynamically loads a library that does? as? NSObject working is not something we want to guarantee works with plain boxed values on Darwin. We control the CF implementation corelibs uses on Linux, so could we make CFCopy do the right thing with Swift boxes?
If we really must make boxed values be NSObject subclasses when Foundation is loaded, we should look into doing so in a way that doesn't invalidate boxed values that exist before Foundation is loaded. For instance, we could make it so that boxing uses heap objects that start out with non-class heap metadata, and overwrite the metadata with an NSObject subclass when Foundation gets loaded.
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
I feel like this would be best written in C++ to have full access to the low-level metadata APIs. Keeping the entire implementation would be cleaner, and probably lead to more understandable code, than doing the oblique things you need to approximate this in Swift slowly and opaquely, such as sprinkling new protocols like _Unwrappable for peeling optionals or new small bits of runtime/stdlib SPI like _extractDynamicValue for peeling existentials. With Optional we also need to be careful that boxing preserves the difference between different levels of .none nesting in nested Optionals—for T??, .none and .some(.none) should be able to round-trip through a box as distinct values.
There was a problem hiding this comment.
Part of why I went with Swift over C++ was the issue of correctness of memory management; is there documentation on the expectations and SPI? My understanding is that with the current work the documentation in the repository may no longer apply.
There was a problem hiding this comment.
The main thing that would've changed since then is that Swift functions now take arguments +0 by default (and we have macros to try to handle both +1 and +0 conventions as a transitional/fallback mechanism, though I'm not sure whether that's important for new code, cc @gottesmm). The Darwin implementation of bridging lives largely in the C++ runtime, and we have fairly extensive runtime tests that you should be able to gradually enable that check for object lifetime issues.
|
Thanks for the initial reviews; any feedback will be applied to the new PR at #16736. |
See https://forums.swift.org/t/bridging-for-swift-corelibs-foundation-on-linux/11994/1 for a full rationale.
This is a work-in-progress patch to enable 'as', 'as?', 'as!' bridging so that it is possible to compile more code that's valid on Darwin on Linux as well.
The final target for this patch is to enable:
NSString(…) as String,"abc" as NSString,1.23 as NSNumber)._SwiftValue) reference types and vice versa (withstruct X {}:([X(), X(), X()] as NSArray) as! Array<X>).(The initial push for this PR implements the former.)
This does not target enabling other
as{,?,!}casts that work on Darwin but not on Linux that strictly require the ObjC runtime:X.selffrom/toAnyObjectAnyObjectFixes https://bugs.swift.org/browse/SR-138.