unsafe_derive_deserialize: do not consider pin!() as unsafe

[samueltardieu]

In Rust 1.88, the pin!() macro uses unsafe and triggers unsafe_derive_deserialize.

Fixes #15120

changelog: [unsafe_derive_deserialize]: do not trigger because of the standard library's pin!() macro

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

This will be replaced by a diagnostic item check once rust-lang/rust#143015 is merged and synced.

[y21]

Hmm, isn't this also a general issue with any macro containing unsafe being expanded there, not just pin!()? Would it be enough to just check if !span.in_external_macro(), or is it done in this way to avoid introducing FNs? I'm trying to come up with a counter-example where we'd actually want to lint external macro code but I can't come up with a case where it'd be useful

[danielhenrymantilla]

This seems oddly specific to pin!; what about something more general such as:

Suggested change

	.source_callee()
	.and_then(|expr| expr.macro_def_id)
	.is_none_or(|did| {
	(self.cx.tcx.crate_name(did.krate), self.cx.tcx.item_name(did)) != (sym::core, sym::pin)
	})
	.from_expansion()
	.not()

• or, if using a LatePass, https://doc.rust-lang.org/1.87.0/nightly-rustc/rustc_span/struct.Span.html#method.in_external_macro seems idoneous.

[samueltardieu]

Hmm, isn't this also a general issue with any macro containing unsafe being expanded there, not just pin!()? Would it be enough to just check if !span.in_external_macro(), or is it done in this way to avoid introducing FNs? I'm trying to come up with a counter-example where we'd actually want to lint external macro code but I can't come up with a case where it'd be useful

As I understand it, the reasoning behind this lint is that as soon as you have unsafe code in your methods, you may be counting on having total control over the content of your structure, which may no longer be true if it is built through deserialization. However, the fact that pin!() now contains unsafe code is an implementation detail which is still perfectly safe (at least as safe as before it started using unsafe internally).

I am not a user of this lint, so I have a hard time judging whether someone would want to be warned if an external macro they use start doing unsafe things behind the scene.

[danielhenrymantilla]

Yeah, it is a fair stance; probably what the original authors had in mind, back in the day when the notion around who is responsible for an unsafe was muddier, precisely because we did not have the unsafe_op_in_unsafe_fn lint, and so many people (including this very lint) incorrectly blamed an unsafe fn definition as "unsafe code", because "unsafe code" is not actually the important metric, or at least, not with this ambiguous terminology.

It should rather be about "precondition-assuming unsafe code", that is, usages of unsafe which appear to state, to Rust/the compiler, that the author thereby promises to have checked something in the compiler's stead, so the language ought to allow them to continue, thereby betting the very sanity / well-defined-ness of the execution that follows from there. That is, this is the place where, if a mistake is done, UB can ensue. unsafe { … } blocks, and unsafe impls, as well as #[unsafe(no_mangle)] or other such attributes, are all instances of this.

unsafe fn, however, is not, much like unsafe traits. This just makes it harder to call the function (impl the trait), by putting the onus of an unsafe { … } (unsafe impl) on the caller (implementer).

Now enter macros. We don't have unsafe macros, and that is where things get muddier. The safety model here cannot be as perfect and clear as with fns and traits.

But the convention is the following:

• if a macro is unsafe-to-invoke, meaning, a user can, with a contrived invocation (but no unsafe code whatsoever), cause, ultimately, somewhere, UB, then it must be the fault of some of this non-unsafe code, which includes the macros it invoked, especially if those did indeed emit unsafe { … } or unsafe impls under the hood.

  For instance:

  macro_rules! deref_read {( $pointer:expr $(,)? ) => (
      match $pointer { pointer => unsafe {
          <*const _>::read(pointer)
      }}
  )}
  
  let x: i32 = deref_read!(ptr::null()); // Uh-oh

  The convention is then that such an unsafe-to-invoke macro ought to require the caller/invoker to provide an unsafe token to it, as the "license" for the macro itself to do its own unsafe shenanigans, under the assumption that invoker and invokee have agreed on the preconditions to be upheld.

  macro_rules! deref_read {( $unsafe:tt, $pointer:expr $(,)? ) => (
      match $pointer { pointer => $unsafe {
          <*const _>::read(pointer)
      }}
  )}
  
  let x: i32 = deref_read!(unsafe, &42 as *const i32); // OK

  • Back to this PR, this means that the unsafe token detected by the lint, when querying its .span() and its .from_expansion()-ness, i.e., whether it stems from a macro expansion , the answer will correctly be negative, as that unsafe token is, verbatim, the one the macro invoker provided, which does not stem from a macro.

• And then there are the vast majority of macros which, much like fns out there, may need to use unsafe themselves, internally, as an implementation detail, which shall not leak to the caller, let alone have them trigger UB.

  macro_rules! pin {( $e:expr $(,)? ) => ({
      super let mut place = $e;
      unsafe { $crate::…::Pin::new_unchecked(&mut place) }
  })}

  pin! might be a typical example, but there will be a bunch of other similar macros (if anything, the other 3rd party pinning macros, such as ::futures::pin_mut!()).

With all that in context / having been said, I do believe it makes sense to ignore any unsafe token that originates from macro-generated code (plus, in general, it is preferable for a lint to have false negatives than false positives):

• the one part where we could go a bit further is if the macro whence the unsafe originated was actually defined in the very crate that invoked it. In that case, between invoker and invokee, there is an unsafe block, and it could be legitimate to lint it.

  Hence my LatePass suggestion about using .in_external_macro(…) rather than the coarser .from_expansion() 🙂

[samueltardieu]

I agree to all that, but this is a larger change which affects the semantics of the lint. We might choose to do that, and that will fix the pin!() issue at the same time, or we might fix the pin!() issue and discuss whether we want to make this lint less restrictive in a second phase. In particular, while I see the unsafe being passed to the macro as a convention in the standard library itself, I don't know whether this is widely used in the Rust ecosystem.

Btw, even in the standard library, unsafe might be passed to the macro but still come from expansion (it's not matched as a tt): https://github.com/rust-lang/rust/blob/master/library/core/src/num/nonzero.rs#L156C1-L165C2