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

Fixes so the compiler can infer msg_send! return types #340

Merged
merged 1 commit into from
Oct 16, 2019
Merged

Fixes so the compiler can infer msg_send! return types #340

merged 1 commit into from
Oct 16, 2019

Conversation

SSheldon
Copy link
Contributor

Currently, due to a quirk in Rust's type inference interacting with the structure of the msg_send! macro, a () return type will be inferred when the compiler cannot otherwise determine the return type. This behavior is expected to change, and in the future could resolve to a ! return type, which results in undefined behavior.

Linting has previously been added for this in rust-lang/rust#39216, but it did not catch these cases due to SSheldon/rust-objc#62. An upcoming version of objc will be fixed to stop hiding these errors, at which point they will become compile errors.

This change fixes these errors and allows cocoa to compile with the fixed version of objc.

Currently, due to a quirk in Rust's type inference interacting with the
structure of the msg_send! macro, a () return type will be inferred when
the compiler cannot otherwise determine the return type. This behavior
is expected to change, and in the future could resolve to a ! return
type, which results in undefined behavior.

Linting has previously been added for this in rust-lang/rust#39216, but
it did not catch these cases due to SSheldon/rust-objc#62. An upcoming
version of objc will be fixed to stop hiding these errors, at which
point they will become compile errors.

This change fixes these errors and allows cocoa to compile with the
fixed version of objc.
@jdm
Copy link
Member

jdm commented Oct 16, 2019

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 0a1b0d6 has been approved by jdm

bors-servo pushed a commit that referenced this pull request Oct 16, 2019
Fixes so the compiler can infer msg_send! return types

Currently, due to a quirk in Rust's type inference interacting with the structure of the `msg_send!` macro, a `()` return type will be inferred when the compiler cannot otherwise determine the return type. This behavior is expected to change, and in the future could resolve to a `!` return type, which results in undefined behavior.

Linting has previously been added for this in rust-lang/rust#39216, but it did not catch these cases due to SSheldon/rust-objc#62. An upcoming version of objc will be fixed to stop hiding these errors, at which point they will become compile errors.

This change fixes these errors and allows cocoa to compile with the fixed version of objc.
@bors-servo
Copy link
Contributor

⌛ Testing commit 0a1b0d6 with merge f7c2096...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: jdm
Pushing f7c2096 to master...

@bors-servo bors-servo merged commit 0a1b0d6 into servo:master Oct 16, 2019
@SSheldon
Copy link
Contributor Author

Thanks @jdm!

Related: how do y'all feel about backported versions here? There's still crates relying on cocoa 0.18 that will need these fixes (primarily glutin and winit). glutin and winit have already moved on to developing alpha versions of their next releases. So either we can backport a cocoa 0.18 fix here or they can backport fixes that update to cocoa 0.19 to get this fix.

If I can do something to make a cocoa 0.18.5, that means fewer PRs overall for me 😛

@SSheldon
Copy link
Contributor Author

As I'm exploring more of the dependency graph, I'm thinking the backport option would be more and more helpful 😛

If you're amenable to it, I've prepared a branch that backports this fix: https://github.com/SSheldon/core-foundation-rs/tree/backport_0.18.5

bors-servo pushed a commit that referenced this pull request Oct 16, 2019
Update cocoa version to 0.19.1

New version needed so that the #340 fixes can be published.
@SSheldon SSheldon deleted the fix_omitted_ret_types branch October 16, 2019 17:41
@SSheldon
Copy link
Contributor Author

@jdm @SimonSapin is there anyone I can talk to about a backport release? Sorry, not sure the right venue/way to bring this up. I think a backport for 0.18 would be really helpful; as described in rust-lang/rust#65355 (comment), over a third of the downloads for cocoa are still the 0.18 release.

@jdm
Copy link
Member

jdm commented Oct 17, 2019

A backport release makes sense. I'll try and figure out the logistics of how to do that based on your branch.

@SSheldon
Copy link
Contributor Author

@jdm if we want a bit more of a paper trail: if you create a new empty branch based off of 8ffba8a, I can open a PR to merge my backport changes into that. No worries if that seems unnecessary, I'm sure you've got it covered.

@jdm
Copy link
Member

jdm commented Oct 17, 2019

@SSheldon
Copy link
Contributor Author

Perfect, I opened #342, will continue any discussion there. Thanks much @jdm!

bors-servo pushed a commit that referenced this pull request Oct 17, 2019
Fixes so the compiler can infer msg_send! return types (backport to 0.18)

This is a backport of #340 onto the 0.18 release.

Currently, due to a quirk in Rust's type inference interacting with the structure of the `msg_send!` macro, a `()` return type will be inferred when the compiler cannot otherwise determine the return type. This behavior is expected to change, and in the future could resolve to a `!` return type, which results in undefined behavior.

Linting has previously been added for this in rust-lang/rust#39216, but it did not catch these cases due to SSheldon/rust-objc#62. An upcoming version of objc will be fixed to stop hiding these errors, at which point they will become compile errors.

This change fixes these errors and allows cocoa to compile with the fixed version of objc.
bors bot added a commit to gfx-rs/metal-rs that referenced this pull request Oct 18, 2019
101: Bump winit r=kvark a=SSheldon

The version of winit used in the window example pins an old version of cocoa lacking a soundness fix (servo/core-foundation-rs#340) for undefined behavior in an upcoming version of rust (rust-lang/rust#65355).

Testing done: ran the window example after this change; it compiles and looks fine!

Co-authored-by: Steven Sheldon <steven@sasheldon.com>
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.

3 participants