[Coding Guideline] Revise guideline on unsafe code in macros

[rcseacord]

Updated guideline on unsafe code in macros to clarify the importance of preserving 'unsafe' token visibility. Added examples of compliant and non-compliant macro implementations.

[netlify]

✅ Deploy Preview for scrc-coding-guidelines ready!

Name	Link
🔨 Latest commit	84c32f4
🔍 Latest deploy log	https://app.netlify.com/projects/scrc-coding-guidelines/deploys/697fcdbeae7e540008ba1a4a
😎 Deploy Preview	https://deploy-preview-368--scrc-coding-guidelines.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[felix91gr]

I opened a Zulip thread to ask about this: How should one combine unsafe and declarative macros?.

I wanted to know how useful this pattern (of having an unsafe block inside of a macro's code) is.

And it seems like it is quite useful, and necessary in some cases:

A very simple example by scottmcm:

let slice = $crate::MyUnsafeTrait::get_me_the_thing($e);
// SAFETY: `MyUnsafeTrait` promises that `get_me_the_thing` returns a non-empty slice
unsafe { slice.get_unchecked(0) }

Another example, this time from std (explained by y21):

Are there things that this unlocks, that are otherwise impossible or unreasonable to achieve with e.g. invoking the macro inside of an unsafe block instead?

I think the pin! macro is also a good example. It calls the unsafe function Pin::new_unchecked in an unsafe block, and it can do so soundly because it takes ownership of the value and only gives back a Pin<&mut _>, ensuring that it stays pinned forever. This only works because it's a macro, and there's no reason to require an unsafe block at the use site because the safety contract of Pin::new_unchecked is upheld by the macro itself; there's nothing that a user needs to uphold when invoking this macro. So an unsafe block inside of the macro is perfectly valid there

[rcseacord]

@felix91gr I think we can both seen the value or need for this rule?

maybe we can make some adjustments around valid use cases.

Something like:

You can have an unsafe macro with an arbitrary identifier like the pin! macro if the macro provided the macro upholds the safety contract and there is no way to misuse the macro to introduce undefined behavior.

If you have an unsafe macro that exposes unsafe behavior you must prefix the macro name with the string "unsafe_"

Unsafe macros without a prefix are not allowed.

Or you could break this up into a strict advisory rule and a less strict required rule.

It's not great when the language / library violates your rule.

MISRA does this with rules like don't use restrict which is used throughout the C library, but it's mostly because they don't know what they are talking about.

[felix91gr]

You can have an unsafe macro with an arbitrary identifier like the pin! macro if the macro provided the macro upholds the safety contract and there is no way to misuse the macro to introduce undefined behavior.

I agree. This seems very reasonable: we would be classifying those, so-to-say, as "safe macros which internally have unsafe code".

In Rust, for safe APIs that contain unsafe code within, the contract they must uphold is that there must be no way of calling them from safe code and introduce Undefined Behavior.

So, this idea would be following the same kind of standard, only for macros instead of functions.

The burden of proof is higher (one must prove that any use of the macro cannot introduce UB), but if done properly (like we believe to be the case for the pin! macro), it seems like a reasonable and scalable approach.

It would also only require local reasoning about safety which is good. It would be much like how properly encapsulating unsafe code within safe APIs allows for local reasoning about them, and thus, scalability of it.

If you have an unsafe macro that exposes unsafe behavior you must prefix the macro name with the string "unsafe_"

Continuing with the theme above, this seems very reasonable as well. It would be the macro equivalent of an unsafe function: "if you call this macro (/ fn), you have to make sure the x, y and z invariants are preserved.", as well as "you may only call this macro (/ fn) from within an unsafe block".

Which are themselves the rules for unsafe functions (hence why the (/ fn))

Unsafe macros without a prefix are not allowed.

That's another parallel to unsafe functions: "if a macro (/fn) requires to manually preserve invariants (to avoid causing UB from the code it expands), then it must be labeled as such". In the case of functions, there's the unsafe keyword to label them as such. For macros, we don't have that, but for now a prefix in the name (+ linting) should be enough.

It's not great when the language / library violates your rule.

MISRA does this with rules like don't use restrict which is used throughout the C library, but it's mostly because they don't know what they are talking about.

🤣 it makes me smile to see you dunk on MISRA, dunno why

In their defense: does gcc have full and sound restrict support? I know LLVM only supports a small subset of it. And back when it had broader support for it, it was incredibly broken.

Given that history (and how hard a time LLVM at least has had trying to fully implement it), I would not be surprised if a broad ban on restrict were raised in the past.