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-7877] 4.2: Pointer-to-context source compatibility regression #50412

Closed
zwaldowski opened this issue Jun 5, 2018 · 22 comments
Closed

[SR-7877] 4.2: Pointer-to-context source compatibility regression #50412

zwaldowski opened this issue Jun 5, 2018 · 22 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself regression swift 4.2

Comments

@zwaldowski
Copy link

Previous ID SR-7877
Radar rdar://problem/40945329
Original Reporter @zwaldowski
Type Bug
Status Closed
Resolution Done
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
Votes 0
Component/s Compiler
Labels Bug, 4.2Regression
Assignee @xedin
Priority Medium

md5: 4c23009be7a3042533ee6256c1cfceb1

is duplicated by:

  • SR-7898 [4.2 regression] can't use & to make UnsafeMutableRawPointer in switch case

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
}
}

}

@belkadan
Copy link
Contributor

belkadan commented Jun 5, 2018

@rudkx, @xedin, we have a dup for this, right?

@xedin
Copy link
Member

xedin commented Jun 6, 2018

@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.

@belkadan
Copy link
Contributor

belkadan commented Jun 6, 2018

Wait, you definitely can't create a separate static function that goes from inout to UnsafePointer. That's going from "probably unsafe" to "definitely unsafe and likely disallowed in a future version of Swift".

@xedin
Copy link
Member

xedin commented Jun 6, 2018

Just a messenger, but that's one of the ways of working around this which has discussed as viable, am I remembering correct, @rudkx?

@rudkx
Copy link
Member

rudkx commented Jun 7, 2018

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 &, especially as it relates to inout-to-pointer conversions, and this was one such change.

Unlike in C/C++, & is not a simple "address of", and doesn't really provide much in the way of guarantees for the pointer produced by the implicit conversion.

@zwaldowski
Copy link
Author

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?

@rudkx
Copy link
Member

rudkx commented Jun 7, 2018

@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;
    }
  }
}

@belkadan
Copy link
Contributor

belkadan commented Jun 8, 2018

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.)

@zwaldowski
Copy link
Author

Yeah, I'd be able to make do with it but Jordan summarizes my fears way better than I could.

@rudkx
Copy link
Member

rudkx commented Jun 8, 2018

/cc @jckarter, @atrick,

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?

@jckarter
Copy link
Member

jckarter commented Jun 8, 2018

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).

@xedin
Copy link
Member

xedin commented Jun 8, 2018

@swift-ci create

@xedin
Copy link
Member

xedin commented Jun 8, 2018

I'm going to revert parser changes since I think I remember the place 🙂

@jckarter
Copy link
Member

jckarter commented Jun 8, 2018

IMO we want the parser change, at least for Swift 5 mode. We should keep Swift 4 code working though.

@rudkx
Copy link
Member

rudkx commented Jun 8, 2018

I was going to make a similar suggestion - perhaps we can just extend the parsing to allow for & to be used for the expression in switch as well as the {{case}}s, and limit it to versions < 5.

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 & is used.\

@xedin, thanks for taking this on!

@xedin
Copy link
Member

xedin commented Jun 8, 2018

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?

@rudkx
Copy link
Member

rudkx commented Jun 8, 2018

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 @autoclosures, and had a change that I started to further restrict them so that you can never infer the converted-to-type to be a pointer type - it has to already be declared as such.

The only other place that I see people use & today is with an argument to == and =, or add method on the pointer types for comparing them.

@atrick
Copy link
Member

atrick commented Jun 9, 2018

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.

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 withUnsafePointer.

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 UnsafePointer )

@xedin
Copy link
Member

xedin commented Jun 9, 2018

I've created #17078 to address this problem and added minimized test-case.

@xedin
Copy link
Member

xedin commented Jun 13, 2018

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?

@zwaldowski
Copy link
Author

Confirmed addressed on the 6/18 build.

@xedin
Copy link
Member

xedin commented Jun 19, 2018

@zwaldowski great! Please close this issue then 🙂

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 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 regression swift 4.2
Projects
None yet
Development

No branches or pull requests

7 participants