Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. #11781

m-rph · 2023-11-09T01:10:26Z

The essence of the issue is that types that implement Borrow provide a facet or a representation of the underlying type. Under these semantics hash(a) == hash(a.borrow()).

This is a problem when a type implements Borrow<str>, Borrow<[u8]> and Hash, it is expected that the hash of all three types is identical. The problem is that the hash of [u8] is not the same as that of a String, even when the byte reference ([u8]) is derived from .as_bytes()

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Explanation of the issue in the code
Tests reproducing the issue
Lint rule and emission

changelog: New lint: [impl_hash_borrow_with_str_and_bytes]
#11781

rustbot · 2023-11-09T01:10:31Z

r? @xFrednet

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

clippy_lints/src/types/hash_borrow_str_soundness.rs

m-rph · 2023-11-09T22:09:00Z

Okay, so as far as I understand, this needs to be a latelint.

We could do this on the early pass and check for impl of all three, but that could cause issues if there are name conflicts.

e.g. Two Structs with the same name in different modules, both impl Hash and Borrow, except one is Borrow and the other is Borrow<[u8]>.

So instead we do a latelint, and check that for the type we don't have both impls.
Does this make sense?

clippy_lints/src/hash_borrow_str_soundness.rs

xFrednet · 2023-11-11T13:18:14Z

Welcome to Clippy and thank you for the PR. I didn't have time to look at this until now, but it seems like you found some excellent help on Zulip, which is awesome.

@y21 would you like to take over the review, since you already helped on Zulip? Once you're done, I can do the final review and r+ for us two :)

y21 · 2023-11-11T13:23:01Z

I see this is still in draft mode. I can give some feedback now, or do you want me to wait until this is marked ready?

xFrednet · 2023-11-11T13:26:51Z

I can give some feedback now, or do you want me to wait until this is marked ready?

This usually depends on the author of the PR. Some use draft mode, when they are still working on the PR and some use it, to request feedback. @partiallytyped posted some questions here, which makes me guess that they wanted to get feedback on their questions. However, they have also asked them on Zulip where you have been helpful. In this case, I would monitor the PR or ask the author if they would already like to have a review: :)

m-rph · 2023-11-11T13:27:46Z

I think we can go ahead with the review while I do some finishing touch ups on the notes :D

@y21 has been very very helpful over on Zulip!

m-rph · 2023-11-11T13:33:52Z

I think this issue pretty much opens a whole branch of checks. This one is specific to Borrow<[u8]> and Borrow<str> as that is guaranteed to be impossible to satisfy, but in general types that impl Borrow<T> and Hash must have identical hashing behaviour and there should be a warning I think.

rustbot · 2023-11-11T13:48:26Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

5b09f90

y21

Some initial feedback :)
(Sorry for the chaotic order, had to jump around a bit in the code)

clippy_lints/src/hash_borrow_str_soundness.rs

m-rph · 2023-11-11T15:16:08Z

@rustbot ready

Hmmm seems like the bot doesn't work if S-waiting-on-review is already there.

y21

Last few things from my side and then I think it looks good to me!

clippy_lints/src/hash_borrow_str_semantics.rs

y21 · 2023-11-11T16:37:46Z

Does this also work when the Hash impl is derived? I think it's worth having a test for that, too.

m-rph · 2023-11-11T17:10:19Z

It does! :D

y21 · 2023-11-12T02:02:20Z

Looks like there are still merge commits somehow, according to rustbot. I think you can just rebase onto master to fix that.

rustbot · 2023-11-12T02:14:40Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

1c56599

m-rph · 2023-11-12T02:16:43Z

This should do it.

Thanks @y21! For all the help :D

y21

Thought about it for a bit and I left some last comments. Most of them are just nits or non-blocking questions/possible improvements though 🙃

clippy_lints/src/hash_borrow_str_semantics.rs

tests/ui/hash_borrow_str_semantics.rs

clippy_lints/src/hash_borrow_str_semantics.rs

tests/ui/hash_borrow_str_semantics.rs

y21

Just some minor documentation nits. Thanks for your patience here.
I'm going to tag @xFrednet since these are probably my last comments.
Aside from the small things in the documentation, this looks great!
It's a very nice lint to have.

clippy_lints/src/hash_borrow_str_semantics.rs

m-rph · 2023-11-12T23:07:25Z

Don't worry about it :D it shows how much you care for this which makes me more excited to help :D

xFrednet · 2023-11-13T19:41:10Z

The start of my week is quite full, it might take until Thursday or the weekend until I have time to give you a full review. Hope that's okay with you :)

Already, a big thanks for the review @y21! I highly appreciate it

bors · 2023-11-14T16:12:40Z

☔ The latest upstream changes (presumably #11791) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2023-11-16T16:59:52Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

9200a9e

xFrednet

The lint logic looks good to me. I have some remarks regarding the cosmetics of the lint. Afterward, it should be good to go.

Thank you for waiting on my reivew :)

clippy_lints/src/hash_borrow_str_semantics.rs

tests/ui/hash_borrow_str_semantics.rs

Implements a lint to prevent implementation of Hash, Borrow<str> and Borrow<[u8]> as it breaks Borrow<T> "semantics". According to the book, types that implement Borrow<A> and Borrow<B> must ensure equality of borrow results under Eq,Ord and Hash. > In particular Eq, Ord and Hash must be equivalent for borrowed and owned values: x.borrow() == y.borrow() should give the same result as x == y. In the same way, hash(x) == hash(x as Borrow<[u8]>) != hash(x as Borrow<str>). changelog: newlint [`impl_hash_with_borrow_str_and_bytes`]

xFrednet · 2023-11-19T10:56:01Z

LGTM, thank you for the update!

@bors r+

bors · 2023-11-19T10:56:03Z

📌 Commit 3c1e0af has been approved by xFrednet

It is now in the queue for this repository.

bors · 2023-11-19T10:57:10Z

⌛ Testing commit 3c1e0af with merge 3eef533...

Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. Fixes #11710 The essence of the issue is that types that implement Borrow<T> provide a facet or a representation of the underlying type. Under these semantics `hash(a) == hash(a.borrow())`. This is a problem when a type implements `Borrow<str>`, `Borrow<[u8]>` and Hash, it is expected that the hash of all three types is identical. The problem is that the hash of [u8] is not the same as that of a String, even when the byte reference ([u8]) is derived from `.as_bytes()` - [x] Followed [lint naming conventions][lint_naming] - [x] Added passing UI tests (including committed `.stderr` file) - [x] `cargo test` passes locally - [x] Executed `cargo dev update_lints` - [x] Added lint documentation - [x] Run `cargo dev fmt` --- - [x] Explanation of the issue in the code - [x] Tests reproducing the issue - [x] Lint rule and emission

bors · 2023-11-19T10:57:36Z

💔 Test failed - checks-action_test

xFrednet · 2023-11-19T10:58:29Z

@bors retry

bors · 2023-11-19T10:59:36Z

⌛ Testing commit 3c1e0af with merge 9c3a365...

bors · 2023-11-19T11:06:18Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 9c3a365 to master...

rustbot assigned xFrednet Nov 9, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 9, 2023

m-rph commented Nov 9, 2023

View reviewed changes

clippy_lints/src/types/hash_borrow_str_soundness.rs Outdated Show resolved Hide resolved

m-rph commented Nov 9, 2023

View reviewed changes

clippy_lints/src/types/hash_borrow_str_soundness.rs Outdated Show resolved Hide resolved

m-rph commented Nov 11, 2023

View reviewed changes

clippy_lints/src/hash_borrow_str_soundness.rs Outdated Show resolved Hide resolved

xFrednet assigned y21 Nov 11, 2023

m-rph marked this pull request as ready for review November 11, 2023 13:27

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 11, 2023

y21 reviewed Nov 11, 2023

View reviewed changes

m-rph changed the title ~~[Draft] Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>.~~ Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. Nov 11, 2023

y21 reviewed Nov 11, 2023

View reviewed changes

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

y21 reviewed Nov 12, 2023

View reviewed changes

m-rph commented Nov 12, 2023

View reviewed changes

tests/ui/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

y21 approved these changes Nov 12, 2023

View reviewed changes

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

clippy_lints/src/hash_borrow_str_semantics.rs Outdated Show resolved Hide resolved

m-rph requested a review from xFrednet November 16, 2023 19:26

xFrednet reviewed Nov 18, 2023

View reviewed changes

xFrednet approved these changes Nov 19, 2023

View reviewed changes

bors merged commit 9c3a365 into rust-lang:master Nov 19, 2023
8 checks passed

This was referenced Nov 19, 2023

new lint: repeat_vec_with_capacity #11597

Merged

New lint: needless_move #11759

Closed

[WIP] New lint manual_checked_op #11789

Draft

m-rph deleted the 11710 branch November 19, 2023 11:22

alexpovel mentioned this pull request Mar 21, 2024

Improve error handling ergonomics (and docs around it) ada-url/rust#62

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. #11781

Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. #11781

m-rph commented Nov 9, 2023 •

edited by xFrednet

Loading

rustbot commented Nov 9, 2023

m-rph commented Nov 9, 2023 •

edited

Loading

xFrednet commented Nov 11, 2023

y21 commented Nov 11, 2023

xFrednet commented Nov 11, 2023

m-rph commented Nov 11, 2023 •

edited

Loading

m-rph commented Nov 11, 2023 •

edited

Loading

rustbot commented Nov 11, 2023

y21 left a comment

m-rph commented Nov 11, 2023 •

edited

Loading

y21 left a comment

y21 commented Nov 11, 2023

m-rph commented Nov 11, 2023 •

edited

Loading

y21 commented Nov 12, 2023

rustbot commented Nov 12, 2023

m-rph commented Nov 12, 2023 •

edited

Loading

y21 left a comment

y21 left a comment

m-rph commented Nov 12, 2023

xFrednet commented Nov 13, 2023 •

edited

Loading

bors commented Nov 14, 2023

rustbot commented Nov 16, 2023

xFrednet left a comment

xFrednet commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

xFrednet commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. #11781

Verify Borrow<T> semantics for types that implement Hash, Borrow<str> and Borrow<[u8]>. #11781

Conversation

m-rph commented Nov 9, 2023 • edited by xFrednet Loading

rustbot commented Nov 9, 2023

m-rph commented Nov 9, 2023 • edited Loading

xFrednet commented Nov 11, 2023

y21 commented Nov 11, 2023

xFrednet commented Nov 11, 2023

m-rph commented Nov 11, 2023 • edited Loading

m-rph commented Nov 11, 2023 • edited Loading

rustbot commented Nov 11, 2023

y21 left a comment

Choose a reason for hiding this comment

m-rph commented Nov 11, 2023 • edited Loading

y21 left a comment

Choose a reason for hiding this comment

y21 commented Nov 11, 2023

m-rph commented Nov 11, 2023 • edited Loading

y21 commented Nov 12, 2023

rustbot commented Nov 12, 2023

m-rph commented Nov 12, 2023 • edited Loading

y21 left a comment

Choose a reason for hiding this comment

y21 left a comment

Choose a reason for hiding this comment

m-rph commented Nov 12, 2023

xFrednet commented Nov 13, 2023 • edited Loading

bors commented Nov 14, 2023

rustbot commented Nov 16, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

xFrednet commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 19, 2023

m-rph commented Nov 9, 2023 •

edited by xFrednet

Loading

m-rph commented Nov 9, 2023 •

edited

Loading

m-rph commented Nov 11, 2023 •

edited

Loading

m-rph commented Nov 11, 2023 •

edited

Loading

m-rph commented Nov 11, 2023 •

edited

Loading

m-rph commented Nov 11, 2023 •

edited

Loading

m-rph commented Nov 12, 2023 •

edited

Loading

xFrednet commented Nov 13, 2023 •

edited

Loading