feat: split unsafe_code lint into lint group

[Ezrashaw]

Fixes #108926

See discussion in linked issue. This PR allows some use of the unsafe keyword where no actual unsafety (as in tangible memory issues) could occur, under the unsafe_code lint.

EDIT: This PR introduces a new lint group: unsafe_code which contains two new lints which replace unsafe_code: unsafe_obligation_discharge and unsafe_obligation_define. These split unsafe_code into code that can cause Undefined Behaviour and code which has the unsafe token but, importantly, cannot introduce Undefined Behaviour. See discussion here and on the linked issue.

[rustbot]

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

[michaelwoerister]

Thanks for the PR, @Ezrashaw!

I'd be grateful for some input from @rust-lang/lang, since this is changing the behavior of the unsafe_code lint.

[scottmcm]

I don't think we should do this right now, because making promises to other unsafe code is possible even if there's nothing unsafe in the body of a function.

The standard example being that Vec::set_len really is very unsafe even though there's nothing in the body that's at all risky on its own.

I think what the situation in the issue actually wants is https://rust-lang.github.io/rfcs/2316-safe-unsafe-trait-methods.html, so that it's then not an unsafe fn in the trait impl.

The other thing that might make this more reasonable would be some kind of unsafe struct feature, to mostly eliminate the "this looks like it's only doing safe stuff" confusion.

[Ezrashaw]

@scottmcm Right, so there are a few points here that I'll address but I'm just the implementer so cc @chbaker0:

I don't think we should do this right now, because making promises to other unsafe code is possible even if there's nothing unsafe in the body of a function.

Vec::set_len doesn't make a promise, it defines a promise which must be upheld by the caller. If somebody misuses Vec::set_len and causes Undefined Behaviour then that's on them and, crucially, not on Vec::set_len. I think what the linked issue is getting at is that there is a difference between declaring unsafe contracts and implementing them, the former being unable to cause (or be responsible for) Undefined Behaviour. In other words, unsafe fns place the burden of upholding unsafety contracts on the caller, not the callee.

I think what the situation in the issue actually wants is https://rust-lang.github.io/rfcs/2316-safe-unsafe-trait-methods.html, so that it's then not an unsafe fn in the trait impl.

Yes it does, and it gets that by also requiring unsafe_op_in_unsafe_fn to be enabled which ensures that no unsafety occurs in the function body. If that is the case, then it is exactly equivalent to the situation in the linked RFC: there is no unsafety there. The only difference is that we are checking this at the lint level rather than the language level.

The other thing that might make this more reasonable would be some kind of unsafe struct feature, to mostly eliminate the "this looks like it's only doing safe stuff" confusion.

I don't quite understand? That sounds like a whole new language feature? I'm not quite sure what structs have to do with this?

[chbaker0]

I don't think we should do this right now, because making promises to other unsafe code is possible even if there's nothing unsafe in the body of a function.

The standard example being that Vec::set_len really is very unsafe even though there's nothing in the body that's at all risky on its own.

Modules are the boundary between safety and unsafety. Vec hides its members that, if set incorrectly, could cause unsoundness. Then it provides unsafe fns like set_len to allow access to these. But it's only unsafe despite having a safe body because there's some unsafe code in the same module whose inputs it manipulates.

I think this is generally true for an unsafe fn. For there to be any potential unsoundness, it must be in the same module as an unsafe { ... } block. And, generally, if safe Rust is able to indirectly cause unsoundness outside of its module, some interface is unsoundly defined.

So, an unsafe fn with a safe body should be allowed. If a module with no unsafe blocks has an unsafe fn, with a safe body, that can cause unsoundness, the problem is elsewhere.

Also like @Ezrashaw said, unsafe fn declares a promise the caller must uphold, not a promise to the caller. The fact is, if we required all functions that may influence unsafety later to be unsafe, everything would be unsafe. Incrementing a pointer by 1 is safe even though the correctness of that operation is critical to unsafe code later on.

The other thing that might make this more reasonable would be some kind of unsafe struct feature, to mostly eliminate the "this looks like it's only doing safe stuff" confusion.

I'm not sure I understand what problem this solves. If a module internally has unsafe code but exposes a safe API, that module is asserting everything is sound and so clients don't need to know about unsafety. If a module can be used incorrectly and cause unsoundness, the relevant functions and/or traits it exposes must be unsafe.

I could only possibly see this as a way of pushing the safety assertion to a different place. Like, instead of

struct FooData {...}

unsafe fn foo(data: FooData) {...}

unsafe { foo(FooData {...}) }

you could do

unsafe struct FooData {...}

fn foo(data: FooData) {...}

foo(unsafe { FooData {...} })

So that constructing FooData is unsafe rather than calling foo. However, this is easily solved like so:

struct FooData {...}

impl FooData {
    unsafe fn new(...) -> FooData {...}
}

fn foo(data: FooData)

[chbaker0]

By the way, the unsafe fn vs unsafe body distinction is similar to the unsafe trait vs unsafe impl distinction. It seems uncontroversial to allow defining an unsafe trait in "safe Rust". What could happen? It's just defining the contract that later unsafe code might rely on. The only difference is which way the data is flowing: unsafe fn, the contract is about data going in, unsafe trait, the contract is about data coming out.

[Ezrashaw]

Exactly, well put.

To summarize: not all use of the unsafe keyword is actually unsafe (i.e. can cause memory bugs). Code that merely defines an unsafe contract, rather than consuming it is safe and should not be covered by this lint.

[workingjubilee]

Note that @rcvalle began a very long quest to implement compiler-based CFI and has made many negotiations with us about this based, in part, on someone else's paper about mere exposed interfaces. That paper had the premise that Rust-defined code that would be caught by the unsafe_code lint and that had C (actually, C++) call into it was "weakening security" due to being a mixed codebase. The complaint was that the code did not appear to have the unsafe token in sight (but it was in fact implicitly unsafe and thus would normally trigger the unsafe_code lint). Therefore:

if mixed_codebase.contains(["C", "Rust"]) {
    assert_eq!(mixed_codebase.security, Security::Unsound);
}

I have... several disagreements with that paper's premise and conclusions, but I think the essential complaint about the unsafe token is legitimate: When Rust code exposes an interface or makes a choice that could later prove unsound, even if that is still latent, the unsafe token should in fact be in sight, and that should be caught by common lints against it rather than requiring manual scanning with one's eyes. I do not think this extremely fine nuance is something we should be trying to capture with the unsafe_code lint specifically, and any such unsafe_obligation_discharge linting should be split into another lint.

In particular, it doesn't make sense to change the unsafe_code lint when we consider how a library might view the concept of unsafe code, and how such has very little to do with the perspective of an application. Currently, the unsafe_code lint can be used on a more module-by-module basis to help control where such interfaces are exposed. Removing this functionality abruptly would hinder that.

[Ezrashaw]

What about having an unafe_code lint group which is composed of unsafe_obligation_discharge and unsafe_oligation_define which trigger on consuming and defining unsafe contracts, respectively? Then by default unsafe_code could retain it's behaviour but could be fine-tuned for specific needs.

[bors]

☔ The latest upstream changes (presumably #108682) made this pull request unmergeable. Please resolve the merge conflicts.

[workingjubilee]

Yeah, I can't speak for lang much less all of T-lang but I have no objection to promoting unsafe_code to a group that covers whatever subcategories you want to slice it into. Knock yerself out.

[Ezrashaw]

Could that be done in this PR? Does it need a FCP or anything? I'm happy to implement this.

[Ezrashaw]

@workingjubilee @chbaker0 I've implemented the new lint group. Could you review please?

[michaelwoerister]

r? lang

[bors]

☔ The latest upstream changes (presumably #109503) made this pull request unmergeable. Please resolve the merge conflicts.

[Ezrashaw]

@nikomatsakis I've just seen a issue with this PR. It stops #[forbid(unsafe_code)] from working. Obviously this is a non-backwards-compatible change and its use is very common in libraries. Not sure what you want to do.

[apiraino]

Hello, visiting this PR, checking for progress cc @nikomatsakis :)

(In case, feel free to re-roll the review T-lang assignment)

[workingjubilee]

@nikomatsakis I've just seen a issue with this PR. It stops #[forbid(unsafe_code)] from working. Obviously this is a non-backwards-compatible change and its use is very common in libraries. Not sure what you want to do.

That would not be acceptable.

[Ezrashaw]

@workingjubilee Of course. I looked a bit closer and the tracking issue is #81670. TLDR is that it's a future compatibility lint because #[forbid(lint_group)] didn't use to work but will soon.

For this PR, we can probably just made an exception (which will be removed when forbidding lint groups is fully fixed). I'm happy to implement this if it's wanted (the original point of this PR remains).

[pnkfelix]

Lang team needs to weigh in on whether this should be done. (Not yet sure if it needs an FCP from T-lang, or just a champion)

@rustbot label: I-lang-nominated

[pnkfelix]

@rustbot label: +T-lang -T-compiler +S-waiting-on-team

[pnkfelix]

Also @jyn514 notes that:

this shouldn't land until forbid(lint_group) works properly

[joshtriplett]

Agreed that we cannot make this change until forbid(lint_group) works, since people extensively use forbid(unsafe_code).

[traviscross]

This issue was discussed in the lang-team meeting today. There was some sympathy for the idea that creating and discharging unsafe obligations are conceptually different, however all of those present wanted to see more motivating examples for when this distinction would usefully matter in the context of this lint.

With respect to #108926 (which seems to have motivated this PR), the feeling was that use-case may be better addressed by an implementation of the safe impls for unsafe trait methods RFC.

@Ezrashaw: What do you think?

[Ezrashaw]

@traviscross Well, firstly I'd just say that I don't have a really strong view on this.

With that in mind, I think that this issue extends to more than just trait functions; free functions have the same problem. There is still a difference between upholding and defining safety invariants (there might be better terms for this that I'm not aware of).

For example, consider the following definition: (1)

// SAFETY: The wrapped `String` must consist only of uppercase ASCII characters.
struct OnlyUppercase(String);

impl OnlyUppercase {
	// SAFETY: `OnlyUppercase`'s invariants must be upheld.
	pub unsafe fn new_unchecked(value: String) -> Self {
		Self(value)
	}
}

And the following function: (2)

fn create_only_uppercase() -> OnlyUppercase {
	// SAFETY: The provided string literal only has uppercase ASCII characters.
	unsafe { OnlyUppercase::new_unchecked("RUST".to_owned()) }
}

(note that this example will become better with unsafe fields; they might be another use-case for this change)

While both of these code snippets use the unsafe keyword (and are thus currently triggered by the lint), the purpose of the unsafe keyword is fundamentally different between these examples. In 1, the unsafe keyword is used to require that values of a type are of a certain form (i.e. that only uppercase letters may be in the value). In 2, the unsafe keyword is used to assert that that variant is being upheld.

At first glance, this point may seem to be minor. However, we should note that in the first code example there is no possibility of Undefined Behavior. Period. To be clear, 1 could rely on this invariant for safety reasons, however if Undefined Behavior were to occur, the code which failed to uphold the invariant would be at fault, not the example. In other words, use of the unsafe keyword does not necessarily imply possible Undefined Behavior; this should be recognized by the lint.

This becomes important when we consider that the first example is (could be) library code and the second user code. Libraries may not want to uphold complex invariants which could cause UB (in my experience, "no unsafety" is often in Github READMEs, etc). They expect to be able to "prove" this with rustc lints. However, this should not preclude them from "exporting unsafety" (i.e. defining unsafe invariants and interfaces) which, as noted before, cannot cause UB.

Again, I'm not anywhere near as experienced as anyone on the library team, so take this with a grain of salt.

(please note that I'm not really contributing right now, so I might take a while to respond)

[traviscross]

@Ezrashaw: Thanks for that. However, in the meeting people were struggling to come up with an example of why a library would export unsafety without also using unsafety internally.

In your example 1, for example, why does the library need the caller to uphold that invariant unless it's internally going to do something unsafe with it? (Because if it does something unsafe internally with it, then forbid(unsafe_code) is going to fire anyway.)

If you could extend that example to show how exporting that invariant might be useful (without the library using unsafety internally), that would answer the question that people had about this.

[Ezrashaw]

@traviscross Well, I think that generally it is possible to have "higher-level" invariants which aren't UB-related. For example, a lexer I wrote recently (in Rust) required input to be ASCII. It was like this because Unicode characters could muck up Spans and so on. (I have no idea if that was idiomatic but however). In general, I don't think that the str UTF-8 invariant necessarily causes insta-memory-UB when broken (I might be totally wrong though). I think that it is important to remember the definition of "invariant" here: "a contract that cannot be enforced by the compiler." Typically we think of this in the borrowck context (i.e. borrowck can't ensure that this reference doesn't alias) but the compiler also cannot ensure contracts around many things (e.g. user input, I/O, complex type stuff, etc).

Also, I'd still say that it isn't a big, big change (i.e. #[forbid(unsafe_code)] still does the same thing; it's just about providing more options if you need them) so even if the potential uses are small, it might be worth it.

[scottmcm]

I think that generally it is possible to have "higher-level" invariants which aren't UB-related.

I would be against any lints that implicitly encourage "unsafe for things that aren't actually about avoiding UB". We already have a hard time holding the line against "well how bad could it be, really?" even for things that really can lead to UB, and if we ever say "well that's unsafe but can never actually lead to UB no matter how it's used" then holding the line becomes essentially impossible.

A type can have correctness invariants, and even both new & new_unchecked constructors, without needing to mark them as unsafe. Both of the following types are entirely reasonable:

// Correctness invariant only; do not rely on in unsafe code.
pub struct Even(u32);
impl Even {
    fn new(val: u32) -> Option<Self>;
    fn new_unchecked(val: u32) -> Self;
}

// Safety: unsafe code is allowed to rely on the field being odd
pub struct Odd(u32);
impl Odd {
    fn new(val: u32) -> Option<Self>;
    unsafe fn new_unchecked(val: u32) -> Self;
}

---

a lexer I wrote recently (in Rust) required input to be ASCII

Totally unrelated to this PR, but &[ascii::Char] 🙂

[joshtriplett]

Based on Scott's analysis:

@rfcbot close

[rfcbot]

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

in the meeting people were struggling to come up with an example of why a library would export unsafety without also using unsafety internally.

The declaration of an unsafe trait could be in a quite different place than the unsafe code that makes use of the trait's contract. And after all deny(unsafe_code) might be used on a module level, not just on a crate level.

I have to admit I am quite surprised that deny(unsafe_code) rejects an unsafe trait declaration. It feels conceptually wrong. It's not a super big deal, but it certainly doesn't help get our story straight wrt. declaration of unsafe precondition vs discharging of unsafe obligations.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[joshtriplett]

Closing this per FCP.

@RalfJung You're not wrong about unsafe trait being a weird case. We discussed this in today's lang meeting, and our conclusion was "that may be a problem, but this isn't the solution we want to adopt for it".

[RalfJung]

Is "just stop denying unsafe trait under deny(unsafe_code)" an option?

[workingjubilee]

I would prefer we not to relax the lint on definitions until we have a clearer story about linting here, partly because such an exception seems like it would be very prone to accidentally allowing other unsafe definitions I may want to catch using linting.