Soft-deprecate the description() method of the Error trait #49536
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SimonSapin
added
T-libs-api
|
Your PR failed on Travis. 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.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 |
|
I think that failed travis build may have been interrupted when I force-pushed an amended commit to the PR’s branch? |
|
This change is as discussed in the libs meeting this week. |
src/libstd/error.rs
Outdated
| @@ -89,7 +75,10 @@ pub trait Error: Debug + Display { | |||
| /// } | |||
| /// ``` | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| fn description(&self) -> &str; | |||
| #[doc(hidden)] | |||
| fn description(&self) -> &str { | |||
There was a problem hiding this comment.
Should we deprecate the method as well, or are we waiting until this lands in stable channel?
There was a problem hiding this comment.
Yes we should wait until the default impl is stable if we're gonna emit a deprecation warning, but I'm not sure than doing that is worth it. We mostly want to make it unnecessary to implement this method, no need to actively remove it from existing code.
|
Your PR failed on Travis. 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.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 |
ollie27
reviewed
Mar 31, 2018
src/libstd/error.rs
Outdated
| @@ -89,7 +75,10 @@ pub trait Error: Debug + Display { | |||
| /// } | |||
| /// ``` | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| fn description(&self) -> &str; | |||
| #[doc(hidden)] | |||
There was a problem hiding this comment.
I don't think hiding this in the docs is a good idea. It's still a stable function so the docs need to include something for it. I suggest just changing the docs above instead.
There was a problem hiding this comment.
I assume you mean something like “if it exists it should be documented”. The point of this change is to try to make it as if the method did not exist anymore, except without breaking code that already implements or calls it.
There was a problem hiding this comment.
“if it exists it should be documented”
Yeah, is that controversial?
The point of this change is to try to make it as if the method did not exist anymore
You can't rewrite history. What do you expect people to do if they see this method used in code and want to find out what it does, or in fact why it's depreciated? I don't see the problem with just making it clear in the documentation that this method is deprecated and shouldn't be used any more.
There was a problem hiding this comment.
is that controversial?
No, not that part.
You can't rewrite history.
Shrug I don’t really care either way, this PR was only motivated by making sure that the outcome of last week’s libs meetings were not lost in a forgotten Dropbox Paper doc. At this point I’ll leave it to someone else in @rust-lang/libs to decide.
|
The book’s first edition has a couple links to the |
|
Possible fix in the book repo: diff --git a/first-edition/src/error-handling.md b/first-edition/src/error-handling.md
index e2f3995c..ad8690ad 100644
--- a/first-edition/src/error-handling.md
+++ b/first-edition/src/error-handling.md
@@ -1340,7 +1340,8 @@ There's one little nit left: the `Box<Error>` type is *opaque*. If we
return a `Box<Error>` to the caller, the caller can't (easily) inspect
underlying error type. The situation is certainly better than `String`
because the caller can call methods like
-[`description`](../../std/error/trait.Error.html#tymethod.description)
+[`to_string`](../../std/string/trait.ToString.html#tymethod.to_string)
+(through the [`Display`](../../std/fmt/trait.Display.html) trait)
and [`cause`](../../std/error/trait.Error.html#method.cause), but the
limitation remains: `Box<Error>` is opaque. (N.B. This isn't entirely
true because Rust does have runtime reflection, which is useful in |
|
Your PR failed on Travis. 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.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 |
dtolnay
approved these changes
Apr 2, 2018
|
@SimonSapin perhaps a comment about the |
… rather than the module’s.
It was out of date, and rustdoc already shows the same information.
|
Changed the PR so that the method stays visible in docs, but docs suggest to use |
It is redundant with Display while being much less flexible for implementors. This is only a "soft" deprecation: it is not worth the hassle of a warning to existing users.
|
@bors: r+ |
|
📌 Commit 21923d8 has been approved by |
bors
added
the
S-waiting-on-bors
|
Your PR failed on Travis. 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.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 |
|
@bors: r- |
bors
added
S-waiting-on-author
|
Your PR failed on Travis. 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.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 |
|
Your PR failed on Travis. 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.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 |
kornelski
added a commit
to kornelski/book
that referenced
this pull request
Apr 20, 2018
| /// } | ||
| /// _ => println!("No error"), | ||
| /// } | ||
| /// ``` | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| fn description(&self) -> &str; | ||
| fn description(&self) -> &str { | ||
| "" |
There was a problem hiding this comment.
Hm, could we at least make this something like "default error description" or "error"? I worry that existing code that calls this would be confusing to read output logs when description isn't implemented since println!("{}", e.description()); results in an empty line being printed -- giving no indication as to the source, making such a problem hard to debug.
There was a problem hiding this comment.
Maybe something along lines of "use Display to get the description"?
|
I’m not going to keep pushing this, anyone else feel free to pick it up. |
This was referenced Apr 21, 2018
bors
added a commit
that referenced
this pull request
Apr 30, 2018
Bury Error::description() Second attempt of #49536 rust-lang/rfcs#2230 The exact wording of the default implementation is still up in the air, but I think it's a detail that can be amended later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By providing a default impl and documenting that
Displayshould be used instead.Closes rust-lang/rfcs#2230.