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

Fix so the compiler can infer msg_send! return types #1227

Merged
merged 2 commits into from
Oct 18, 2019
Merged

Fix so the compiler can infer msg_send! return types #1227

merged 2 commits into from
Oct 18, 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 winit to compile with the fixed version of objc. It also bumps cocoa to ensure users of winit are on a version that has addressed those issues.

  • Tested on all platforms changed
    • I ran cargo test on macos, close enough?
  • Compilation warnings were addressed
    • No compilation warnings added by this change!
  • cargo fmt has been run on this branch
    • No, it's such a li'l change and I don't have rustfmt installed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
    • Probably not valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
    • No user-facing changes!
  • Created or updated an example program if it would help users understand this functionality
    • No functionality change!
  • Updated feature matrix, if new features were added or implemented
    • No new features!

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 winit to compile with the
fixed version of objc.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Should we be bumping the objc dependency as well?

@SSheldon
Copy link
Contributor Author

@aleksijuvani that's a great question that I will try not to write too long of an answer about 😛

Currently there is no new version of objc to bump to. I'm planning to publish the new version as 0.2.7, so if I publish it right now it'll get checked out for new/updating users and cause compile failures for them. The obvious question then is: isn't that a breaking change and shouldn't it be 0.3.0? If I publish as 0.3, those people won't hit compile failures, but when ! stabilizes their code will begin silently experiencing undefined behavior. Unfortunately, among those two options, I think a compile failure is better than silent undefined behavior. Thankfully, it seems like most crates using objc won't be hit by this issues.

So my plan has been: try to fix all the issues I can before publishing, then afterwards publish 0.2.7 which will cause compile failures in some cases, but hopefully that headache is better than undefined behavior.

(Also 0.3 of objc would mean big compatibility issues and a change to every crate, not just ones that have this problem.)

@Osspial Osspial merged commit af3ef52 into rust-windowing:master Oct 18, 2019
@SSheldon
Copy link
Contributor Author

Awesome, thank you!

@Osspial, do y'all have plans for publishing another alpha of 0.20? Will that be soon? I'm wondering if I should wait for the next alpha with these fixes before I publish objc 0.2.7. (At this point cocoa has published fixes for the 0.18 and 0.19 releases, and every other dependency I've checked is looking good, so I think winit 0.20 is the last place that would be impacted by the new objc version.)

@SSheldon SSheldon deleted the fix_omitted_ret_types branch October 18, 2019 16:55
@Osspial
Copy link
Contributor

Osspial commented Oct 18, 2019

@SSheldon I just uploaded Alpha 4 about ten minutes ago!

hecrj pushed a commit to hecrj/winit that referenced this pull request Oct 20, 2019
…1227)

* Fix so the compiler can infer msg_send! return type

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 winit to compile with the
fixed version of objc.

* Bump cocoa to 0.19.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants