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

Enable the deny(unsafe_op_in_unsafe_fn) lint everywhere in zcash-light-client-ffi we define unsafe functions #65

Closed
daira opened this issue Nov 15, 2022 · 5 comments · Fixed by #66
Labels
enhancement New feature or request

Comments

@daira
Copy link
Contributor

daira commented Nov 15, 2022

From https://github.com/zcash-hackworks/zcash-light-client-ffi/pull/32/files#r1022962018 :

A side-effect of declaring a function unsafe is that unsafe operations in its body are no longer required to be in unsafe blocks (see Rust RFC 2585 for a proposal to change this, not yet adopted). Ideally, we should enable the deny(unsafe_op_in_unsafe_fn) lint everywhere we define unsafe functions, since the style we write FFI functions in is mostly already consistent with that lint.

@daira daira added the enhancement New feature or request label Nov 15, 2022
@daira daira changed the title Enable the deny(unsafe_op_in_unsafe_fn) lint everywhere we define unsafe functions Enable the deny(unsafe_op_in_unsafe_fn) lint everywhere in zcash-light-client-ffi we define unsafe functions Nov 15, 2022
@daira
Copy link
Contributor Author

daira commented Nov 15, 2022

This can be enabled crate-wide by adding

[build]
rustflags = ["-Dunsafe_op_in_unsafe_fn"]

in .cargo/config.

@sellout
Copy link
Contributor

sellout commented Nov 15, 2022

This should also update all extern functions with safety requirements on callers to unsafe extern.

@nuttycom
Copy link
Contributor

I mentioned this in a comment, but has @str4d chimed in on this change yet? He had suggested to me that he preferred that the function not be labeled unsafe, but instead that the unsafe operations be explicitly demarcated by unsafe blocks internal to the function, so that it's easier to identify which parts are actually performing unsafe operations.

@sellout
Copy link
Contributor

sellout commented Nov 16, 2022

Yeah, that’s the primary portion of this issue – using deny(unsafe_op_in_unsafe_fn) so that we can have both.

And get back to vacation!

@daira
Copy link
Contributor Author

daira commented Nov 16, 2022

This will be fixed by #66 (which includes #67).

@pacu pacu closed this as completed in #66 Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants