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

Hack: Force UIEdgeInsets.zero to always come from the SDK #17122

Merged
merged 1 commit into from Jun 12, 2018

Conversation

jrose-apple
Copy link
Contributor

...instead of relying on the one in the overlay in pre-4.2 versions of Swift. This caused crashes in deserialization, which (deliberately) doesn't respect availability.

There are three changes here:

  • Remove UIEdgeInsets.zero and UIOffset.zero from the UIKit overlay.
  • Always use the 4.2 name for UIEdgeInsetsZero and UIOffsetZero from the underlying UIKit framework. (This is the nested name.)
  • Ignore the unavailability messages for those two constants in pre-4.2 Swift, since we're now relying on them being present.

The latter two, the compiler changes, can go away once UIKit's API notes no longer specify different pre-4.2 behavior, but meanwhile we need to keep compatibility with the SDKs released in Xcode 10b1.

SR-7879 / rdar://problem/40735990

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - b4d95c8f11980ac5a5befa4fa118fdec281ccc08

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - b4d95c8f11980ac5a5befa4fa118fdec281ccc08

@jrose-apple
Copy link
Contributor Author

Let's try that again.

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - b4d95c8f11980ac5a5befa4fa118fdec281ccc08

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - b4d95c8f11980ac5a5befa4fa118fdec281ccc08

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility


import UIKit

_ = UIEdgeInsets.zero
Copy link
Member

Choose a reason for hiding this comment

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

Can you try defining default arguments with these values as well so that we can test serializing references to these globals too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. The original bug showed up just a mention of the SIL global in the partial swiftmodules, but having an actual reference to it is important.

@slavapestov
Copy link
Member

@jrose-apple I'm curious why XREFs to imported types don't respect the module filter. This would have fixed the problem without adding a special case.

@jrose-apple
Copy link
Contributor Author

That doesn't actually fix the problem, because we don't want to find the one in the overlay. We want the one in the actual Clang module.

(The original motivation, though, was when we thought swiftmodule was going to be a stable format, in which case it needs to cope with declarations being moved from one framework to another, something that's legal in Objective-C.)

...instead of relying on the one in the overlay in pre-4.2 versions of
Swift. This caused crashes in deserialization, which (deliberately)
doesn't respect availability.

There are three changes here:

- Remove UIEdgeInsets.zero and UIOffset.zero from the UIKit overlay.
- Always use the 4.2 name for UIEdgeInsetsZero and UIOffsetZero from
  the underlying UIKit framework. (This is the nested name.)
- Ignore the unavailability messages for those two constants in
  pre-4.2 Swift, since we're now relying on them being present.

The latter two, the compiler changes, can go away once UIKit's API
notes no longer specify different pre-4.2 behavior, but meanwhile we
need to keep compatibility with the SDKs released in Xcode 10b1.

https://bugs.swift.org/browse/SR-7879
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 8a8adf71812a4f3ce3ac082bb46c808128e203f2

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 8a8adf71812a4f3ce3ac082bb46c808128e203f2

@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test Linux

@jrose-apple jrose-apple merged commit a6ae2d7 into apple:master Jun 12, 2018
@jrose-apple jrose-apple deleted the UIEdgeInsetsZero branch June 12, 2018 20:40
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Jun 12, 2018
...instead of relying on the one in the overlay in pre-4.2 versions of
Swift. This caused crashes in deserialization, which (deliberately)
doesn't respect availability.

There are three changes here:

- Remove UIEdgeInsets.zero and UIOffset.zero from the UIKit overlay.
- Always use the 4.2 name for UIEdgeInsetsZero and UIOffsetZero from
  the underlying UIKit framework. (This is the nested name.)
- Ignore the unavailability messages for those two constants in
  pre-4.2 Swift, since we're now relying on them being present.

The latter two, the compiler changes, can go away once UIKit's API
notes no longer specify different pre-4.2 behavior, but meanwhile we
need to keep compatibility with the SDKs released in Xcode 10b1.

https://bugs.swift.org/browse/SR-7879
(cherry picked from commit a6ae2d7)
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Jun 12, 2018
...instead of relying on the one in the overlay in pre-4.2 versions of
Swift. This caused crashes in deserialization, which (deliberately)
doesn't respect availability.

There are three changes here:

- Remove UIEdgeInsets.zero and UIOffset.zero from the UIKit overlay.
- Always use the 4.2 name for UIEdgeInsetsZero and UIOffsetZero from
  the underlying UIKit framework. (This is the nested name.)
- Ignore the unavailability messages for those two constants in
  pre-4.2 Swift, since we're now relying on them being present.

The latter two, the compiler changes, can go away once UIKit's API
notes no longer specify different pre-4.2 behavior, but meanwhile we
need to keep compatibility with the SDKs released in Xcode 10b1.

https://bugs.swift.org/browse/SR-7879
(cherry picked from commit a6ae2d7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants