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-7877] 4.2: Pointer-to-context source compatibility regression #50412
Comments
@belkadan Yes, Deferred had the same problem - rdar://problem/40171796 @zwaldowski We are aware of this and consensus between @DougGregor, @rudkx and me was that we should move away from allowing this because it depends on the fact that ~= operator has overload which matched because of implicit conversion from inout to unsafe pointer. AFAIK you can either create a separate static function which is going to take inout of T and return unsafe pointerand keep using switch statement or convert this into series of if statements. |
Wait, you definitely can't create a separate static function that goes from |
Just a messenger, but that's one of the ways of working around this which has discussed as viable, am I remembering correct, @rudkx? |
I believe the comment I made was that one could, at the present time, trivially work around the restriction here by making such a function. That doesn't mean that it's something we'd encourage or support long term. We've been slowly moving toward better enforcement of where you can use Unlike in C/C++, |
w.r.t. the source compatibility suite, I suppose that one's my fault too… 😉 So the solution here is doubly relevant! I'm happy to make a code change, but switching to if/else doesn't seem like that's a permanent fix; rather, that it's likely to be the cause of another future source compatibility problem. Can someone help me understand why ~= working with pointer conversion is a problem, but == isn't? Certainly it seems like the latter is also on the table for breaking based on the post I link above. A magic context token is going to remain a problem for C interop even as the state of the art for KVO evolves away. I'm sympathetic to Swift's need to not make guarantees around where things i.e., statics live, but using a static is still Apple's recommendation. Is there a better way that will "always" work? Or can we explore other ways to make the guarantee, like by only allowing trivial types to participate in the magic? |
@zwaldowski, have you considered switching on some element of the contents of the context pointer you pass when registering the observer? For example something like: class ViewController {
static var context1 = 1
static var context2 = 2
static var context3 = 3
func observeValue(context: UnsafeMutableRawPointer?) {
guard let context = context else {
return
}
switch context.bindMemory(to: Int.self, capacity: 1).pointee {
case ViewController.context1:
break;
case ViewController.context2:
break;
case ViewController.context3:
break;
default:
break;
}
}
} |
The context pointer is not guaranteed to be loadable, even if non-nil. (Other parts of the system that also use KVO can register their own callbacks.) |
Yeah, I'd be able to make do with it but Jordan summarizes my fears way better than I could. |
The regression here is due to parser changes. I recall a related evolution forum thread suggesting that we should only allow '&' to be parsed in argument positions. I'm not aware of what all the guarantees and limitations are around the results of '&', and I don't know of any place this is clearly spelled out, either. It seems like we should have those guarantees and limitations clearly spelled out somewhere, and a plan to work towards enforcement of what we can enforce. In the meantime, perhaps we need to back this parser change out and allow this again? |
The Cocoa Interop Guide describes the limitations of `&`, but we'd like to simplify the model to the point where there are no special dispensations beyond what you get with an inout argument, which is why we made `&` part of call syntax. In this particular case, we have informally blessed using the address of global variables as unique tokens for KVO contexts (since you're not reading or writing from the pointer, and there's no good reason we'd ever change the address of a global in memory). |
@swift-ci create |
I'm going to revert parser changes since I think I remember the place 🙂 |
IMO we want the parser change, at least for Swift 5 mode. We should keep Swift 4 code working though. |
I was going to make a similar suggestion - perhaps we can just extend the parsing to allow for The question I have is if we always expect that to produce sensible code, e.g. will does SILGen always ensure the switch expression and case are lowered to a simple expression with I just don't know enough about what if any assumptions SILGen might be making about how the result of @xedin, thanks for taking this on! |
Yeah, I think it would be pretty straightforward to make that work, I just need to fix couple of places to pass `allowAmpPrefix` to enable that for guarded patterns of case statements, I was under impression that we should do this for all versions since once people migrate to version 5 they are going to be in the same situation once again because there is no safe alternative? |
Based on previous discussion, the impression I have was that the inout-to-pointer conversions were only ever intended to be used in the context of calling imported Objective-C code that expects pointers. I recently added a restriction to ensure that this conversion does not happen for The only other place that I see people use |
inout-to-pointer conversions are a normal way to drop down to UnsafePointers, particularly when multiple arguments are involved. It fits some use cases much better than func foo(src: UnsafePointer<T>, dst: UnsafeMutablePointer<T>) {
dst.pointee = src.pointee
}
foo(&x, &y) I don't care what happens with operators and resolution of fully generic argument types (that don't refer to nominal |
I've created #17078 to address this problem and added minimized test-case. |
My PR got merged into both master and 4.2 branches. @zwaldowski could you please verify using master/4.2 nightly snapshot which should be available tomorrow and resolve this issue? |
Confirmed addressed on the 6/18 build. |
@zwaldowski great! Please close this issue then 🙂 |
Environment
Xcode 10.0 beta (10L176w)
Apple Swift version 4.2 (swiftlang-1000.0.16.7 clang-1000.10.25.3)
macOS 10.14 Beta (18A293u)
Additional Detail from JIRA
md5: 4c23009be7a3042533ee6256c1cfceb1
is duplicated by:
Issue Description:
In previous releases, the following syntax has worked (see "switch"):
{code:swift}
class ViewController: NSViewController {
static var context1 = false
static var context2 = false
static var context3 = false
override func viewDidLoad() {
super.viewDidLoad()
addObserver(self, forKeyPath: #keyPath(NSViewController.title), context: &ViewController.context1)
view.addObserver(self, forKeyPath: #keyPath(NSView.bounds), context: &ViewController.context2)
view.addObserver(self, forKeyPath: #keyPath(NSView.effectiveAppearance), context: &ViewController.context3)
}
override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) {
switch context {
case (&ViewController.context1)?:
break
case (&ViewController.context2)?:
break
case (&ViewController.context3)?:
break
}
}
}
The text was updated successfully, but these errors were encountered: