Remove unsafe from Buffer::typed_data

[alamb]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
As pointed out by @jhorstmann on #921 at #921 (comment),

Buffer::typed_data does not actually need to be marked unsafe since it checks the alignment requirements. The very similarly implemented MutableBuffer::typed_data_mut is not marked as unsafe. The safety notes mention bool as a special case, but that is no longer an ArrowNativeType since a while.

https://github.com/apache/arrow-rs/blob/6a6e7f7/arrow/src/buffer/immutable.rs#L160-L181

Describe the solution you'd like

1. Remove unsafe from Buffer::typed_data
2. Remove note about bool in docstrings

[alamb]

I reviewed the code a bit more, and the comments make a great point:

    /// `ArrowNativeType` is public so that it can be used as a trait bound for other public
    /// components, such as the `ToByteSlice` trait.  However, this means that it can be
    /// implemented by user defined types, which it is not intended for.

Meaning that if a user implements ArrowNativeType for their types, this will result in undefined behavior.

I also looked more carefully at Buffer and it effectively allows reinterpreting arbitrary bytes as different types, so I am not sure that behavior is safe...

What could
be done is make MutableBuffer::typed_data unsafe instead as the docs there say very specifically

    /// # Safety
    /// This function must only be used when this buffer was extended with items of type `T`.
    /// Failure to do so results in undefined behavior.

[alamb]

Closing this one as I don't think it is actionable upon further review. FYI @jhorstmann

[jhorstmann]

Ok, I wasn't aware that implementing ArrowNativeType for arbitrary types by a user is a supported usecase. Maybe making that into a sealed trait would also be an option. Anyway, my comment was more about the inconsistency between MutableBuffer::typed_data and Buffer::typed_data, so marking both unsafe is also fine.

[alamb]

Sounds good -- I'll file some follow on tickets.

…

On Sat, Dec 4, 2021 at 10:35 AM Jörn Horstmann ***@***.***> wrote: Ok, I wasn't aware that implementing ArrowNativeType for arbitrary types by a user is a supported usecase. Maybe making that into a sealed trait would also be an option. Anyway, my comment was more about the inconsistency between MutableBuffer::typed_data and Buffer::typed_data, so marking both unsafe is also fine. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#996 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXZMIHSUGILKQBP7XUE7DUPIYMPANCNFSM5JJ6HAWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[alamb]

Follow on tickets:

• MutableBuffer::typed_data_mut is not safe #1027
• Consider sealing ArrowNativeType  #1028