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

Finalize the error type for `try_reserve` #61780

Merged
merged 3 commits into from Aug 16, 2019

Conversation

@SimonSapin
Copy link
Contributor

commented Jun 12, 2019

See tracking issue comments from #48043 (comment).

It is now:

/// The error type for `try_reserve` methods.
#[derive(Clone, PartialEq, Eq, Debug)]
#[unstable(feature = "try_reserve", reason = "new API", issue="48043")]
pub enum TryReserveError {
    /// Error due to the computed capacity exceeding the collection's maximum
    /// (usually `isize::MAX` bytes).
    CapacityOverflow,

    /// The memory allocator returned an error
    AllocError {
        /// The layout of allocation request that failed
        layout: Layout,

        #[doc(hidden)]
        #[unstable(feature = "container_error_extra", issue = "0", reason = "\
            Enable exposing the allocator’s custom error value \
            if an associated type is added in the future: \
            https://github.com/rust-lang/wg-allocators/issues/23")]
        non_exhaustive: (),
    },
}

#[unstable(feature = "try_reserve", reason = "new API", issue="48043")]
impl From<LayoutErr> for TryReserveError {
    #[inline]
    fn from(_: LayoutErr) -> Self {
        TryReserveError::CapacityOverflow
    }
}

Changes:

  • A Layout is included. Firefox wants to log the size of failed allocations. If this were not part of the return value of e.g. HashMap::try_reserve, users would only be able to estimate based on HashMap::capacity and assumptions about the allocation strategy of HashMap.

  • There’s a dummy field that can stay unstable when try_reserve and the rest of this enum are stabilized. This forces non-exhaustive matching (#44109 is not implemented yet for variants) and allows adding another field in the future if we want to expose custom error values from the allocator. See rust-lang/wg-allocators#23.

    • If the Alloc trait is stabilized without an associated error type and with a zero-size AllocErr type, we can simply remove this dummy field.
    • If an associated type is added, we can add a default type parameter to ContainerError and a generic field to the AllocError variant.
  • Moved from the collections module to the alloc module, and replaced Collection in the enum name with Container. The wold collection implies a multiplicity of items which is not relevant to this type. For example we may want to use this error type in a future Box::try_new method.

    • Renamed to TryReserveError, after the methods that involve this type: #61780 (comment)
  • Replaced Err with Error in the enum and variant names. There is more precedent for this in https://doc.rust-lang.org/std/error/trait.Error.html#implementors, AllocErr and LayoutErr are the odd ones.

  • Dropped Alloc in the enum name. ContainerAllocError with a mouthful, and being in the alloc module already provides the same indication.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

r? @Kimundi

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

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

(#[non_exhaustive] is no implemented yet on enum variants)

I thought @davidtwco implemented this?

@SimonSapin

This comment has been minimized.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

At the libs team triage meeting, Alex pointed out that if we extend the try_ pattern to other methods, some of those will want different error types. For example, Vec::try_push likely will return the pushed value as part of the error when pushing fails.

Therefore, rather than try to anticipate future usage, the conservative choice is to pick a name corresponding to the only methods that currently use this type. I’ll amend this PR to use enum TryReserveError {…}.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from 24d7017 to 5939a45 Jun 12, 2019

@davidtwco

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2b1b4edc0b7658a9e9f9a5e872226b08 compiles without a warning.

@SimonSapin I’m not sure there should be a warning for that example. #[non_exhaustive] only affects downstream users of a item (in other crates - unfortunately, this means that it can’t be demonstrated on the playground). #[non_exhaustive] should be working for enum variants, if you’ve found a case where it isn’t working as expected, I’d be interested in knowing more so I can resolve that.

@Gankra

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Couldn't/wouldn't Vec::try_push be:

fn try_push(&mut self, val: T) -> Result<(), (T, CollectionAllocErr)>`
@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@davidtwco What does “downstream” mean exactly? Moving the enum definition didn’t change anything. Is it only in other crates?

Does #[non_exhaustive] also prevent users from constructing a value of that enum variant? We’d want this for users outside of the standard library, but then we’d need an unstable constructor for use in std. (The type is defined in the alloc crate.)

@davidtwco

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

What does “downstream” mean exactly? Moving the enum definition didn’t change anything. Is it only in other crates?

Sorry, I should have been more clear. I meant other crates. Within the defining crate, the privacy of the item (and any constructors) is unchanged.

Does #[non_exhaustive] also prevent users from constructing a value of that enum variant?

Yes, #[non_exhaustive] prevents the user from constructing a value of the enum variant (to do so would require providing a value for all the fields, which would fail to compile if new fields were added, and is thus disallowed).

We’d want this for users outside of the standard library, but then we’d need an unstable constructor for use in std. (The type is defined in the alloc crate.)

I don’t know if there’s a great solution to this. So long as the external interface of alloc is only accessible through std re-exports (I’m not familiar enough with std and alloc to know if this is true), you could define your own free constructor function that would be exported for std to use but not re-exported from std. Outside of that, I’m not sure, someone else might have a solution to this.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

While I disagree with the title of the PR :) it is forwards-compatable with what I want, and I do like adding the Layout.

I also agree with @gankro that we can probably make this fit the needs of other methods, but that can be worked out later.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@gankro Maybe. Or maybe some other named type that can more easily have From impls, for the ? operator. shrugs.

The TryReserveError name is motivated by not blocking try_reserve on figuring that out. It’ll still be possible to make it a deprecated alias later.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Oh another issue is whether CapacityOverflow should be possible when allocation failures abort.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

☔️ The latest upstream changes (presumably #61771) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from 5939a45 to f0d68e4 Jun 18, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

☔️ The latest upstream changes (presumably #62100) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from f0d68e4 to b843004 Jul 5, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

IIRC we talked about this awhile back in a libs triage meeting and I have a feeling I said the same thing I'm going to say now, but we may have also decided to not do this so if it's the case please let me know! I feel though that this may be slightly more conservative without losing much ergonomics by making TryReserveError an opaque struct? We could have all the accessors necessary to determine what the error actually happened for but it would allow us to pick our own encoding of the internals if necessary

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

☔️ The latest upstream changes (presumably #62733) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from b843004 to 35cc20f Jul 18, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I tried it and the ergonomics were not great.

We can make an opaque struct if you think that’s important, but I feel that it’s only being maximally conservative out of principle for dubious benefits. The semantics of try_reserve methods are such that I don’t see how this error could gain a new variant, or how the CapacityOverflow variant could gain a field. This PR does already plan for adding field(s) to the AllocError variant.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Sorry I'm running shorter nowadays on time than I previously though. @sfackler would you be willing to do the final review for this?

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

☔️ The latest upstream changes (presumably #60340) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from 35cc20f to 94051db Aug 1, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

☔️ The latest upstream changes (presumably #61393) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from 94051db to abc4f25 Aug 2, 2019

@gagan0723

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Ping from triage, @sfackler would you be willing to review this?
If not can we have someone else to review this? Thanks.

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Ping from triage, requesting a review from @rust-lang/libs

@Amanieu

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

📌 Commit abc4f25 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout container-error (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self container-error --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/libstd/lib.rs
Auto-merging src/liballoc/tests/vec_deque.rs
Auto-merging src/liballoc/raw_vec.rs
Auto-merging src/liballoc/lib.rs
Auto-merging src/liballoc/collections/vec_deque.rs
Auto-merging Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

SimonSapin added some commits Jun 12, 2019

Add the Layout of the failed allocation to TryReserveError::AllocError
… and add a separately-unstable field to force non-exhaustive matching
(`#[non_exhaustive]` is no implemented yet on enum variants)
so that we have the option to later expose the allocator’s error value.

CC rust-lang/wg-allocators#23

@SimonSapin SimonSapin force-pushed the SimonSapin:container-error branch from abc4f25 to 59a3409 Aug 16, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Cargo.lock conflict automatically resolved by kdiff3

@bors r=Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

📌 Commit 59a3409 has been approved by Amanieu

Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019

Rollup merge of rust-lang#61780 - SimonSapin:container-error, r=Amanieu
Finalize the error type for `try_reserve`

See tracking issue comments from rust-lang#48043 (comment).

It is now:

```rust
/// The error type for `try_reserve` methods.
#[derive(Clone, PartialEq, Eq, Debug)]
#[unstable(feature = "try_reserve", reason = "new API", issue="48043")]
pub enum TryReserveError {
    /// Error due to the computed capacity exceeding the collection's maximum
    /// (usually `isize::MAX` bytes).
    CapacityOverflow,

    /// The memory allocator returned an error
    AllocError {
        /// The layout of allocation request that failed
        layout: Layout,

        #[doc(hidden)]
        #[unstable(feature = "container_error_extra", issue = "0", reason = "\
            Enable exposing the allocator’s custom error value \
            if an associated type is added in the future: \
            rust-lang/wg-allocators#23")]
        non_exhaustive: (),
    },
}

#[unstable(feature = "try_reserve", reason = "new API", issue="48043")]
impl From<LayoutErr> for TryReserveError {
    #[inline]
    fn from(_: LayoutErr) -> Self {
        TryReserveError::CapacityOverflow
    }
}
```

Changes:

* A `Layout` is included. Firefox wants to log the size of failed allocations. If this were not part of the return value of e.g. `HashMap::try_reserve`, users would only be able to estimate based on `HashMap::capacity` and assumptions about the allocation strategy of `HashMap`.

* There’s a dummy field that can stay unstable when `try_reserve` and the rest of this enum are stabilized. This forces non-exhaustive matching ~(rust-lang#44109 is not implemented yet for variants)~ and allows adding another field in the future if we want to expose custom error values from the allocator. See rust-lang/wg-allocators#23.

  - If the `Alloc` trait is stabilized without an associated error type and with a zero-size `AllocErr` type, we can simply remove this dummy field.
  - If an associated type is added, we can add a default type parameter to `ContainerError` and a generic field to the `AllocError` variant.

* ~Moved from the `collections` module to the `alloc` module, and replaced `Collection` in the enum name with `Container`. The wold collection implies a multiplicity of items which is not relevant to this type. For example we may want to use this error type in a future `Box::try_new` method.~

  - Renamed to `TryReserveError`, after the methods that involve this type: rust-lang#61780 (comment)

* Replaced `Err` with `Error` in the enum and variant names. There is more precedent for this in https://doc.rust-lang.org/std/error/trait.Error.html#implementors, `AllocErr` and `LayoutErr` are the odd ones.

* ~Dropped `Alloc` in the enum name. `ContainerAllocError` with a mouthful, and being in the `alloc` module already provides the same indication.~

bors added a commit that referenced this pull request Aug 16, 2019

Auto merge of #63640 - Centril:rollup-yeb8o66, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #60492 (Add custom nth_back for Chain)
 - #61780 (Finalize the error type for `try_reserve`)
 - #63495 ( Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.)
 - #63525 (Make sure that all file loading happens via SourceMap)
 - #63595 (add sparc64-unknown-openbsd target)
 - #63604 (Some update for vxWorks)
 - #63613 (Hygienize use of built-in macros in the standard library)
 - #63632 (A couple of comment fixes.)
 - #63634 (ci: properly set the job name in CPU stats)
 - #63636 (ci: move linkcheck from mingw-2 to mingw-1)

Failed merges:

r? @ghost

@bors bors merged commit 59a3409 into rust-lang:master Aug 16, 2019

4 checks passed

pr Build #20190816.21 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.