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.

Sign up for GitHub

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

Jump to bottom

Remove unwrap_none/expect_none. #1734

Closed

m-ou-se wants to merge 3 commits into rust-lang:master from m-ou-se:remove-unwrap-none

Member

m-ou-se commented Mar 4, 2021

We're not going to stabilize Option::{unwrap_none, expect_none}. (See rust-lang/rust#62633.) This removes the usage of those unstable methods from miri.


          Remove unwrap_none/expect_none.

1cec756

oli-obk reviewed

View reviewed changes

src/shims/posix/fs.rs Outdated Show resolved Hide resolved


          Use assert!(.is_none()) instead of assert_eq!(, None) where needed.

b9d7dd5

Member

RalfJung commented Mar 4, 2021

We're not going to stabilize Option::{unwrap_none, expect_none}

That's a bummer, I felt it made the code here a lot cleaner. :(

RalfJung reviewed

View reviewed changes

src/machine.rs

+                          this.memory
+                              .extra
+                              .extern_statics
+                              .insert(Symbol::intern(name), ptr.alloc_id),

Member

RalfJung Mar 4, 2021 •

edited

Loading

I think it's bad style for assertions to have important side-effects like this. Can you let-bind the result of insert?

Member

RalfJung Mar 4, 2021

Okay I should have just said "and the same everywhere else", sorry for that... but when I realized it affects basically all the lines you changed, I was already half-way through...

Member Author

m-ou-se Mar 4, 2021

Sure, I can change it. I'd be curious to hear why you think this is bad style though. Assertions with side effects are quite common in rustc. In C and C++ it's to be avoided because it's only conditionally compiled, but in Rust it's not much different than a panic!() in an if.

Member

RalfJung Mar 4, 2021

I answered that in rust-lang/rust#62633 (comment).

RalfJung reviewed

View reviewed changes

src/shims/posix/fs.rs

    
            @@ -223,7 +223,7 @@ impl<'tcx> FileHandler {
          
                          self.handles.last_key_value().map(|(fd, _)| fd.checked_add(1).unwrap()).unwrap_or(min_fd)

                      });

                      self.handles.insert(new_fd, file_handle).unwrap_none();

                      assert!(self.handles.insert(new_fd, file_handle).is_none());

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

src/shims/posix/fs.rs

    
            @@ -381,7 +381,7 @@ impl DirHandler {
          
                  fn insert_new(&mut self, read_dir: ReadDir) -> u64 {

                      let id = self.next_id;

                      self.next_id += 1;

                      self.streams.insert(id, read_dir).unwrap_none();

                      assert!(self.streams.insert(id, read_dir).is_none());

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

src/shims/tls.rs

    
            @@ -65,7 +65,7 @@ impl<'tcx> TlsData<'tcx> {
          
                  pub fn create_tls_key(&mut self, dtor: Option<ty::Instance<'tcx>>, max_size: Size) -> InterpResult<'tcx, TlsKey> {

                      let new_key = self.next_key;

                      self.next_key += 1;

                      self.keys.insert(new_key, TlsEntry { data: Default::default(), dtor }).unwrap_none();

                      assert!(self.keys.insert(new_key, TlsEntry { data: Default::default(), dtor }).is_none());

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

src/stacked_borrows.rs

    
            @@ -201,7 +201,7 @@ impl GlobalState {
          
                      self.base_ptr_ids.get(&id).copied().unwrap_or_else(|| {

                          let tag = Tag::Tagged(self.new_ptr());

                          trace!("New allocation {:?} has base tag {:?}", id, tag);

                          self.base_ptr_ids.insert(id, tag).unwrap_none();

                          assert_eq!(self.base_ptr_ids.insert(id, tag), None);

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

src/thread.rs

+                      assert_eq!(
+                          self.thread_local_alloc_ids
+                              .borrow_mut()
+                              .insert((def_id, self.active_thread), new_alloc_id),

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

src/thread.rs

-                          .unwrap_none();
+                      assert!(
+                          self.timeout_callbacks
+                              .insert(thread, TimeoutCallbackInfo { call_time, callback })

Member

RalfJung Mar 4, 2021

Same here, please add a let-binding to avoid assertions with side-effects.

RalfJung mentioned this pull request

Tracking issue for Option::expect_none(msg) and unwrap_none() rust-lang/rust#62633

Closed

3 tasks

Contributor

oli-obk commented Mar 4, 2021

Another alternative is to use map(|what| panic!("expected none, got {:?}", what)) instead of using asserts. The advantage of this is the chaining.

Member Author

m-ou-se commented Mar 4, 2021

Another alternative is to use map(|what| panic!("expected none, got {:?}", what)) instead of using asserts. The advantage of this is the chaining.

Yeah that's what I did in rustc to replace expect_none, which already had an error message. But in miri, these were all unwrap_none without any message, and I don't have enough context to come up with the right error message myself. (e.g. "allocation with id {} already exists as {}", etc.)


          Fix test.

3ae666f

Member Author

m-ou-se commented Mar 4, 2021

Putting this on hold until we figure out a better alternative for insert().unwrap_none(): rust-lang/rfcs#3092

Thanks for the feedback.

m-ou-se closed this

Member

RalfJung commented Mar 4, 2021 •

edited

Loading

Putting this on hold until we figure out a better alternative for insert().unwrap_none(): rust-lang/rfcs#3092

Thanks for opening the issue! EDIT: And PR ❤️

If the libs team wants to get rid of these functions, I certainly won't block that. I can also help figure out panic messages for .map(|_| panic!), though I have to say I view that pattern as more of a hack since the intent is only clear on second sight. But it is probably still nicer than adding new let-bindings.

m-ou-se deleted the remove-unwrap-none branch

March 8, 2021 15:56

m-ou-se mentioned this pull request

Remove unwrap_none/expect_none, take 2. #1735

Merged

bors added a commit that referenced this pull request


          Auto merge of #1735 - m-ou-se:remove-unwrap-none, r=RalfJung

413e341

Remove unwrap_none/expect_none, take 2.

This is #1734, but now with a better alternative.

This also upgrades rustc to the latest version, to be able to use the better alternative (`try_insert`).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet