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

Move error::Error from libstd to liballoc #64017

Closed
wants to merge 5 commits into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Aug 30, 2019

This moves Error trait from libstd to liballoc. When importing Error trait from liballoc, it is necessary to enable an unstable feature (As I referred to #51846, I’m making this unstable, but is it actually unnecessary?).

Unlike #51846, this is just moving the trait, so I don't think RFC is needed, but I'm not sure how this will affect #53487 (backtrace). cc @alexcrichton

Closes #62502

r? @SimonSapin

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2019
@SimonSapin
Copy link
Contributor

@rust-lang/libs, any thoughts on this?

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-30T08:56:49.2819482Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-30T08:56:49.3016976Z ##[command]git config gc.auto 0
2019-08-30T08:56:49.3107679Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-30T08:56:49.3227969Z ##[command]git config --get-all http.proxy
2019-08-30T08:56:49.3385821Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64017/merge:refs/remotes/pull/64017/merge
---
2019-08-30T09:04:02.4871867Z tidy check
2019-08-30T09:04:03.4561725Z * 578 error codes
2019-08-30T09:04:03.4561866Z * highest error code: E0733
2019-08-30T09:04:03.8402726Z * 263 features
2019-08-30T09:04:04.1208956Z tidy error: `/checkout/src/liballoc/error.rs:920` contains `#[test]`; unit tests and benchmarks must be placed into separate files or directories named `tests.rs`, `benches.rs`, `tests` or `benches`
2019-08-30T09:04:04.5599151Z some tidy checks failed
2019-08-30T09:04:04.5599761Z 
2019-08-30T09:04:04.5599761Z 
2019-08-30T09:04:04.5600574Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-30T09:04:04.5603066Z 
2019-08-30T09:04:04.5603101Z 
2019-08-30T09:04:04.5612918Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-30T09:04:04.5612997Z Build completed unsuccessfully in 0:01:35
2019-08-30T09:04:04.5612997Z Build completed unsuccessfully in 0:01:35
2019-08-30T09:04:04.5664997Z == clock drift check ==
2019-08-30T09:04:04.5677906Z   local time: Fri Aug 30 09:04:04 UTC 2019
2019-08-30T09:04:04.6520694Z   network time: Fri, 30 Aug 2019 09:04:04 GMT
2019-08-30T09:04:04.6526181Z == end clock drift check ==
2019-08-30T09:04:06.0956269Z ##[error]Bash exited with code '1'.
2019-08-30T09:04:06.0990569Z ##[section]Starting: Checkout
2019-08-30T09:04:06.0992720Z ==============================================================================
2019-08-30T09:04:06.0992807Z Task         : Get sources
2019-08-30T09:04:06.0992860Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-30T09:23:55.6866121Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-30T09:23:55.7066541Z ##[command]git config gc.auto 0
2019-08-30T09:23:55.7149561Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-30T09:23:55.7226805Z ##[command]git config --get-all http.proxy
2019-08-30T09:23:55.7382200Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64017/merge:refs/remotes/pull/64017/merge
---
2019-08-30T10:26:48.6467055Z .................................................................................................... 1500/8977
2019-08-30T10:26:54.3535257Z .................................................................................................... 1600/8977
2019-08-30T10:27:07.1393783Z ................................................i...............i................................... 1700/8977
2019-08-30T10:27:15.6138852Z .................................................................................................... 1800/8977
2019-08-30T10:27:29.7315729Z .......................................iiiii........................................................ 1900/8977
2019-08-30T10:27:40.9351548Z .................................................................................................... 2100/8977
2019-08-30T10:27:43.6022992Z .................................................................................................... 2200/8977
2019-08-30T10:27:47.8235405Z .................................................................................................... 2300/8977
2019-08-30T10:27:55.6147622Z .................................................................................................... 2400/8977
---
2019-08-30T10:30:56.0361903Z ..........................i...............i......................................................... 4700/8977
2019-08-30T10:31:08.0388806Z .................................................................................................... 4800/8977
2019-08-30T10:31:14.3603253Z .................................................................................................... 4900/8977
2019-08-30T10:31:25.4112230Z .................................................................................................... 5000/8977
2019-08-30T10:31:31.1401855Z .......ii.ii........................................................................................ 5100/8977
2019-08-30T10:31:44.9892990Z .................................................................................................... 5300/8977
2019-08-30T10:31:52.8209368Z ......................................................................i............................. 5400/8977
2019-08-30T10:32:00.1595212Z .................................................................................................... 5500/8977
2019-08-30T10:32:07.1293748Z .................................................................................................... 5600/8977
2019-08-30T10:32:07.1293748Z .................................................................................................... 5600/8977
2019-08-30T10:32:17.7297946Z ................................................................ii...i..ii...........i.............. 5700/8977
2019-08-30T10:32:43.8684302Z .................................................................................................... 5900/8977
2019-08-30T10:32:52.8545815Z .................................................................................................... 6000/8977
2019-08-30T10:32:52.8545815Z .................................................................................................... 6000/8977
2019-08-30T10:32:59.2074268Z .................................................................i..ii.............................. 6100/8977
2019-08-30T10:33:28.2175128Z .................................................................................................... 6300/8977
2019-08-30T10:33:30.3987621Z ....................i............................................................................... 6400/8977
2019-08-30T10:33:32.6228780Z ............................................................................................i....... 6500/8977
2019-08-30T10:33:35.4542788Z .................................................................................................... 6600/8977
---
2019-08-30T10:38:21.0917237Z  finished in 21.266
2019-08-30T10:38:21.0918025Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:38:21.0918276Z 
2019-08-30T10:38:21.0918494Z running 149 tests
2019-08-30T10:38:23.7713886Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/149
2019-08-30T10:38:25.7353314Z ..iiii..............i.........iii.i......ii......
2019-08-30T10:38:25.7354769Z 
2019-08-30T10:38:25.7361206Z  finished in 5.516
2019-08-30T10:38:25.7542749Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:38:25.9080531Z 
---
2019-08-30T10:38:28.5989165Z  finished in 2.209
2019-08-30T10:38:28.5989570Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:38:28.5989618Z 
2019-08-30T10:38:28.5989670Z running 9 tests
2019-08-30T10:38:28.5990023Z iiiiiiiii
2019-08-30T10:38:28.5990388Z 
2019-08-30T10:38:28.5990437Z  finished in 0.150
2019-08-30T10:38:28.5990781Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:38:28.5990825Z 
---
2019-08-30T10:38:46.6894422Z  finished in 18.538
2019-08-30T10:38:46.7255299Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:38:46.8991925Z 
2019-08-30T10:38:46.8993144Z running 123 tests
2019-08-30T10:39:11.4270724Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....ii..........iiii..........i...ii...i.......ii. 100/123
2019-08-30T10:39:16.2169241Z i.i.i......iii.i.....ii
2019-08-30T10:39:16.2169688Z 
2019-08-30T10:39:16.2175057Z  finished in 29.492
2019-08-30T10:39:16.2184760Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-08-30T10:39:16.2185166Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-08-30T10:51:58.1125522Z ---- error.rs - error::Error::cause (line 78) stdout ----
2019-08-30T10:51:58.1125904Z error: use of deprecated item 'std::error::Error::cause': replaced by Error::source, which can support downcasting
2019-08-30T10:51:58.1127116Z   --> error.rs:126:41
2019-08-30T10:51:58.1127200Z    |
2019-08-30T10:51:58.1133464Z 50 |             println!("Caused by: {}", e.cause().unwrap());
2019-08-30T10:51:58.1133648Z    |
2019-08-30T10:51:58.1133696Z note: lint level defined here
2019-08-30T10:51:58.1134341Z   --> error.rs:78:9
2019-08-30T10:51:58.1134497Z    |
---
2019-08-30T10:51:58.1135540Z 
2019-08-30T10:51:58.1254037Z error: test failed, to rerun pass '--doc'
2019-08-30T10:51:58.1274531Z 
2019-08-30T10:51:58.1274641Z 
2019-08-30T10:51:58.1275337Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "alloc" "--" "--quiet"
2019-08-30T10:51:58.1275473Z 
2019-08-30T10:51:58.1275503Z 
2019-08-30T10:51:58.1287125Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-30T10:51:58.1287618Z Build completed unsuccessfully in 1:20:51
2019-08-30T10:51:58.1287618Z Build completed unsuccessfully in 1:20:51
2019-08-30T10:51:58.1341312Z == clock drift check ==
2019-08-30T10:51:58.1358358Z   local time: Fri Aug 30 10:51:58 UTC 2019
2019-08-30T10:51:58.2205142Z   network time: Fri, 30 Aug 2019 10:51:58 GMT
2019-08-30T10:51:58.2205211Z == end clock drift check ==
2019-08-30T10:51:58.8622357Z ##[error]Bash exited with code '1'.
2019-08-30T10:51:58.8663366Z ##[section]Starting: Checkout
2019-08-30T10:51:58.8665424Z ==============================================================================
2019-08-30T10:51:58.8665489Z Task         : Get sources
2019-08-30T10:51:58.8665545Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@BurntSushi
Copy link
Member

BurntSushi commented Aug 30, 2019

It's there any reason not to do this? It otherwise seems like a big win. (And ideally, Error would be in core too, although it sounds like the downcast API makes that tricky I guess?)

@SimonSapin
Copy link
Contributor

There’s a code comment about impl coherence not allowing the trait to be in libcore:

// 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.

Having it in liballoc sounds ok to me. Maybe even doing FCP to stabilize the new location in this PR, if other team members are ok with that.

@taiki-e
Copy link
Member Author

taiki-e commented Aug 30, 2019

(I think it would be preferable if it could be stabilized in this PR, so I reverted the commit on unstable attributes.)

@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Aug 30, 2019
@Centril Centril added this to the 1.39 milestone Aug 30, 2019
@withoutboats
Copy link
Contributor

RFC 2504, which hasn't been fully implemented, intends to expose a Backtrace type in std, and a fn backtrace(&self) -> Option<&Backtrace> method on the Error trait. Moving Error to alloc would require exposing backtrace from alloc; I don't know if that's possible and desirable or not, but it needs to be considered.

@SimonSapin
Copy link
Contributor

Thank you @taiki-e for the PR. I will close it now as it unfortunately appears to be incompatible with RFC 2504 which is already accepted (even if not implemented yet).

I’m not sure whether backtrace functionality qualifies for libcore/liballoc (that is, can it always be available everywhere Rust can run, without a dependency on the environment or operating system). But even if it does, the RFC currently specifies a behavior that relies on an environment variable and std::env is not available without libstd.

Maybe we could decide to have a Backtrace type always be present but only sometimes providing useful functionality (possibly when libstd is linked?). However I feel such a proposal should go through the RFC process to amend RFC 2504.

@SimonSapin SimonSapin closed this Sep 2, 2019
@withoutboats
Copy link
Contributor

Maybe we could decide to have a Backtrace type always be present but only sometimes providing useful functionality (possibly when libstd is linked?). However I feel such a proposal should go through the RFC process to amend RFC 2504.

Hard to remember 2 years later, but I believe this is what I did to make failure no_std - the backtrace type in failure is always present, but is a ZST if not compiled with the backtrace feature (which depends on std).

However, I think the best solution is to move to a feature-based architecture so that traits in core can have methods added in std, because core/alloc/std are all the same crate under different feature flags. But that's a big project.

@cbeck88

This comment has been minimized.

@SimonSapin
Copy link
Contributor

And now at a moment when we could have moved Error to the recently stable liballoc, we decided that it was more interesting and important to integrate backtrace functionality that reads an environment variable into Error.

To clarify: I did not make a judgment of what is preferable, nor did I say that Error in liballoc could never happen. Only that since we already accepted an RFC for Error::backtrace, process-wise reverting or amending this decision requires an equivalent level of community discussion and team decision.

It makes me really sad that there's no clear way to make progress on this.

I did propose a way above. I feel that if you’re writing 1600+ words for everyone else to read, the least you can do is read the comments you’re responding to.

Why do we really need impl<'a> From<Cow<'a, str>> for Box<dyn Error>

That would have been relevant discussion when this impl was being proposed. Now that’s it’s stable we can’t simply remove it.

Error is the interface of "catch_unwind"

It’s not. This signature has no bound on the Error trait:

type Result<T> = std::result::Result<T, Box<dyn Any + Send + 'static>>;
pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {}

@withoutboats
Copy link
Contributor

withoutboats commented Sep 5, 2019

@cbeck88 I really encourage you to read in detail RFC 2504 as well the discussion on the pull request - rust-lang/rfcs#2504. Your comment evidences a confusion about some of the issues at play, and I have to be honest that I find it rather impolitely bombastic for not understanding the situation clearly.

There are two viable paths toward making Error available to no_std crates:

  • Make the backtrace type available to no_std as a ZST (like failure does).
  • Change the core/alloc/std compilation system so that we can stop dealing with coherence/local method issues like this. We have many issues of this nature and it would be great to overcome them all in one swoop.

@Centril Centril removed this from the 1.39 milestone Sep 26, 2019
@taiki-e taiki-e deleted the alloc-error branch August 24, 2022 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider moving std::error::Error to alloc
7 participants