Address exclusivity warnings in JSONEncoder

[devincoughlin]

An idiom used in JSONEncoder is violating Swift's rules about exclusive
access to memory. The 'with(pushedKey:)' family of mutating functions
on EncodingContainers modify 'self' by pushing a key, executing a
passed-in closure, and popping the key. However, when the passed-in closure
refers to 'self' this violates the rules for exclusive access since the
closure reads self while with(pushedKey:) has an already in-progress
modification of 'self'.

return self.with(pushedKey: _JSONKey(index: self.count - 1)) {
// Referring to 'self.codingPath' violates rules for exclusive access.
...
}

The compiler is warning about this. To address these warnings, change
the callback to with(pushKey:) to accept an additional parameter: the
modified encoding container. This enables the closure to safely refer to it
without accessing via the captured self and avoids the violation of
exclusive access to memory. This also makes it explicit that in the body the
body of the closure the value of .codingPath read is the mutated one and
not the original.

[devincoughlin]

@swift-ci please test

[devincoughlin]

@itaiferber Would you be willing to take a look?

[itaiferber]

Thanks for taking a look at this and putting this together! In swiftlang/swift#10821 we eliminated the issue by simply removing with<T> altogether (this was preferred because we need to defer in case something throws and not corrupt the coding path), and unfortunately, it looks like we forgot to port those changes over.

To keep things in line, do you think we could match suit here instead?

[parkera]

Porting those changes is where I ran into the bridging issues in #1347.

[parkera]

I agree we should not diverge further here.

[itaiferber]

@parkera I don't think the specific changes made in swiftlang/swift#10821 caused the bridging issues in #1347; they don't touch any of the code surrounding that. I think it should be possible to apply those pretty cleanly.

[parkera]

All I did there was merge in the same changes (before any of the key strategy stuff), but if we're more successful here then -- great!

[devincoughlin]

Updated to bring in the changes from swiftlang/swift#10821 instead.

[devincoughlin]

@swift-ci please test

[devincoughlin]

@swift-ci please test

[devincoughlin]

@itaiferber is this OK to merge? The only interesting conflicts I had to resolve were making sure the encode() functions continue to call '._bridgeToObjectiveC()' (they didn't in swiftlang/swift#10821).

[ianpartridge]

Thanks for working on this! There are still quite a large number of differences between the version in the overlay and the result of this PR: https://www.diffchecker.com/XV65PeTY

Our goal is to bring the file in this repo back in sync with the overlay, as far as possible. Would you be willing to do that under this PR?

[ikesyo]

@ianpartridge JSON key strategy part should not be included in this PR; that should be tracked separately in #1347.

[devincoughlin]

@ianpartridge I'm doing this to unblock a needed compiler change (swiftlang/swift#15915). I don't think we should generally block compiler changes on swift-corelibs-foundation being in sync with the overlays.

[ianpartridge]

I see - I wasn't aware of the compiler work. Thanks for the info. In that case, if @itaiferber is happy we should merge this as-is.

[itaiferber]

Looks good, thanks for taking care of this!

[devincoughlin]

@ianpartridge To be clear, I think the criterion that the divergence shouldn't get any worse makes a lot of sense!