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 Ref/RefMut::map_split #51476

Open
joshlf opened this Issue Jun 10, 2018 · 22 comments

Comments

Projects
None yet
@joshlf
Copy link
Contributor

joshlf commented Jun 10, 2018

Tracking issue for Ref::map_split and RefMut::map_split (feature refcell_map_split), implemented in #51466.

Blocking stabilization

cc @RalfJung @jhjourdan

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jun 19, 2018

Can I just say how awesome it is for people to actually be willing to block something on a proof of soundness? I love this community <3

@jhjourdan

This comment has been minimized.

Copy link
Contributor

jhjourdan commented Sep 21, 2018

I have just completed the proof of soundness of this new feature:

https://gitlab.mpi-sws.org/FP/LambdaRust-coq/commit/70db5cbf1f63879ff8171218d4ba679e8e4c69a2

Note, however, that our model do not model machine integer. Therefore, it is possible that our proof would not spot an overflow in the borrow counter that would cause an unsoundness. That said, causing an overflow in the borrow counter is very hard anyway.

@joshlf

This comment has been minimized.

Copy link
Contributor Author

joshlf commented Sep 21, 2018

That's fantastic! Thanks so much for doing that! In your opinion, is integer overflow something we should be concerned about, or are you comfortable that it's not an issue even though the proof doesn't technically cover it?

@joshtriplett @dtolnay Anything left to do before proposing stabilization?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Sep 21, 2018

Given recent events, I think it's rather reasonable to want to carefully examine the assumptions that integer overflow can't happen here.

Looking over the code, it appears that several of the overflow checks occur in a debug_assert! rather than an assert!, for instance.

@jhjourdan

This comment has been minimized.

Copy link
Contributor

jhjourdan commented Sep 21, 2018

That's fantastic! Thanks so much for doing that! In your opinion, is integer overflow something we should be concerned about, or are you comfortable that it's not an issue even though the proof doesn't technically cover it?

Of course, a thorough review of the code cannot hurt. But actually, I cannot imagine a realistic scenario where this overflow would occur without causing an out of memory first (or using a lot of mem::forget calls for each of the created Ref/RefMut, which is also a weird thing to do).

@jhjourdan

This comment has been minimized.

Copy link
Contributor

jhjourdan commented Sep 21, 2018

Looking over the code, it appears that several of the overflow checks occur in a debug_assert! rather than an assert!, for instance.

What overflow check are you speaking about? Reading the current state of the code, all the overflow checks are either is_reading(...) or is_writing(...) or self.borrow.get() == UNUSED, none of which check for overflow.

As far as I can tell, each incrementation or decrementation of the counter in this code is either guarded by an overflow check or cannot overflow because of the current sate of the counter.

@joshlf

This comment has been minimized.

Copy link
Contributor Author

joshlf commented Sep 21, 2018

That's fantastic! Thanks so much for doing that! In your opinion, is integer overflow something we should be concerned about, or are you comfortable that it's not an issue even though the proof doesn't technically cover it?

Of course, a thorough review of the code cannot hurt. But actually, I cannot imagine a realistic scenario where this overflow would occur without causing an out of memory first (or using a lot of mem::forget calls for each of the created Ref/RefMut, which is also a weird thing to do).

Unsoundness is unsoundness, regardless of how pathological the program would have to be :)

But regardless, I don't think it's an issue. All overflow is checked by assert!s. The debug_assert!s only check for conditions which we, the programmers, believe should always be true. In particular:

  • This one is in BorrowRef::drop, and asserts that the borrow is a reading borrow. BorrowRef is a reading borrow, so if it weren't true, it'd be a bug.
  • This one is in BorrowRef::clone, and asserts that the borrow is a reading borrow. It's safe for the same reason as the previous one.
  • This one and this one are in BorrowRefMut::drop and BorrowRefMut::clone respectively, check that the borrow is a writing borrow, and are safe for the same reason.
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Sep 21, 2018

@joshlf Thanks for clarifying! With that clarification on the debug_assert! calls, this LGTM.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Sep 21, 2018

That said: could someone add a note to the comment at the top of that file, regarding checking for overflow, that any such overflow checks must be assert! and not debug_assert!, just to make it less likely that future changes accidentally make that mistake?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Nov 10, 2018

@sfackler shall we stabilize this perhaps?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 18, 2019

We previously had Ref::filter_map and removed it in #27746. If map_split is deemed useful enough for inclusion in the standard library, should filter_map be brought back? @rust-lang/libs what do you think?

@joshlf

This comment has been minimized.

Copy link
Contributor Author

joshlf commented Jan 18, 2019

We previously had Ref::filter_map and removed it in #27746. If map_split is deemed useful enough for inclusion in the standard library, should filter_map be brought back?

FWIW, my own use case for map_split has gone away; I no longer use it anymore. So I wouldn't oversell the utility of map_split :)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 18, 2019

I don't personally have an opinion, I'd be fine either way

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Jan 28, 2019

I definitely think this opens a door to a ton of other methods that may be desired, and are just as easily justified, that we may not want to support. For example, fn map_result<T, U, E>(orig: RefMut<'_, T>, f: impl FnOnce(&mut T) -> Result<&mut U, E>) -> Result<RefMut<'_, U>, E> is something that I've needed.

It's worth noting though that filter_map, map_result, and probably others that folks want, can be implemented in a library. I don't see any way that split_map could be.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 1, 2019

This is a fair point. split_map needs to change RefCell fundamentally to make it track (reference count) multiple multiple mutable borrows (of disjoint parts of course). Implementing filter_map in another crate requires unsafe and is tricky to get right, but it is not impossible.

Alright, let’s stabilize split_map, we can always reconsider adding filter_map later.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 1, 2019

Team member @SimonSapin has proposed to merge 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 none object), 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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 1, 2019

I’m not sure this needs sign-off from the whole lang team though. UnsafeCell is a lang item but RefCell is not and it could (as a whole) be a separate library. @rust-lang/lang what do you think?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 2, 2019

I agree, @SimonSapin -- this looks like a purely library change to me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 2, 2019

Alright.

@rfcbot fcp cancel

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 2, 2019

@SimonSapin proposal cancelled.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 2, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 2, 2019

Team member @SimonSapin has proposed to merge 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 none object), 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.

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