Require users of msg_send! to specify the return type

[SimonSapin]

This is a breaking change, to prevent misuses such as #62.

[SimonSapin]

I found after making this patch #62 (comment) which suggests an alternative idea: requiring the return type to implement some (unsafe) trait that is not implemented for !. This has the advantage of not changing every single call sites in users of the crate.

[SSheldon]

@SimonSapin you mention in rust-lang/rust#65355 (comment):

The match expression where an unconstrained type parameter is unified with the type of a panic!() expression is hidden in a macro expansion

If we remove the match and panic expressions, will that help? If so, I can explore some refactor there. Would a trivial reordering like this prevent the types from being unified?

let r = $crate::__send_message(...);
if r.is_err() {
    panic!("{}", r.as_ref().unwrap_err());
}
return r.unwrap();

[SimonSapin]

I don’t know, it’s worth trying

[SimonSapin]

Is there a reason the panic is part of a macro expansion rather than in __send_message or some other library function?

[SSheldon]

Just tested that change on rustc 1.35 and confirmed that with it, I get a compile error at the usage site of the macro:

error[E0282]: type annotations needed
  --> examples/example.rs:28:9
   |
28 |         msg_send![*obj, retain];
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
   |         |
   |         cannot infer type for `R`
   |         consider giving `r` a type
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

Neat! Didn't realize that I could get the misuse case to compile error. I think we should just make a change like that.

[SSheldon]

@SimonSapin I wanted the panic to report the line and file where msg_send! was used rather than always reporting a line from inside the objc crate, especially for when people hit errors and don't have backtraces enabled. That was all!

[SimonSapin]

Re line numbers I see, that makes sense.

It’s rather extreme, but how would you feel about yanking all current versions? See rust-lang/rust#65355 (comment)

[SSheldon]

Regarding yanking, I wouldn't love that, but we can keep it under consideration. First we'll have to figure out how the heck to get everyone to fix the existing misuses of msg_send!...

Closing this in favor of df957f2, a more backwards compatible way to prevent this.

[cmyr]

Just noticed that this was published as 0.2.7, e.g. a patch; noticed because ci started failing in a downstream crate. Shouldn't this at least be 0.3?

Edit okay, I found #62, ignore me ✌️

[SSheldon]

@cmyr sorry for the inconvenience! A bit more on my reasoning in rust-lang/rust#65355 (comment) and #86 (comment).

If I had published the change as 0.3, you would have undefined behavior when ! stabilizes. Using msg_send! without a return type was never an intentional feature of this crate, and with the resulting undefined behavior, I considered this a soundness fix and an unfortunate but necessary breakage.

Let me know if I can help the crate you're using get fixed! We tested the biggest users of objc and made sure they had fixes ready, but the crate you're using may have slipped under the radar.

[cmyr]

@SSheldon no worries, the rationale makes sense, the fix wasn't that painful. I'm glad I can do let () = .. instad of let _: () = ..,. In any case this is up as druid#219. In that thread raph makes a good point, which is that reflexively casting unused results to () might cause other problems. Is there any room for concern here? I would expect to be able to cast any id to a void*, but I'm not sure about casting to void. 😕

edit: here's an example where I've casted an NSTimer* to ().

edit edit: also I assume you've seen https://www.mikeash.com/pyblog/objc_msgsends-new-prototype.html? I found it really interesting. :)