rename Error::iter_chain() and remove Error::iter_sources() by haraldh · Pull Request #65557 · rust-lang/rust

haraldh · 2019-10-18T13:46:23Z

~~Rename~~

~~Error::iter_chain() -> Error::chained()~~
~~Error::iter_sources() -> Error::ancestors()~~
~~ErrorIter -> Chained and Ancestors~~

Tracker:
#58520

Edit:

Rename

Error::iter_chain() -> Error::chained()
ErrorIter -> Chain

So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR #65557 to only have chained and Chain.

Rationale:

Such iterators are helpful. They should better be stabilized sooner than later.
self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it.
The chosen name should be telling and reflect the fact that self is included. .chained() was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self.
The resulting iterator is named Chain because the error::Chain is what we want to have.

rust-highfive · 2019-10-18T13:46:27Z

r? @kennytm

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

kennytm · 2019-10-19T06:28:01Z

r? @withoutboats

teiesti · 2019-10-19T10:45:33Z

I'd like to suggest to entirely remove Error::chained() and stay only with Error::ancestors() because Error::chained() is essentially an Error::ancestors().skip(1). We don't need a separate API for that but we could give a hint in the documentation of Error::ancestors().

haraldh · 2019-10-19T16:42:30Z

It's the other way round, but yes, I agree that we might only want to have one.

teiesti · 2019-10-19T20:22:37Z

Oh, but then the suggested names are somehow counter-intuitive, because std::path::Path::ancestors does include self.

CryZe · 2019-10-20T07:50:00Z

Am I really confused or is everyone else? Path::ancestors includes the original, while Error::ancestors does not. I'd honestly prefer just .sources() as source is already the terminology we use nowadays for the underlying error. We already deprecated .cause(), so introducing yet another term for it is just confusing as can be seen in this thread.

haraldh · 2019-10-21T08:40:27Z

So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR #65557 to only have chained and Chain.

Rationale:

Such iterators are helpful. They should better be stabilized sooner than later.
self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it.
The chosen name should be telling and reflect the fact that self is included. .chained() was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self.
The resulting iterator is named Chain because the error::Chain is what we want to have.

CryZe · 2019-10-21T09:18:35Z

I think in the majority of the cases you only want the sources, so I'd say this is definitely catering more towards an edge case scenario where you would want the current Error as well, having a negative impact on the usual use case. But I definitely agree that it simplifies the implementation and reduces the problem to just a single iterator.

teiesti · 2019-10-22T00:12:50Z

I think in the majority of the cases you only want the sources, so I'd say this is definitely catering more towards an edge case scenario where you would want the current Error as well, having a negative impact on the usual use case.

I disagree. E.g. when displaying errors, I am usually interested in the actual error and all the causes. Thus, an iterator that does not include self may lead to much more code and subtile bugs.

One more data point about the naming: https://docs.rs/anyhow/1.0.17/anyhow/struct.Error.html#method.chain

withoutboats · 2019-10-22T09:19:37Z

I think we should go with sources as the name. None of the alternatives seem at all clearer, and we have precedent from other std APIs (Path::ancestors) for the specific confusing aspect of the name (that it includes the self type).

Naming APIs "in honor" of old libraries does not seem consistent with std's usual UX, which doesn't make reference to other crates or Rust's history, and doesn't expect the user to know anything about that. The use of "chained" here doesn't seem clear without that context.

I'm +1 on the shape of the API proposed here. If you're comfortable with the name sources @haraldh, I'd r+.

CryZe · 2019-10-22T09:30:50Z

.sources() including self, instead of just the sources is even more confusing imo. That's not what the current .iter_sources() does and it's the reason why there's a separate .iter_chain().

haraldh · 2019-10-22T09:52:57Z

.sources() including self, instead of just the sources is even more confusing imo. That's not what the current .iter_sources() does and it's the reason why there's a separate .iter_chain().

exactly this was also my thinking!!

teiesti · 2019-10-22T14:33:26Z

After thinking once again about this, I believe, the best option is error::Error::chain (without the ed) and error::Chain; including self as the first element.

Rationale:

I think we should go with sources as the name. None of the alternatives seem at all clearer, and we have precedent from other std APIs (Path::ancestors) for the specific confusing aspect of the name (that it includes the self type).

Path::ancestors was initially called Path::parents because the iterator calls Path::parent over and over. We finally renamed it because it seemed to be odd to call self a parent. Calling self some kind of zero-ancestor seemed at least a little better. I think, with sources the rationale is similar: self is not at all its own source.

Naming APIs "in honor" of old libraries does not seem consistent with std's usual UX, which doesn't make reference to other crates or Rust's history, and doesn't expect the user to know anything about that.

Although this is certainly a good approach, I think, we haven't followed it all the time, e.g. when integrating the futures crate into the standard library. But, fair enough, my wording ("in honor of error-chain") was a litte misleading, so let me try clarify what I meant: I do not think we should go with Error::chain in order to honor the error-chain crate but because people are already familiar with the concept of iterating over causal chains of errors because of error-chain and others.

I think, the question to ask is "What does the iterator represent?". My answer is: the causal chain of errors leading to self, thus Error::chain. I think, You don't need more context (e.g. the knowledge of ancient crates) to understand this.

Rename * Error::iter_chain() -> Error::chain() * ErrorIter -> Chain Removed * Error::iter_sources() according to rust-lang#58520 Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 2. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 3. The chosen name should be telling and reflect the fact that self is included. `.chain()` was chosen because the iterator iterates over the chain of errors that is somehow included in self. 4. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

haraldh · 2019-10-22T14:56:10Z

yeah, I like chain() more than chained() ... changed and force pushed.

haraldh · 2019-10-25T10:06:14Z

@withoutboats or do you insist on having Error::sources() as an ergonomic shortcut for Error::chain().skip(1) ??

faern · 2019-10-31T03:26:23Z

A separate method and iterator for sources excludingself could be added later if there is demand. But it can't be removed once stabilized. So to make any progress at all I think we should try to move forward without it.

JohnCSimon · 2019-11-09T06:48:58Z

Ping from triage
@withoutboats - is this PR ready for merge, or does this PR need more work?
cc: @tspiteri @haraldh
Thanks

Dylan-DPC-zz · 2019-11-12T22:25:35Z

r? @sfackler

haraldh · 2019-11-14T12:42:08Z

want me to tackle #58520 (comment) ?

rename Error::iter_chain() and remove Error::iter_sources() ~~Rename~~ * ~~Error::iter_chain() -> Error::chained()~~ * ~~Error::iter_sources() -> Error::ancestors()~~ * ~~ErrorIter -> Chained and Ancestors~~ according to rust-lang#58520 (comment) Tracker: rust-lang#58520 Edit: Rename * Error::iter_chain() -> Error::chained() * ErrorIter -> Chain So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR rust-lang#65557 to only have `chained` and `Chain`. Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 1. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 1. The chosen name should be telling and reflect the fact that self is included. `.chained()` was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self. 1. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

bors · 2019-11-14T17:13:43Z

⌛ Testing commit 7b9d50d with merge 1eefd5f6621fb7dbbe0e04df6070bc0a608bb6fe...

tmandry · 2019-11-14T17:44:34Z

@bors retry rolled up

bors · 2019-11-14T18:52:57Z

⌛ Testing commit 7b9d50d with merge 182ce0c4e2fbe7cf5f95e1871683de94f1d45497...

rename Error::iter_chain() and remove Error::iter_sources() ~~Rename~~ * ~~Error::iter_chain() -> Error::chained()~~ * ~~Error::iter_sources() -> Error::ancestors()~~ * ~~ErrorIter -> Chained and Ancestors~~ according to rust-lang#58520 (comment) Tracker: rust-lang#58520 Edit: Rename * Error::iter_chain() -> Error::chained() * ErrorIter -> Chain So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR rust-lang#65557 to only have `chained` and `Chain`. Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 1. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 1. The chosen name should be telling and reflect the fact that self is included. `.chained()` was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self. 1. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

tmandry · 2019-11-14T20:06:50Z

@bors retry rolled up

rename Error::iter_chain() and remove Error::iter_sources() ~~Rename~~ * ~~Error::iter_chain() -> Error::chained()~~ * ~~Error::iter_sources() -> Error::ancestors()~~ * ~~ErrorIter -> Chained and Ancestors~~ according to rust-lang#58520 (comment) Tracker: rust-lang#58520 Edit: Rename * Error::iter_chain() -> Error::chained() * ErrorIter -> Chain So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR rust-lang#65557 to only have `chained` and `Chain`. Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 1. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 1. The chosen name should be telling and reflect the fact that self is included. `.chained()` was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self. 1. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

bors · 2019-11-15T08:17:01Z

⌛ Testing commit 7b9d50d with merge 6f6f08cd026987e2f5961d1df141ef771b370956...

rename Error::iter_chain() and remove Error::iter_sources() ~~Rename~~ * ~~Error::iter_chain() -> Error::chained()~~ * ~~Error::iter_sources() -> Error::ancestors()~~ * ~~ErrorIter -> Chained and Ancestors~~ according to rust-lang#58520 (comment) Tracker: rust-lang#58520 Edit: Rename * Error::iter_chain() -> Error::chained() * ErrorIter -> Chain So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR rust-lang#65557 to only have `chained` and `Chain`. Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 1. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 1. The chosen name should be telling and reflect the fact that self is included. `.chained()` was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self. 1. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

JohnTitor · 2019-11-15T08:23:22Z

@bors retry rolled up

@ghost

Rollup of 12 pull requests Successful merges: - #65557 (rename Error::iter_chain() and remove Error::iter_sources()) - #66013 (Avoid hashing the key twice in `get_query()`.) - #66306 (Remove cannot mutate statics in initializer of another static error) - #66338 (Update mdbook.) - #66388 (Do not ICE on recovery from unmet associated type bound obligation) - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`) - #66391 (Do not ICE in `if` without `else` in `async fn`) - #66394 (Fix two OOM issues related to `ConstProp`) - #66398 (Remove some stack frames from `.async` calls) - #66410 (miri: helper methods for max values of machine's usize/isize) - #66418 (Link to tracking issue in HIR const-check error code description) - #66419 (Don't warn labels beginning with `_` on unused_labels lint) Failed merges: r? @ghost

bors · 2019-11-15T09:31:12Z

⌛ Testing commit 7b9d50d with merge 3f3b7ba169b20c03f9e014895a87f142f9e4e79c...

rename Error::iter_chain() and remove Error::iter_sources() ~~Rename~~ * ~~Error::iter_chain() -> Error::chained()~~ * ~~Error::iter_sources() -> Error::ancestors()~~ * ~~ErrorIter -> Chained and Ancestors~~ according to rust-lang#58520 (comment) Tracker: rust-lang#58520 Edit: Rename * Error::iter_chain() -> Error::chained() * ErrorIter -> Chain So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR rust-lang#65557 to only have `chained` and `Chain`. Rationale: 1. Such iterators are helpful. They should better be stabilized sooner than later. 1. self should be included. It is easy to .skip(1) it. Not including self is harmful because it is harder to add self to the iterator than to remove it. 1. The chosen name should be telling and reflect the fact that self is included. `.chained()` was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self. 1. The resulting iterator is named `Chain` because the `error::Chain` is what we want to have.

JohnTitor · 2019-11-15T09:37:18Z

@bors retry rolled up

@ghost

Rollup of 12 pull requests Successful merges: - #65557 (rename Error::iter_chain() and remove Error::iter_sources()) - #66013 (Avoid hashing the key twice in `get_query()`.) - #66306 (Remove cannot mutate statics in initializer of another static error) - #66338 (Update mdbook.) - #66388 (Do not ICE on recovery from unmet associated type bound obligation) - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`) - #66391 (Do not ICE in `if` without `else` in `async fn`) - #66398 (Remove some stack frames from `.async` calls) - #66410 (miri: helper methods for max values of machine's usize/isize) - #66418 (Link to tracking issue in HIR const-check error code description) - #66419 (Don't warn labels beginning with `_` on unused_labels lint) - #66428 (Cleanup unused function) Failed merges: r? @ghost

bors · 2019-11-15T12:57:53Z

⌛ Testing commit 7b9d50d with merge ce36ab2...

rust-highfive assigned kennytm Oct 18, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2019

haraldh mentioned this pull request Oct 18, 2019
Tracking issue for error source iterators #58520
Open

rust-highfive assigned withoutboats and unassigned kennytm Oct 19, 2019

haraldh force-pushed the error_iter_rename branch from 1ec0312 to 71599e3Compare October 21, 2019 08:40

haraldh changed the title ~~rename Error::iter_chain() and Error::iter_sources()~~rename Error::iter_chain() and remove Error::iter_sources()Oct 21, 2019

haraldh force-pushed the error_iter_rename branch from 71599e3 to 323f6a4Compare October 21, 2019 08:43

haraldh force-pushed the error_iter_rename branch from 323f6a4 to 7b9d50dCompare October 22, 2019 14:56

rust-highfive assigned sfackler and unassigned withoutboats Nov 12, 2019

tmandry mentioned this pull request Nov 14, 2019
Rollup of 8 pull requests #66417
Closed

tmandry mentioned this pull request Nov 14, 2019
Rollup of 9 pull requests #66421
Closed

JohnTitor mentioned this pull request Nov 15, 2019
Rollup of 12 pull requests #66432
Closed

JohnTitor mentioned this pull request Nov 15, 2019
Rollup of 12 pull requests #66436
Closed

JohnTitor mentioned this pull request Nov 15, 2019
Rollup of 12 pull requests #66438
Merged

bors merged commit 7b9d50d into rust-lang:masterNov 15, 2019

Uh oh!

Uh oh!

Conversation

haraldh commented Oct 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 18, 2019

Uh oh!

kennytm commented Oct 19, 2019

Uh oh!

teiesti commented Oct 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haraldh commented Oct 19, 2019

Uh oh!

teiesti commented Oct 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CryZe commented Oct 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haraldh commented Oct 21, 2019

Uh oh!

CryZe commented Oct 21, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teiesti commented Oct 22, 2019

Uh oh!

withoutboats commented Oct 22, 2019

Uh oh!

CryZe commented Oct 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haraldh commented Oct 22, 2019

Uh oh!

teiesti commented Oct 22, 2019

Uh oh!

haraldh commented Oct 22, 2019

Uh oh!

haraldh commented Oct 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faern commented Oct 31, 2019

Uh oh!

JohnCSimon commented Nov 9, 2019

Uh oh!

Dylan-DPC-zz commented Nov 12, 2019

Uh oh!

haraldh commented Nov 14, 2019

Uh oh!

bors commented Nov 14, 2019

Uh oh!

tmandry commented Nov 14, 2019

Uh oh!

bors commented Nov 14, 2019

Uh oh!

tmandry commented Nov 14, 2019

Uh oh!

bors commented Nov 15, 2019

Uh oh!

JohnTitor commented Nov 15, 2019

Uh oh!

bors commented Nov 15, 2019

Uh oh!

JohnTitor commented Nov 15, 2019

Uh oh!

bors commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

haraldh commented Oct 18, 2019•
edited
Loading

teiesti commented Oct 19, 2019•
edited
Loading

teiesti commented Oct 19, 2019•
edited
Loading

CryZe commented Oct 20, 2019•
edited
Loading

CryZe commented Oct 21, 2019•
edited
Loading

CryZe commented Oct 22, 2019•
edited
Loading

haraldh commented Oct 25, 2019•
edited
Loading