non_send_fields_in_send_ty seems to be misguided

[tspiteri]

The non_send_fields_in_send_ty lint wrongly states that implementations of Send are unsound because some fields are !Send.

If the fields were Send, there would be no need to write unsafe impl Send for S {}, as S would be automatically Send. The only time you need to explicitly implement Send for a struct is when some fields aren't Send. That is why the code needs to be marked unsafe.

[tspiteri]

And here's a reduced version of actual code which triggered the false positive for me.

use core::cell::UnsafeCell;
use core::ptr::NonNull;
pub struct S(UnsafeCell<NonNull<()>>);
// Safety: S ensures that there is no aliasing even though
// a pointer (NonNull) is used rather than a reference.
unsafe impl Send for S {}

[sfackler]

I am also confused as to what the purpose of this lint is supposed to be - the entire point of unsafe impls of Send is to implement Send for a type with !Send fields!

[GuillaumeGomez]

This is indeed very unhelpful...

[sdroege]

This was added in #7709, FWIW. But the explanation there is also not very helpful, and also the example is arguably half-wrong (it shows two unsound uses, but the one concering the Rc is completely sound if the refcount of the Rc is 1, i.e. it would depend on the context and other checks that are in place or not if there is a problem).

[kpreid]

This is not about whether the lint is useful, but while reading 1.57 release notes, I noticed that the documentation for it is also wrong — it says

Sending the struct to another thread will transfer the ownership to the new thread by dropping in the current thread during the transfer.

which is not how sending works (sending is not observable / does not run code, let alone Drop). The original proposal wrote

Sending the struct to another thread and drops it there will also drop the field in the new thread.

which is a more correct (s/and drops/and then dropping/) description of the problem.

On the original question of whether there's any value at all, maybe this lint should be looking for fields which are !Send and implement Drop / have drop glue? That way, it would warn on things like Rc, which need extra caution, but not raw pointers, because they don't implement Drop. (I have no opinion on whether that's worthwhile; just brainstorming.)

[tspiteri]

On the original question of whether there's any value at all, maybe this lint should be looking for fields which are !Send and implement Drop / have drop glue? That way, it would warn on things like Rc, which need extra caution, but not raw pointers, because they don't implement Drop. (I have no opinion on whether that's worthwhile; just brainstorming.)

You can have a Rust struct wrapping an FFI struct that in turn contains a pointer. The Rust struct makes sure there is no aliasing so that it can be Send as it ensures the pointer is not aliased, but it needs Drop to free the pointer in the end.

[xFrednet]

but the one concering the Rc is completely sound if the refcount of the Rc is 1

Even if it is completely safe in that case, I think that it is correct to mark this as suspicious.

On the original question of whether there's any value at all, maybe this lint should be looking for fields which are !Send and implement Drop / have drop glue? That way, it would warn on things like Rc, which need extra caution, but not raw pointers, because they don't implement Drop. (I have no opinion on whether that's worthwhile; just brainstorming.)

Personally, I like the notion to only mark allow structs to be Send if all fields are also Send but that might be too restrictive in some applications. There is the enable-raw-pointer-heuristic-for-send configuration to allow the lint for raw pointers, we could maybe add a new configuration to have this more accepting linting behavior.

---

cc: @Qwaz What are your thoughts on this? 🙃

[kpreid]

Personally, I like the notion to only mark allow structs to be Send if all fields are also Send

This is exactly what the automatic implementation of Send does already for all types.

but that might be too restrictive in some applications.

Which is when you use unsafe impl Send. All uses of unsafe impl Send are either intentionally bypassing the restriction, or are redundant with the auto implementation.

[Qwaz]

Thanks for bringing this false positive to the attention.

TL;DR (1) This lint has a rule that handles Copy types and raw pointers, but unfortunaetly NonNull was not included. I'll work on that as soon as possible (hopefully upload a PR by the end of tomorrow EST). (2) Many people being confused about the purpose of the lint indicate that the current lint description needs an improvement.

Let me start with the background (and excuse) for this lint. This lint is designed after actual soundness bugs we found during our ecosystem scale memory safety bugs finding work, Rudra. The original proposal #7703 included 4 representative bugs, but the lint was designed after reviewing more than a hundred of Send/Sync soundness bugs throughout the Rust ecosystem (the bugs with SV tag in the PoC repository).

This lint is most useful when writing a generic data structure. There are surprising amount of unsound Send impls in published Rust crates that do unsafe impl<T> Send for Wrapper<T> without T: Send bound, and this lint helps find and fix them. However, since the lint itself is not limited only to this scope, I tried to write the description generic and beginner-friendly, and unfortunaetly it seems that it did not work well. Many people being confused about the purpose of the lint indicate that the description needs an improvement.

Regarding false postives, it already has a rule to suppress false positives related to Copy types and pointer types. The lint comes with various test cases from real world bugs, and @xFrednet ran the lintcheck tool to make sure it doesn't cause false positives for the default list of crates. Unfortunately, I overlooked NonNull and didn't include it in the logic. I'll create a PR to address that as soon as possible, hopefully by the end of tomorrow in EST.

It would be extra helpful if anyone can share (1) types that are specifically designed !Send to encourage manual Send implementation (like raw pointers and NonNull) and (2) the rationale behind using UnsafeCell<NonNull<()>> instead of using NonNull<()> directly (direct use of NonNull is in fact allowed even in the current implementation).

[kpreid]

Given that clarification, I'd say the lint documentation has two problems:

1. Instead of “This lint has a heuristic to filter out false positives”, the documentation should be written to focus on the idea that it's distinguishing between likely unsound cases (such as impl<T> for Wrapper<T> as you mention) cases and likely sound cases (bare pointers), and warning about the former only. Without that context, it sounds like the lint is redundant with the fact that Send is already an unsafe trait.

2. "Sending the struct to another thread will transfer the ownership to the new thread by dropping in the current thread during the transfer." is completely incorrect; it is both false that dropping is a part of transferring ownership and false that dropping in the current thread is the problem. This should be fixed ASAP to avoid people learning false information about how sending works when they look up this lint.

[Qwaz]

That description is indeed incorrect and I'll include that in the fix

[sdroege]

the rationale behind using UnsafeCell<NonNull<()>> instead of using NonNull<()> directly (direct use of NonNull is in fact allowed even in the current implementation).

Not exactly the same situation, but in gtk-rs we have helper structs around NonNull<T> that provide some additional functionality for the underlying raw pointers (including e.g. a Drop impl). These are never Send (or Sync), but the user-facing structs that are wrapping around them might be.

Currently this lint triggers for each of these user-facing structs that are actually Send because the lint does not know about our helper structs.

[tspiteri]

the rationale behind using UnsafeCell<NonNull<()>> instead of using NonNull<()> directly (direct use of NonNull is in fact allowed even in the current implementation).

I hit this in Rug. This is somehow similar to having a small vector which can be borrowed as a full vector, that is there is a way to get &Vec from &SmallVec. The small vector has two fields: one that mimics the Vec struct and one which is the data. The Vec itself contains a pointer to data, so the function that converts &SmallVec to &Vec has to update the pointer in its vector-mimic, so the pointer requires interior mutability, hence UnsafeCell. Of course, since the vector is borrowed it cannot be moved for as long as the &Vec exists so the pointer remains valid while &Vec exists.

[Qwaz]

Thanks for sharing more false positive cases! It seems it's evident that we need a better heuristic to handle non-Send internal types. I can see two directions to approach this problem at high level. We can (1) limit the lint to Send impls related to generic types (Send/Sync variance bugs) or (2) try to invent a ruleset to allow internal structs similar to what we do for pointer-like types.

I suggest to demote this lint back to nursery until we figure that out. @xFrednet

[sdroege]

I suggest to demote this lint back to nursery until we figure that out. @xFrednet

Unfortunately this lint ended up in the default set of lints for 1.58 now.

[xFrednet]

Yes, I think we didn't back port the group change, not that you say that 🙈 It should be in nursery on beta and nightly, but it should have been the same for stable. Sorry that I've missed that. We're currently planning on how this can be improved for the future, to prevent instances like these. Thank you for pointing that out!

[maurer]

We're needing to disable this lint for Android because it triggers with cxx's extern C++ type feature if any of them are impl'd as Send. What is happening is semantically equivalent to this example (which doesn't trigger the lint), but has a crate boundary in between A and B since B is in generated code, so the linter fires.

Specifically, cxx has the type Opaque, which is !Send, and this is correct. When defining an external C++ type, it produces a type definition which includes Opaque to maintain correct properties.

For specific types, the user may want to specify that the generated type is in fact Send, based on knowledge of the underlying type - this lint prohibits that.

I think that for this lint to be productive, at a minimum it would need to only fire on annotated structures, since marking a structure containing !Send fields as Send is literally the purpose of unsafe impl Send for.

In general though, I'm not sure there is a right way to do this lint. Even if there were an Rc in the struct (from the motivating example for the lint), it could be sent safely if all Rc and Weak made from the object remained within the struct and the field was created internally to the struct, which would be an invariant asserted by the author when they write the unsafe part of unsafe impl Send for.

[Ralith]

This is triggering a ton of false positive spam for hecs, for similar reasons to the above. Is there a plan to make a point release with the backport?

[Xaeroxe]

I'm also getting some false positives. Here's a workaround.

cargo clippy -- -A clippy::non-send-fields-in-send-ty

[xFrednet]

This is triggering a ton of false positive spam for hecs, for similar reasons to the above. Is there a plan to make a point release with the backport?

No, there is not. Even if this is annoying, I'm not convinced that the problem is big enough to justify a point release. For now, I sadly have to suggest allowing the lint. You can use the console like @Xaeroxe suggested or a crate level attribute like this:

#![allow(clippy::non_send_fields_in_send_ty)]

[Xaeroxe]

I'm concerned that a common response to this problem is going to be disabling clippy in CI. I'd strongly encourage a point release, but I acknowledge this isn't my decision to make.

[withoutboats]

I also think this should be taken out of stable in point release (made inert, rather, because people are already committing code to ignore it).

Ultimately, I think this lint needs to be reworked to something much more targeted. It catches too many false positives, even for clippy. For example, a lint that handled the wrapper case specifically would still have false positives, but would probably catch bugs far more often than not.

[pietroalbini]

Even if this is annoying, I'm not convinced that the problem is big enough to justify a point release.

The release team discussed this today as part of its weekly meeting, and we can prepare 1.58.1 for next Thursday moving the lint to the nursery. @xFrednet is backporting just 5d63a28 enough?

[Manishearth]

Yes, let's do it. I thought this already was in the nursery, didn't realize that fix hadn't been backported.

[xFrednet]

Alright, thank you very much for taking this up @pietroalbini. Porting the commit is enough, as that will make the lint allow-by-default and add it to the nursery group. 👍

[Qwaz]

I believe the other commits in PR #8075 (c0fd250 and c0fd250) are also relevant as they fix the lint message that were technically incorrect.

[Qwaz]

Maybe we need to include a few of the crates mentioned in this thread to the lintcheck crate set to prevent a similar situation in the future. Also, it would be great to have a process to run Clippy at a much larger scale than the current fixed set before stabilizing a new lint (#6623?). Thanks to the release team for handling the situation.

[xFrednet]

I believe the other commits in PR #8075 (c0fd250 and c0fd250) are also relevant as they fix the lint message that were technically incorrect.

They can be included without any risk. However, it's also fine if not, since normal users won't see the message unless they deliberately enable nursery or this lint. Thanks for pointing this out!

Maybe we need to include a few of the crates mentioned in this thread to the lintcheck crate set

We definitely should, we're also planning to improve the stabilization process of lint to prevent this in the future. Currently, I'm sadly busy with some RL stuff, but I plan to extend the lintcheck crates with a more diverse selection. Adding a few crates from this thread is a great idea 👍

[Qwaz]

In general though, I'm not sure there is a right way to do this lint. Even if there were an Rc in the struct (from the motivating example for the lint), it could be sent safely if all Rc and Weak made from the object remained within the struct and the field was created internally to the struct, which would be an invariant asserted by the author when they write the unsafe part of unsafe impl Send for.

With the standard of existing suspicious lints #, I believe a warning on Rc can be justified. In fact, I think it's even beneficial to have a Clippy warning on such field despite the manual unsafe impl in some cases.

The difficulty of implementing unsafe impl Send/Sync is that they are much more non-local than other unsafe in Rust language. This non-locality is detrimental to the maintainability. They guarantee a "type-level property" about the type structure and all the related methods, and their soundness can be affected by a safe code change far from where unsafe is written¹.

I've reported a few Send/Sync soundness bugs where unsafe impl was correct when it was written, but another commit added a new field, a new derive, or a new method and broke the previously written unsafe impl without writing the unsafe keyword. After seeing these happen, I'd be happy if Clippy can point out code regions that might affect the soundness but doesn't have the unsafe keyword, and #[allow(clippy::...)] can act as a pseudo-unsafe there that signals the programmers about their consequence in soundness.

(Just to be clear, I'm not using this argument to justify all the false positives.)

Footnotes

1. Strictly speaking, unsafe block's soundness can also be affected by safe code outside, but such construction is less common and more visible (e.g., by dataflow) than unsafe impl. ↩

[pietroalbini]

I believe the other commits in PR #8075 (c0fd250 and c0fd250) are also relevant as they fix the lint message that were technically incorrect.

Hmm, I'd prefer to backport the minimal set of changes needed to revert the breakage in the point release. As xFrednet said users won't likely see those messages anyway.

[xFrednet]

Then 5d63a28 will be sufficient. 👍

[withoutboats]

@Qwaz The problem with this lint is that it is very unlikely that a user added an unsafe impl Send and included a non Send field unintentionally. The lint does not effectively discern whether that action was unsound, and cannot give the user any guidance as to whether or not they were wrong. Since a user presumably believes their code is sound, they are very likely to ignore this lint rather than change their code, making it very ineffective.

On the other hand, a lint that only targets the wrapping case is much more likely to highlight an oversight (users meant to add the Send bound to the generic parameter, but forgot). Users will only have to allow it in the uncommon event that their type does not need the bound. This would be much more effective.

[pietroalbini]

Rust 1.58.1 moved this lint to the nursery, disabling it by default.