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

Be more direct about borrow contract #59663

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Be more direct about borrow contract #59663

merged 1 commit into from
Apr 4, 2019

Conversation

matklad
Copy link
Member

@matklad matklad commented Apr 3, 2019

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

If you implement Borrow, you must make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some vague and philosophical notions are used.

So I suggest to mention the requirements of Borrow very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as Borrow differs from AsRef in W, so that's why Borrow is for X and AsRef is for Y.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}

Now Bob uses this Person struct, puts it into HashMap and tries to look it up using &str for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that Bob is to blame: Eq and Hash bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the Borrow impl does not yield well-behaved Eq, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes #44868

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2019
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a useful distinction.

@dtolnay
Copy link
Member

dtolnay commented Apr 3, 2019

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 3, 2019

📌 Commit 1cfed0d has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
Centril added a commit to Centril/rust that referenced this pull request Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
bors added a commit that referenced this pull request Apr 4, 2019
Rollup of 6 pull requests

Successful merges:

 - #59316 (Internal lints take 2)
 - #59663 (Be more direct about borrow contract)
 - #59664 (Updated the documentation of spin_loop and spin_loop_hint)
 - #59666 (Updated the environment description in rustc.)
 - #59669 (Reduce repetition in librustc(_lint) wrt. impl LintPass by using macros)
 - #59677 (rustfix coverage: Skip UI tests with non-json error-format)

Failed merges:

r? @ghost
@bors bors merged commit 1cfed0d into rust-lang:master Apr 4, 2019
@matklad matklad deleted the borrow branch April 4, 2019 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsRef/Borrow/BorrowMut need better documentation
4 participants