Skip to content

Forget marker trait #3782

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

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Conversation

Ddystopia
Copy link

@Ddystopia Ddystopia commented Mar 1, 2025

Add a Forget marker trait indicating whether it is safe to skip the destructor before the value of a type exits the scope and basic utilities to work with !Forget types. Introduce a seamless migration route for the standard library and ecosystem.

Rendered

Pre-RFC thread

Unresolved questions

  • Unsafe guarantee that !Forget gives to the unsafe code is already fulfilled for all 'static types. Because of that, it doesn't really make a lot of sense to have !Forget + 'static, it is still sound to mem::forget that type. Should we force impl<T: 'static> Forget for T then? Won't that impl create some unexpected problems?

@Noratrieb Noratrieb added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. T-types Relevant to the types team, which will review and decide on the RFC. labels Mar 1, 2025
@clarfonthey
Copy link

clarfonthey commented Mar 1, 2025

This definitely seems to be a reasonably motivated RFC, although it's going to definitely be something I'll have to read through a lot more closely to fully comment on it. A few first impressions:

  1. I don't think that Forget as a name is very good for a marker. Forgetting something as a verb is fine, but as… an adjective? Not really. I think that at least Forgettable would be a better name, although something like ForgetSafe would probably be more in line for this trait, in line with UnwindSafe.
  2. While I appreciate the effort to be as accessible as possible by explaining everything, including concepts like RAII guards which not everyone might be familiar with, your desire to explain everything in advance of using them means that the RFC itself is difficult to follow. For example, the motivation section doesn't really get into the actual motivation until it provides multiple code blocks and examples. This makes the entire thing difficult to follow, since I can't easily look at the top of the section and understand what it's about. Chronological order is good for explaining the prior art, but not for the motivation, IMHO.
  3. On that note, I don't think that the current guide-level explanation is very good for a guide-level explanation. It feels very structured like an FAQ, which I don't think is good for the guide level: you're explaining to someone what the feature is outright, and not just answering their questions about it.

Again, I do want to go through this a bit more closely before fully commenting on it, but I think that you definitely need to go back and make the primary motivation for this crystal clear: because of forget, invariants in types cannot be violated by borrowing wrappers. This was not a problem before async code, because you could effectively ensure that function calls happen to completion (minus poisoning semantics) and nothing is ever left in an invalid state. However, with async code, you now have the issue that you can "forget" to finish part of a full function call via forgetting its Future, which leaves things in an invalid state.

Generally, the only way to deal with this is to make these calls unsafe and tell people to pinky-promise they run the future to completion, but it would be nice to be able to have a safe way to do this instead.

@kennytm
Copy link
Member

kennytm commented Mar 2, 2025

@clarfonthey We do use verbs as trait name when the only purpose of the trait is to support that verb, think of Send or Copy or Borrow or std::iter::Extend.

@kornelski
Copy link
Contributor

This is definitely a much desired feature that would enable new safe design patterns in Rust.

Having a gradual migration path with default_generic_bounds sounds reassuring.

I appreciate all the links to the prior art.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

The core goal of `Forget` trait, as proposed in that RFC, is to bring back the "Proxy Guard" idiom for non-static types, with `async` being the primary motivation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that all !Forget types need to have a lifetime bound to have any effect? Does it ever make sense to have a !Forget type that has no lifetime? (like an integer)

Copy link
Contributor

@Jules-Bertholet Jules-Bertholet Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s correct. Non-Forget types must be dropped before their lifetime ends

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the perspective of use cases and definitions, it really makes little sense to have !Forget and 'static together. But I am not opposed too, it may have some other benefits which I am not aware of / or implementational / migrational benefits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder whether PhantomNotForget should have a generic lifetime argument. Although it's not technically necessary, it would make it clearer that it's about lifetimes. I could work as self-documenting code which lifetime is the tricky one, and PhantomNotForget<'static> would be something to lint against.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also think so, but I fear some decision makers may not like it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were to be a blanket impl<T: 'static> Forget for T, then the lifetime parameter would be necessary for the type to work at all. I think that alone justifies it. IIUC the lifetime parameter would have to be covariant

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would mention it just in case, this is already in unresolved questions:

Maybe force impl Forget for T where T: 'static {} and add a generic to the PhantomNonForget? Use cases and unsafe guarantee are fine with it, and we already allow !Forget in static.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder whether PhantomNotForget should have a generic lifetime argument. Although it's not technically necessary, it would make it clearer that it's about lifetimes. I could work as self-documenting code which lifetime is the tricky one, and PhantomNotForget<'static> would be something to lint against.

In my (not new) reference implementation of the Forget trait, I have added it on a wrapper type Unforget as a contravariant lifetime:

https://zetanumbers.github.io/leak-playground/leak_playground_std/marker/struct.Unforget.html

As to the magic 'static lifetime, seems like it made a lot of contention in its prior discussion:

#t-lang > The destruction guarantee and linear types formulation

@oriongonza

This comment was marked as resolved.

@Ddystopia

This comment was marked as resolved.

@oriongonza

This comment was marked as resolved.

@Ddystopia
Copy link
Author

And RFC directly forbids Box::leak and talks about statics

I retract my comment with the correction in the typo of the RFC 😛

Please provide an example of unsoundness that exploits that loop {} can cause unsoundness.

The point has already been addressed here: #3782 (comment)

Great. Then you can find an explanation from @kornelski or me under that comment you linked.

It may be confusing + we may potentially teach `Pin` in terms of `Forget` then the other way around.
@Jules-Bertholet
Copy link
Contributor

Wouldn't be enough if the proposed Forget trait prevent memory being invalid? Then I think these assumptions are enough to solve the problems using Forget trait.

As for any T If

  1. T is 'static and Forget then It is safe to Box::leak and forget.
  2. T is 'static and !Forget then It is safe to Box::leak, but not safe to forget.
  3. T is non 'static but Forget then It is safe to Box::leak and forget.
  4. T is non 'static and !Forget then It is not safe to Box::leak or forget.

After considering this for a bit, I don’t think category 2 makes sense. If you have an owned value (on the stack or in a Box), you can always invalidate the memory simply by moving the value out to some other location. If the value has to stay in a particular location, the proper mechanism is Pin.

@storycraft
Copy link

storycraft commented Apr 3, 2025

After considering this for a bit, I don’t think category 2 makes sense. If you have an owned value (on the stack or in a Box), you can always invalidate the memory simply by moving the value out to some other location. If the value has to stay in a particular location, the proper mechanism is Pin.

If the value has to be pinned in particular place, than using Pin is correct. Maybe my first sentence was a bit misleading because I thought that moving values are not invalidating memory(place for the value still exists, although it's moved). So category 2 is more like movable and unforgettable 'static type(not sure if it would be useful or not). If it leaks, the backing memory would be still valid to use. But forgetting value on the stack deallocate memory without dropping. Pin has almost same drop guarantee too. But only for 'static type(for non 'static type references can be dangling). Forget trait prevents these cases.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a useful change, but it would be easier to understand with some tweaks to the ordering of paragraphs, and some wording clarifications.

@Ddystopia Ddystopia force-pushed the forget-marker-trait branch from e6a737b to e534b0c Compare April 3, 2025 10:11
@Ddystopia Ddystopia force-pushed the forget-marker-trait branch from 3aef99f to 073b9ec Compare April 3, 2025 10:54
@Ddystopia Ddystopia force-pushed the forget-marker-trait branch from 073b9ec to 555fbfa Compare April 3, 2025 10:54
@target-san
Copy link

AFAIR original reason why std::mem::forget was deemed safe is because you can effectively omit drop by stuffing guard object into refcounted pointer loop. Object is alive but unreachable.

How does !Forget solve that issue?

@programmerjake
Copy link
Member

programmerjake commented Jul 1, 2025

AFAIR original reason why std::mem::forget was deemed safe is because you can effectively omit drop by stuffing guard object into refcounted pointer loop. Object is alive but unreachable.

How does !Forget solve that issue?

I'd assume things like Rc<T> are changed to require T: Forget (edit: rather than requiring T: Forget for any Rc<T>, instead T: Forget is required whenever constructing a Rc<T>, unless you use a new unsafe function)

@target-san
Copy link

AFAIK there was another solution, scope-bound lifetimes. You effectively forbid object from leaving scope it was created in.

@Ddystopia
Copy link
Author

Ddystopia commented Jul 2, 2025

AFAIK there was another solution, scope-bound lifetimes. You effectively forbid object from leaving scope it was created in.

Design of the RFC should support any extensions, as you can just create your own abstractions and use unsafe constructors internally, publish as a crate etc.

@tvallotton
Copy link

Does this RFC address this issue raised by @withoutboats?

@tmccombs
Copy link

tmccombs commented Jul 10, 2025

Does this RFC address this issue raised by @withoutboats?

I don't think it has that specific issue, because it has a different backwards compatibility issue that prevents you from getting into that situation:

If crate A has a trait like:

trait A {
  fn foo<T>(t: T);
}

and crate B has an implementation of A, then if A changes the bound on T to be ?Forget, then that is a breaking change, because the impl in B would have the constraint of Forget, which is more specific.

Unless there was some mechanism to allow an impl to have more specific bounda than the trait it impls.

Would this impact any std traits?

Edit: I think this issue has some similarity to const methods in traits, and perhaps it could be solved in a similar way, once that is finalized.

@Ddystopia
Copy link
Author

Does this RFC address this issue raised by @withoutboats?

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. T-types Relevant to the types team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.