Skip to content
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

Consider moving std::error::Error to alloc #62502

Closed
thomcc opened this issue Jul 8, 2019 · 16 comments
Closed

Consider moving std::error::Error to alloc #62502

thomcc opened this issue Jul 8, 2019 · 16 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@thomcc
Copy link
Member

thomcc commented Jul 8, 2019

Now that alloc is stable, this is a somewhat visible omission from no_std contexts.

In practice, it being in std means that library crates that would otherwise be no_std (+ alloc) compatible which expose some sort of error type have to either:

  1. Skip implementing std::error::Error for their error type, giving up error ergonomics for users regardless of whether or not they care about no_std
  2. Hide the implementation of std::error::Error behind a feature, which is awkward.
    • And if you already have alloc behind a feature, now you end up with separate alloc and std features (which is the option I took in one of my crates, and is mostly why I'm filing this) but is tedious, and makes your library have a larger configuration surface than feels necessary.
  3. Ignore no_std completely (what most crates do and will probably continue to do).

Bummers, all of them.

If Error is moved to alloc, while not ideal [1], it would make things easier for the relatively common case of library crates that could otherwise be no_std + alloc, but don't want to give up error handling ergonomics.


[1]: I think having Error in core makes more sense, but IIUC it can't: #33149 (my read of this, which is probably wrong in some way, is that it's more or less because Error::downcast needs to be able to return a Box<T>)

As far as I know nothing prevents it from being in alloc (since Box is present in alloc), but I could easily be missing something important.

anyway thx for coming to my ted talk

@clarfonthey
Copy link
Contributor

I think that longer term offering it in core is best, but allowing it in alloc in the meantime is a good idea.

@Lokathor
Copy link
Contributor

Lokathor commented Jul 9, 2019

@clarfon core doesn't have Box

@hellow554
Copy link
Contributor

@clarfon core doesn't have Box

true, but Box is only used for downcast, so that could live in alloc and the rest maybe in core? :/ not sure how to do that though.

@clarfonthey
Copy link
Contributor

It would require changes to how the language works, yes. But it'd be doable long term.

@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 10, 2019
@cbeck88
Copy link

cbeck88 commented Aug 2, 2019

Hi, as a user I want to say that this would really help our projects, especially if it is adopted in serde, because it allows them to expose the same interface in std and alloc configurations

I think it (std::error not being in alloc) might also be related to problems like this: rust-random/rand#645

@taiki-e
Copy link
Member

taiki-e commented Aug 30, 2019

Moving to liballoc should be fine.

// A note about crates and the facade:
//
// Originally, the `Error` trait was defined in libcore, and the impls
// were scattered about. However, coherence objected to this
// arrangement, because to create the blanket impls for `Box` required
// knowing that `&str: !Error`, and we have no means to deal with that
// sort of conflict just now. Therefore, for the time being, we have
// moved the `Error` trait into libstd. As we evolve a sol'n to the
// coherence challenge (e.g., specialization, neg impls, etc) we can
// reconsider what crate these items belong in.

@Mark-Simulacrum
Copy link
Member

Given Error::backtrace and Backtrace, it's pretty clear that doing this would need something akin to an RFC -- we need to decide how to expose Backtrace to no_std users: an opaque blob, always an empty backtrace, or something else?

Given that, closing.

@cbeck88
Copy link

cbeck88 commented Feb 16, 2020

@Mark-Simulacrum it was suggested in #64017 that it can be a zero-width type, presumably with no contents

@Mark-Simulacrum
Copy link
Member

It's possible that's the right decision. I don't think it answers all the questions. I would suggest opening a PR to this repository after implementing this; if you're interested in discussing how to do so I believe internals is the right place for that.

@cbeck88
Copy link

cbeck88 commented Feb 17, 2020

@Mark-Simulacrum ok

I don't think I have bandwidth to do that right at this moment, but yeah i'm probably interested

If you think an RFC is the right way to go I'm also potentially interested in driving that process or participating in it, though I've never done that before

@tarcieri
Copy link
Contributor

Doesn't the backtrace crate support no_std?

https://github.com/rust-lang/backtrace-rs/blob/3804dac/src/lib.rs#L67

@shepmaster
Copy link
Member

Doesn't the backtrace crate support no_std?

I've been told

backtrace-rs could in theory be no_std and in fact std is an optional feature, although I suspect all current backends require std (maybe the goblin/addr2line backend doesn't ?)

@tarcieri
Copy link
Contributor

I suspect all current backends require std (maybe the goblin/addr2line backend doesn't ?)

@shepmaster well, regardless of whether it's possible to capture a backtrace or not (that's something that can always be fixed in the future), it'd be nice to be able to use its Backtrace type in no_std + alloc environments, which would solve the aforementioned issue:

we need to decide how to expose Backtrace to no_std users: an opaque blob, always an empty backtrace, or something else?

Unfortunately I just confirmed one problem: the Backtrace type is presently gated on the std feature, ostensibly because it uses Vec. So, it seems, backtrace also needs an alloc feature as well to be able to use Vec in these environments.

That said, it seems like with a little work it should be possible to support the Backtrace type in no_std + alloc environments.

@yaahc
Copy link
Member

yaahc commented May 24, 2021

reconsidering attempting this

related convo: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/error.20in.20core

@Enselic
Copy link
Member

Enselic commented Dec 17, 2023

Triage: Error was moved to core in #99917 (🎉 ). Closing this as fixed.

@Enselic Enselic closed this as completed Dec 17, 2023
@daxpedda
Copy link
Contributor

See #103765 for actually stabilizing the move.

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

Successfully merging a pull request may close this issue.