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

Short-circuit Rc/Arc equality checking on equal pointers where T: Eq #56550

Open
wants to merge 3 commits into
base: master
from

Conversation

@chpio

chpio commented Dec 5, 2018

based on #42965

Is the use of the private trait ok this way? Is there anything else needed for this to get pulled?

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 5, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Dec 5, 2018

@oli-obk

Do we have tests anywhere along the lines of

    let x = std::rc::Rc::new(std::f32::NAN);
    assert_ne!(x, x);
Show resolved Hide resolved src/liballoc/rc.rs

@oli-obk oli-obk added the T-libs label Dec 6, 2018

Show resolved Hide resolved src/liballoc/rc.rs Outdated
Show resolved Hide resolved src/liballoc/sync.rs Outdated
Show resolved Hide resolved src/liballoc/rc.rs Outdated

@chpio chpio force-pushed the chpio:rc-eq branch from 21bff17 to 1e2ac05 Dec 6, 2018

@chpio chpio force-pushed the chpio:rc-eq branch from 1e2ac05 to d828c22 Dec 8, 2018

@chpio

This comment has been minimized.

chpio commented Dec 8, 2018

@oli-obk added a few tests, but it think they are somewhat wanky, testing the specific impl.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 11, 2018

Thanks for the PR @chpio and sorry for the delay! Initially reading this I was wary that this would break cases like @oli-obk mentioned above, but I think after reading over it this is correct and we should consider merging it.

@rfcbot fcp merge

For those just joining, this reimplements PartialEq for Rc<T> and Arc<T> with internal specialization that does a pointer check before calling T::eq if T: Eq. According to Eq's own documentation it signifies the reflexive property (a == a) which I believe means that this is a semantically correct implementation.

One minor worry I'd have is that for very simple T this may cause Rc<T>::eq to be a bit slower. For example Rc<i32> may be more expensive now as the previous one tiny branch is now two tiny branches. Have others seen this sort of optimization in standard libraries before? Should we be wary of something like this?

@rfcbot

This comment has been minimized.

rfcbot commented Dec 11, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

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.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Dec 11, 2018

We document that Eq is supposed to be reflexive, but unsafe code cannot rely on that, so its plausible that some implementations aren't following that. Isn't it possible this will break someone's code in which they had a non-reflexive PartialEq that still impl'd Eq? Are we fine with that?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 11, 2018

Code would only break if it relies on a buggy implementation of Eq to stay buggy. I have a hard time to think of a "real" case where that could happen.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

Another good point to bring up @withoutboats! In addition to relying on a buggy Eq implementation they'd also have to be relying somehow on PartialEq for Rc<BuggyType>, which I think puts me in @SimonSapin's camp of "seems very unlikely to come up in practice"

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Dec 12, 2018

I think I'm inclined to agree, mainly from a pragmatic perspective - its not likely to come up in practice. But I'm uncertain what our general guidelines around backwards compatibility in regard to the "guarantees" that are made about the behavior of traits in std. There's nothing unsafe about making an Eq impl that isn't reflexive, and you can't rely on that reflexivity to uphold memory invariants. So where is the line drawn on whose backwards compatibility we are willing to guarantee? Which unenforced contracts of traits are considered "bugs" not to uphold?

For example, members of the team have previously argued that types that implement both ToString and FromStr should be roundtrippable, would we consider a third party library that doesn't uphold that guarantee similarly buggy and be fine if a change to std broke them? That's what I'm trying to clarify, I guess: we can't actually rely on ecosystem impls upholding untyped contracts, how much allowance is there to implement the traits in a manner different from their documented intent?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 12, 2018

There's nothing unsafe about making an Eq impl that isn't reflexive,

But there’s nothing unsafe about changing the result of Rc<Foo>::eq either, is there?

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Dec 12, 2018

@SimonSapin Definitely not. This is about backwards compatibility.

I bring up unsafe because if the contract were reliable for to avoid UB, breaking that code would clearly be permissible. If Eq were an unsafe trait and we could rely on its reflexivity for memory safety, clearly it would be permissible to break code with a non-reflexive Eq impl, because it failed to uphold the invariants its required to as unsafe code.

But the reflexivity is in this different category of contracts, which only exist in the std documentation and aren't part of the language. How do we draw the line about which of those contracts you need to uphold to get backwards compatibility from std, and which are just contracts that std is guaranteeing to uphold?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

I think the way I typically approach these kinds of problems is that we strive toward the world we want to be in, for example where Eq always represents reflexivity. If, however, along the way we do something that ends up having a large practical impact because that's in fact not true (aka not everything is reflexive) then we backpedal and figure out how to work with that.

That may be a bit cavalier though and certainly isn't as conservative as we could be. In any case one thing I've forgotten is that we could certainly run a crater run for this and just see if there's any impact there initially.

@bors: try

@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

⌛️ Trying commit d828c22 with merge aa49d8e...

bors added a commit that referenced this pull request Dec 12, 2018

Auto merge of #56550 - chpio:rc-eq, r=<try>
Short-circuit Rc/Arc equality checking on equal pointers where T: Eq

based on #42965

Is the use of the private trait ok this way? Is there anything else needed for this to get pulled?
@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

☀️ Test successful - status-travis
State: approved= try=True

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

@craterbot run start=master#bd47d6825bf4090517549d33cfef10d3300b4a75 end=try#aa49d8ef14939ddec0e34b346b60174a5673d48f mode=build-and-test

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Dec 12, 2018

👌 Experiment pr-56550 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Dec 12, 2018

🚧 Experiment pr-56550 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Kimundi

This comment has been minimized.

Member

Kimundi commented Dec 12, 2018

I see it similar to the ability to define a Clone impl for a T: Copy that does more than just a memcpy: Its safe to do so, but a violation of the interface, so if some code does not end up calling the method its because you misused the interface.

@rfcbot

This comment has been minimized.

rfcbot commented Dec 12, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@chpio

This comment has been minimized.

chpio commented Dec 13, 2018

Is crater just calling cargo check? It would be pretty useless for this PR if that's the case.

@memoryruins

This comment has been minimized.

Contributor

memoryruins commented Dec 13, 2018

The current agent is in build-and-test mode (check the final portion of the command issued to craterbot).

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Dec 16, 2018

🎉 Experiment pr-56550 is completed!
📊 13 regressed and 14 fixed (50551 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@chpio

This comment has been minimized.

chpio commented Dec 16, 2018

couldn't spot any meaningful regressions here, just some timing or env-var not set failures. is anyone else willing to look into it?

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