magnus icon indicating copy to clipboard operation
magnus copied to clipboard

Safety improvements to `rb-sys-interop`

Open ianks opened this issue 2 years ago • 2 comments

This PR is a safety + ergonomics revamp to the rb-sys-interop feature. I want to provide some basic sanity checks around when converting from raw ID and VALUE. Previously, the methods id_from_raw and value_from_raw were infallible, but I think we should at least attempt to provide some safety around these functions when we can. There may be more cases we can check as well (such as checking that the value % 40 == 0?), but this is a good start.

ianks avatar Jul 23 '22 03:07 ianks

Quick feedback: sorry this is brief and potentially full of mistakes, but hopefully it’s helpful getting this feedback sooner.

I think I’d like this to be more explicitly not part of the regular Magnus api at the call site in end user code. I’d be happy with something like AsRawValue and AsRawId traits that would need to be explicitly used in a scope to get the as_raw/from_raw methods. Hopefully that’d be a nice balance of improved ergonomics while keeping it clear in end user code and published examples that these functions aren’t for regular use.

I think any error checking should be debug asserts, that only run for debug builds and panic rather than returning an error. Passing an non-VALUE value to from_raw isn’t an error that can be handled, it’s something so completely wrong it invalidates the whole program and should crash. But also these functions should optimise away to nothing, but error checks would stop that, so it should be for debug builds only.

matsadler avatar Jul 23 '22 04:07 matsadler

Done @matsadler

ianks avatar Jul 25 '22 16:07 ianks

@matsadler Finally got around to updating this. I also added a PartialEq derivation for ID which is useful to have, but happy to remove if you do not want it.

ianks avatar Sep 07 '22 01:09 ianks

This is great, thanks!

Sorry for taking such a long time to merge it.

matsadler avatar Oct 18 '22 02:10 matsadler