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

Tracking issue for Option::expect_none(msg) and unwrap_none() #62633

Closed
1 of 3 tasks
cuviper opened this issue Jul 12, 2019 · 112 comments · Fixed by #83349
Closed
1 of 3 tasks

Tracking issue for Option::expect_none(msg) and unwrap_none() #62633

cuviper opened this issue Jul 12, 2019 · 112 comments · Fixed by #83349
Labels
A-result-option Area: Result and Option combinators B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper commented Jul 12, 2019

impl<T: fmt::Debug> Option<T> {
    pub fn expect_none(self, msg: &str);
    pub fn unwrap_none(self);
}

Steps:

@cuviper cuviper added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jul 12, 2019
@jfrimmel

This comment has been minimized.

@cuviper

This comment has been minimized.

@jfrimmel

This comment has been minimized.

@cuviper
Copy link
Member Author

cuviper commented Apr 20, 2020

I guess it could use more in-repo callers? I do see that rustc_middle and rustc_mir are using expect_none, at least. Grepping for assert.*is_none shows a lot more possibilities, but maybe some of those would not like the added code to Debug the value in the panic message.

@aclysma
Copy link

aclysma commented May 16, 2020

I'd love to see this go to stable.. I'm having to do this:

let old_value = self.values.insert(key, value);
assert!(old_value.is_none());

And I'd much rather do

self.values.insert(key, value).unwrap_none();

@dtolnay
Copy link
Member

dtolnay commented Jun 9, 2020

In the stabilization PR it was raised that &self may be more appropriate and flexible in these signatures than self. Do potential users here have a preference for either?

@RustyYato
Copy link
Contributor

You can use as_ref if you need a reference. Also, we should be consistent between our functions (Option::unwrap).

@KodrAus KodrAus added A-result-option Area: Result and Option combinators Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@aclysma
Copy link

aclysma commented Sep 20, 2020

I'd like to see unwrap_none be completely parallel to unwrap_err. Result has unwrap(self) and unwrap_err(self), so I think Some should likewise have unwrap(self) and unwrap_none(self).

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Sep 29, 2020
cc rust-lang#62633

These methods have been on nightly for over a year without any issues.
@naiveai
Copy link

naiveai commented Dec 9, 2020

@aclysma what would unwrap_none return? Result::unwrap_err exists because the Err type still contains a value. But None doesn't.

@aclysma
Copy link

aclysma commented Dec 9, 2020

unwrap_none would return nothing if it is None and panic if it is Some. Here is a practical example of how I would like to use it:

EDIT: Oops! I didn't realize I already had this example above: #62633 (comment) (More examples here: https://github.com/aclysma/rafx/search?q=old.is_none)

@cuviper
Copy link
Member Author

cuviper commented Dec 9, 2020

It could be called assert_none, since there's nothing to return, but calling it unwrap_none is nice to mirror Result.

@douglascaetano
Copy link

My two cents on this topic after finding myself wanting to use these APIs:

Besides #73077 (comment), I do believe there is value to add unwrap_none and expect_none methods. I do agree with the argument that we should avoid adding new methods to std, but these seem pretty natural to me.

As for the name of the methods, I believe unwrap_none is better than assert_none because it parallels with Result::unwrap_err and pairs with Option::unwrap. It is not an absolute incoherence to "unwrap" a None to a () (it seems quite logical, actually). Also, one already knows unwrap and expect methods, so it is a smaller cognitive burden to find (and memorize) all other similar methods, and we wouldn't open a can of worms by creating a new "group" of assert methods.

That said, I would either stick with unwrap_none and expect_none or remove these methods altogether.

@John-Nagle
Copy link

I'd argue for expect_none and unwrap_none as standard. Having them as optional "features" is a strange limbo state. That creates confusion as to whether they're staying around or going away.

Using "assert!" around an expression which must be executed for its side effects seems poor form.

My main use case for this is for collection "insert" operations where failure is not expected and should be fatal. It's tempting to write

let _ = somehash.insert(k,v);

but it's better to check. A concise syntax for checking is a good thing in this situation.

@KodrAus
Copy link
Contributor

KodrAus commented Feb 10, 2021

@rfcbot fcp close

We discussed this in the Libs meeting and decided against stabilizing these methods in favor of assert!(option.is_none()).

The reasoning was that all this method can do is either succeed with a () or panic, so is an assert, but that's already handled by the assert! macro.

@rfcbot
Copy link

rfcbot commented Feb 10, 2021

Team member @KodrAus has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Feb 10, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 5, 2021
Add {BTreeMap,HashMap}::try_insert

`{BTreeMap,HashMap}::insert(key, new_val)` returns `Some(old_val)` if the key was already in the map. It's often useful to assert no duplicate values are inserted.

We experimented with `map.insert(key, val).unwrap_none()` (rust-lang#62633), but decided that that's not the kind of method we'd like to have on `Option`s.

`insert` always succeeds because it replaces the old value if it exists. One could argue that `insert()` is never the right method for panicking on duplicates, since already handles that case by replacing the value, only allowing you to panic after that already happened.

This PR adds a `try_insert` method that instead returns a `Result::Err` when the key already exists. This error contains both the `OccupiedEntry` and the value that was supposed to be inserted. This means that unwrapping that result gives more context:
```rust
    map.insert(10, "world").unwrap_none();
    // thread 'main' panicked at 'called `Option::unwrap_none()` on a `Some` value: "hello"', src/main.rs:8:29
```

```rust
    map.try_insert(10, "world").unwrap();
    // thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
    // OccupiedError { key: 10, old_value: "hello", new_value: "world" }', src/main.rs:6:33
```

It also allows handling the failure in any other way, as you have full access to the `OccupiedEntry` and the value.

`try_insert` returns a reference to the value in case of success, making it an alternative to `.entry(key).or_insert(value)`.

r? ``@Amanieu``

Fixes rust-lang/rfcs#3092
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 5, 2021
Add assert_matches macro.

This adds `assert_matches!(expression, pattern)`.

Unlike the other asserts, this one ~~consumes the expression~~ may consume the expression, to be able to match the pattern. (It could add a `&` implicitly, but that's noticable in the pattern, and will make a consuming guard impossible.)

See rust-lang#62633 (comment)

This re-uses the same `left: .. right: ..` output as the `assert_eq` and `assert_ne` macros, but with the pattern as the right part:

assert_eq:
```
assertion failed: `(left == right)`
  left: `Some("asdf")`,
 right: `None`
```
assert_matches:
```
assertion failed: `(left matches right)`
  left: `Ok("asdf")`,
 right: `Err(_)`
```

cc ``@cuviper``
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 5, 2021
Add {BTreeMap,HashMap}::try_insert

`{BTreeMap,HashMap}::insert(key, new_val)` returns `Some(old_val)` if the key was already in the map. It's often useful to assert no duplicate values are inserted.

We experimented with `map.insert(key, val).unwrap_none()` (rust-lang#62633), but decided that that's not the kind of method we'd like to have on `Option`s.

`insert` always succeeds because it replaces the old value if it exists. One could argue that `insert()` is never the right method for panicking on duplicates, since already handles that case by replacing the value, only allowing you to panic after that already happened.

This PR adds a `try_insert` method that instead returns a `Result::Err` when the key already exists. This error contains both the `OccupiedEntry` and the value that was supposed to be inserted. This means that unwrapping that result gives more context:
```rust
    map.insert(10, "world").unwrap_none();
    // thread 'main' panicked at 'called `Option::unwrap_none()` on a `Some` value: "hello"', src/main.rs:8:29
```

```rust
    map.try_insert(10, "world").unwrap();
    // thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
    // OccupiedError { key: 10, old_value: "hello", new_value: "world" }', src/main.rs:6:33
```

It also allows handling the failure in any other way, as you have full access to the `OccupiedEntry` and the value.

`try_insert` returns a reference to the value in case of success, making it an alternative to `.entry(key).or_insert(value)`.

r? ```@Amanieu```

Fixes rust-lang/rfcs#3092
m-ou-se added a commit to m-ou-se/rust that referenced this issue Mar 5, 2021
Add assert_matches macro.

This adds `assert_matches!(expression, pattern)`.

Unlike the other asserts, this one ~~consumes the expression~~ may consume the expression, to be able to match the pattern. (It could add a `&` implicitly, but that's noticable in the pattern, and will make a consuming guard impossible.)

See rust-lang#62633 (comment)

This re-uses the same `left: .. right: ..` output as the `assert_eq` and `assert_ne` macros, but with the pattern as the right part:

assert_eq:
```
assertion failed: `(left == right)`
  left: `Some("asdf")`,
 right: `None`
```
assert_matches:
```
assertion failed: `(left matches right)`
  left: `Ok("asdf")`,
 right: `Err(_)`
```

cc ```@cuviper```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 18, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 18, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 19, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 19, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 19, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 19, 2021
…chenkov

Remove unwrap_none/expect_none from compiler/.

We're not going to stabilize `Option::{unwrap_none, expect_none}`. (See rust-lang#62633.) This removes the usage of those unstable methods from `compiler/`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 21, 2021
Remove Option::{unwrap_none, expect_none}.

This removes `Option::unwrap_none` and `Option::expect_none` since we're not going to stabilize them, see rust-lang#62633.

Closes rust-lang#62633
@bors bors closed this as completed in 29e64e9 Mar 25, 2021
@Bendrien
Copy link

Bendrien commented Apr 3, 2021

I hope it is fine that im writing here. I just stumbled over this api and was wondering about the Debug constrain. I want to use expect_none on a generic option but dont care about the value itself, I just want to give some context msg why a None is expected. Since this is still an unstable feature I thought it makes sense to share my opinion with you.

@RalfJung
Copy link
Member

RalfJung commented Apr 3, 2021

@Bendrien expect_none was removed recently, see #83349 and the comments in this thread.

@mankinskin
Copy link

My god, what is the harm in adding unwrap_none and expect_none(msg)? So many people are asking for this very API because they find it useful.

People here seem to completely miss the idea behind using it. It is obviously not about unwrapping to get some value, but about panicking when it is not none. maybe "unwrap_none" sounds a bit odd, but "expect_none" is very clear. Also "unwrap_none" makes sense when the wrapper is empty.

Having to wrap an Option in an assert increases code complexity when you are mostly using functional adapters which chain behind oneanother.

@RalfJung
Copy link
Member

RalfJung commented Nov 24, 2022

The reasoning was that all this method can do is either succeed with a () or panic, so is an assert, but that's already handled by the assert! macro.

This makes me once again wish that unwrap was called assert or assert_some, because every unwrap is an assertion. As assertion that also returns a value, but an assertion no less. (Making things that usually don't return something, into expressions that return something, is a common thing Rust does, so assertions that return values should not be surprising.) Then we would call this here assert_none and nobody would be surprised by it. Right now we are in the awkward situation where some assertions on Option can be written with nice method call syntax, but others cannot.

And yeah, I have run into the same iterator usecase as @hikari-no-yume and used to use expect_none there, until it got removed, which made me a bit sad.

@hikari-no-yume
Copy link

Sorry, I deleted the comment you're referencing (where I mentioned it would be nice to be able to do args.next().expect_none("There shouldn't be extra arguments")) since I felt my contribution wasn't valuable enough to be worth essentially re-opening discussion.

I saw the wisdom of the lib team's argument when I noticed I'd also then want expect_ for some other types, and yeah, I could just do assert!(args.next().is_none); in that case.

@mankinskin
Copy link

mankinskin commented Nov 24, 2022

I could just do assert!(args.next().is_none); in that case.

which obviously uses way more characters than args.next().unwrap_none() and requires you to bounce back to process the logic of the line. I just don't see the value of not having a method like this. I don't find it misleading or anything, there are always different ways to do things, doesn't mean you don't want to take shortest paths. Not having this just makes using the language harder for no reason.

@aclysma
Copy link

aclysma commented Nov 24, 2022

I’m still doing this, over and over.

let old_value = self.values.insert(key, value);
assert!(old_value.is_none());

The libs team implemented an alternative in response to it #82764

18 months later, it hasn’t moved. So I’m stuck here doing this over and over.

(Additionally, I’m still disappointed at how this FCP was handled. Not just the result, but the way in which the libs team executed the process/engaged with the community)

@SOF3
Copy link
Contributor

SOF3 commented Nov 25, 2022

The problem is that this issue is fundamentally an XY problem, where the real problem is the lack of postfix macros but we tried to solve this problem by bloating the standard library instead.

@RalfJung
Copy link
Member

RalfJung commented Nov 25, 2022 via email

@SOF3
Copy link
Contributor

SOF3 commented Nov 25, 2022

@RalfJung yes, I think unwrap and expect are bloat as well. They should have simply been

value.expect!()
value.expect!("expect message")
value.expect!("formatted {expect} {}", message)

eggyal pushed a commit to eggyal/copse that referenced this issue Jan 9, 2023
Add {BTreeMap,HashMap}::try_insert

`{BTreeMap,HashMap}::insert(key, new_val)` returns `Some(old_val)` if the key was already in the map. It's often useful to assert no duplicate values are inserted.

We experimented with `map.insert(key, val).unwrap_none()` (rust-lang/rust#62633), but decided that that's not the kind of method we'd like to have on `Option`s.

`insert` always succeeds because it replaces the old value if it exists. One could argue that `insert()` is never the right method for panicking on duplicates, since already handles that case by replacing the value, only allowing you to panic after that already happened.

This PR adds a `try_insert` method that instead returns a `Result::Err` when the key already exists. This error contains both the `OccupiedEntry` and the value that was supposed to be inserted. This means that unwrapping that result gives more context:
```rust
    map.insert(10, "world").unwrap_none();
    // thread 'main' panicked at 'called `Option::unwrap_none()` on a `Some` value: "hello"', src/main.rs:8:29
```

```rust
    map.try_insert(10, "world").unwrap();
    // thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
    // OccupiedError { key: 10, old_value: "hello", new_value: "world" }', src/main.rs:6:33
```

It also allows handling the failure in any other way, as you have full access to the `OccupiedEntry` and the value.

`try_insert` returns a reference to the value in case of success, making it an alternative to `.entry(key).or_insert(value)`.

r? ```@Amanieu```

Fixes rust-lang/rfcs#3092
@semiviral
Copy link

  • unwrap_none is not the right name, as there's nothing inside a None to unwrap.

This statement doesn't really make any sense. You aren't unwrapping the None, you're unwrapping the Option, which may or may not contain Something. Unless the libs team or someone else explicitly specifies what 'unwrap' to be consistent with that argument, the 🎁 analogy used earlier holds: unwrapping a present and finding nothing was still unwraping the present.

  • As a method that only asserts something, it doesn't fit any existing pattern. It'd be a new pattern that would also fit things like i32::assert_positive(), Vec::assert_empty(), bool::expect_true(), f64::assert_finite(), etc. This is not a direction we want to pursue. (Or at least not without an RFC.)

I'm skeptical of this at its face value. The notion of unwrap_ is fundamentally the concept of "unwrap this container, expect this valuedness, then return what was inside":

  • unwrap() -> unwrap and expect Some()/Ok(), and return the value
  • unwrap _err() -> unwrap Err(), and return the value
  • unwrap_none() -> unwrap None, and return the value (in this case, nothing, which has a value in Rust!)
  • assert_eq!(option, None); already shows the value inside the Some in the panic message. (Note that unlike in C and C++, asserts in Rust are unconditional and also included in release builds.)

This is simply not ergonomically consistent with how one expects to assert the valuedness of the container enums. Unless there are plans to deprecate the panicking unwrap_ APIs, then this semantic inconsistency will continue to draw new users to this issue, only to be disappointed by unclear and inconsistent logic from the rust teams.

During our discussion, the idea of an Option::err_or was brought up. In examples where panicking on a Some makes sense, the Some is considered an error. So, a function that would convert an Option::Some to a Result::Err might make sense in some of those cases, such that that Result can then be handled in the usual ways, such as by unwrapping it. We didn't discuss this option in detail, so there's no concensus on this yet, but it might be an interesting direction to explore separately from unwrap_none/expect_none.

This seems like skirting around the reality that the expectation of a semantic way to inline-assert the valuedness of a container has already been proposed in this issue. You certainly can introduce the function discussed in this thread as an entirely new unwrapping API separate from the existing unwrapping APIs, but it feels like arbitrarily beating around the bush of this issue. Really, this is functionally another inline syntax for Option::ok_or().unwrap_err().

Overall, reading this, I wasn't particularly satisfied with the responses, justifications, or in general the way this issue was handled. There was next to no communication while the issue was being reviewed, followed by subjective arguments, followed (finally) by an actual discussion of the logical reasoning of the libs team.

It really just seems like the team that voted on this doesn't like the idea of unwrapping an empty 🎁.

@buzmeg
Copy link

buzmeg commented Apr 17, 2023

Quoting @SOF3

The problem is that this issue is fundamentally an XY problem, where the real problem is the lack of postfix macros but we tried to solve this problem by bloating the standard library instead.

Which would be fine, except that 5 years on we don't have a solution for X OR Y. And none of the alternatives mentioned to solve this have moved even slightly over 5 years. And, if postfix macros didn't get resolved given their usefulness for "async" which everybody on the core team was totally obsessed with, they are never going to move.

So, I'm so glad you maintained your vaunted "language purity", but, in the meantime, users of the language are still stuck exactly where they started.

That's fine. Many of us can take a hint and understand that it is time to move on from Rust.

@mankinskin
Copy link

mankinskin commented Apr 17, 2023

To contrast, I find there is value in a clean code base and principled design. But I think pragmatism is one of the most important principles which should drive language design. And I personally don't really have any validated reasons on my list about why unwrap_none or expect_none would be bad to have. If there is any reason why the existence of these functions could reduce the quality of the Rust language, then maybe we should start talking about those instead of why it might be a good idea to have these. They are obviously useful, so if there are no reasons why this should be blocked it should simply be added to a library and eventually stabilized.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 18, 2023

The notion of unwrap_ is fundamentally the concept of "unwrap this container, expect this valuedness, then return what was inside"

Sure.

This statement doesn't really make any sense. You aren't unwrapping the None, you're unwrapping the Option, which may or may not contain Something.

To unwrap the Option, we already have a method: .unwrap(). In an Option::None, there is nothing to unwrap, so it panicks.

unwrap None, and return the value (in this case, nothing, which has a value in Rust!)

.unwrap() is about getting the contents of the Some out. But .unwrap_none() isn't about getting the contents of the None out, because there are none. Sure, you can represent that as (), but if you want a (), you don't need to call this method. The only reason you'd ever call .unwrap_none() is as an assertion, a conditional panic, not as a way to access a value. So, it'd basically be a .panic_if_some() or .assert_none().

So then the question is whether we want such methods. I don't think we do, because we also don't have anything like i32::assert_nonzero(), bool::panic_if_false(), or Vec::unwrap_empty(), and so on. All of those assertions you'd write using an assert macro or a panic inside an if. I don't think there is enough reason to give Option::is_none() special treatment.

So, I'm so glad you maintained your vaunted "language purity", but, in the meantime, users of the language are still stuck exactly where they started.

Many of us can take a hint and understand that it is time to move on from Rust.

I don't think Rust is very "pure". I also don't think users are "stuck". There are many simple ways to express a check for None, so I doubt this is really a blocker for anyone. Maybe there's even a crate with an extension trait to add .panic_if_…() to various common types.

If there is any reason why the existence of these functions could reduce the quality of the Rust language, them maybe we should start talking about those [..]

They are obviously useful, so if there are no reasons why this should be blocked [..]

Many reasons have been given in this thread. I'm not sure what I can add at this point without just repeating things that have already been said.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 18, 2023

Overall, reading this, I wasn't particularly satisfied with the responses, justifications, or in general the way this issue was handled. There was next to no communication while the issue was being reviewed, …

As I highlighted here, we're all just volunteers working on the standard library next to many other tasks, so while we'll try our best to communicate everything as clearly as possible, it sometimes takes longer than you might hope for.

… followed by subjective arguments, …

I'm afraid that API design will always be based on subjective arguments. There's no objective right or wrong way to design a library.

… followed (finally) by an actual discussion of the logical reasoning of the libs team.

I'm not sure what you expect from us at this point. Many arguments have been given and thoroughly explained over the past few years, and it just seems like we're going in circles now. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-result-option Area: Result and Option combinators B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. 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.