Skip to content

Commit 3c1e0af

Browse files
New Lint [impl_hash_with_borrow_str_and_bytes]
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`]
1 parent 6eb935a commit 3c1e0af

6 files changed

+287
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -5128,6 +5128,7 @@ Released 2018-09-13
51285128
[`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none
51295129
[`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
51305130
[`ignored_unit_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
5131+
[`impl_hash_borrow_with_str_and_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_hash_borrow_with_str_and_bytes
51315132
[`impl_trait_in_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_trait_in_params
51325133
[`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone
51335134
[`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher

clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
204204
crate::if_not_else::IF_NOT_ELSE_INFO,
205205
crate::if_then_some_else_none::IF_THEN_SOME_ELSE_NONE_INFO,
206206
crate::ignored_unit_patterns::IGNORED_UNIT_PATTERNS_INFO,
207+
crate::impl_hash_with_borrow_str_and_bytes::IMPL_HASH_BORROW_WITH_STR_AND_BYTES_INFO,
207208
crate::implicit_hasher::IMPLICIT_HASHER_INFO,
208209
crate::implicit_return::IMPLICIT_RETURN_INFO,
209210
crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::ty::implements_trait;
3+
use rustc_hir::def::{DefKind, Res};
4+
use rustc_hir::{Item, ItemKind, Path, TraitRef};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty::Ty;
7+
use rustc_session::{declare_lint_pass, declare_tool_lint};
8+
use rustc_span::symbol::sym;
9+
10+
declare_clippy_lint! {
11+
/// ### What it does
12+
///
13+
/// This lint is concerned with the semantics of `Borrow` and `Hash` for a
14+
/// type that implements all three of `Hash`, `Borrow<str>` and `Borrow<[u8]>`
15+
/// as it is impossible to satisfy the semantics of Borrow and `Hash` for
16+
/// both `Borrow<str>` and `Borrow<[u8]>`.
17+
///
18+
/// ### Why is this bad?
19+
///
20+
/// When providing implementations for `Borrow<T>`, one should consider whether the different
21+
/// implementations should act as facets or representations of the underlying type. Generic code
22+
/// typically uses `Borrow<T>` when it relies on the identical behavior of these additional trait
23+
/// implementations. These traits will likely appear as additional trait bounds.
24+
///
25+
/// In particular `Eq`, `Ord` and `Hash` must be equivalent for borrowed and owned values:
26+
/// `x.borrow() == y.borrow()` should give the same result as `x == y`.
27+
/// It follows then that the following equivalence must hold:
28+
/// `hash(x) == hash((x as Borrow<[u8]>).borrow()) == hash((x as Borrow<str>).borrow())`
29+
///
30+
/// Unfortunately it doesn't hold as `hash("abc") != hash("abc".as_bytes())`.
31+
/// This happens because the `Hash` impl for str passes an additional `0xFF` byte to
32+
/// the hasher to avoid collisions. For example, given the tuples `("a", "bc")`, and `("ab", "c")`,
33+
/// the two tuples would have the same hash value if the `0xFF` byte was not added.
34+
///
35+
/// ### Example
36+
///
37+
/// ```
38+
/// use std::borrow::Borrow;
39+
/// use std::hash::{Hash, Hasher};
40+
///
41+
/// struct ExampleType {
42+
/// data: String
43+
/// }
44+
///
45+
/// impl Hash for ExampleType {
46+
/// fn hash<H: Hasher>(&self, state: &mut H) {
47+
/// self.data.hash(state);
48+
/// }
49+
/// }
50+
///
51+
/// impl Borrow<str> for ExampleType {
52+
/// fn borrow(&self) -> &str {
53+
/// &self.data
54+
/// }
55+
/// }
56+
///
57+
/// impl Borrow<[u8]> for ExampleType {
58+
/// fn borrow(&self) -> &[u8] {
59+
/// self.data.as_bytes()
60+
/// }
61+
/// }
62+
/// ```
63+
/// As a consequence, hashing a `&ExampleType` and hashing the result of the two
64+
/// borrows will result in different values.
65+
///
66+
#[clippy::version = "1.76.0"]
67+
pub IMPL_HASH_BORROW_WITH_STR_AND_BYTES,
68+
correctness,
69+
"ensures that the semantics of `Borrow` for `Hash` are satisfied when `Borrow<str>` and `Borrow<[u8]>` are implemented"
70+
}
71+
72+
declare_lint_pass!(ImplHashWithBorrowStrBytes => [IMPL_HASH_BORROW_WITH_STR_AND_BYTES]);
73+
74+
impl LateLintPass<'_> for ImplHashWithBorrowStrBytes {
75+
/// We are emitting this lint at the Hash impl of a type that implements all
76+
/// three of `Hash`, `Borrow<str>` and `Borrow<[u8]>`.
77+
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
78+
if let ItemKind::Impl(imp) = item.kind
79+
&& let Some(TraitRef {path: Path {span, res, ..}, ..}) = imp.of_trait
80+
&& let ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
81+
&& let Some(hash_id) = cx.tcx.get_diagnostic_item(sym::Hash)
82+
&& Res::Def(DefKind::Trait, hash_id) == *res
83+
&& let Some(borrow_id) = cx.tcx.get_diagnostic_item(sym::Borrow)
84+
// since we are in the `Hash` impl, we don't need to check for that.
85+
// we need only to check for `Borrow<str>` and `Borrow<[u8]>`
86+
&& implements_trait(cx, ty, borrow_id, &[cx.tcx.types.str_.into()])
87+
&& implements_trait(cx, ty, borrow_id, &[Ty::new_slice(cx.tcx, cx.tcx.types.u8).into()])
88+
{
89+
span_lint_and_then(
90+
cx,
91+
IMPL_HASH_BORROW_WITH_STR_AND_BYTES,
92+
*span,
93+
"the semantics of `Borrow<T>` around `Hash` can't be satisfied when both `Borrow<str>` and `Borrow<[u8]>` are implemented",
94+
|diag| {
95+
diag.note("the `Borrow` semantics require that `Hash` must behave the same for all implementations of Borrow<T>");
96+
diag.note(
97+
"however, the hash implementations of a string (`str`) and the bytes of a string `[u8]` do not behave the same ..."
98+
);
99+
diag.note("... as (`hash(\"abc\") != hash(\"abc\".as_bytes())`");
100+
diag.help("consider either removing one of the `Borrow` implementations (`Borrow<str>` or `Borrow<[u8]>`) ...");
101+
diag.help("... or not implementing `Hash` for this type");
102+
},
103+
);
104+
}
105+
}
106+
}

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ mod if_let_mutex;
144144
mod if_not_else;
145145
mod if_then_some_else_none;
146146
mod ignored_unit_patterns;
147+
mod impl_hash_with_borrow_str_and_bytes;
147148
mod implicit_hasher;
148149
mod implicit_return;
149150
mod implicit_saturating_add;
@@ -1066,6 +1067,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
10661067
store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv())));
10671068
store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
10681069
store.register_late_pass(|_| Box::new(iter_over_hash_type::IterOverHashType));
1070+
store.register_late_pass(|_| Box::new(impl_hash_with_borrow_str_and_bytes::ImplHashWithBorrowStrBytes));
10691071
// add lints here, do not remove this comment, it's used in `new_lint`
10701072
}
10711073

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
#![warn(clippy::impl_hash_borrow_with_str_and_bytes)]
2+
3+
use std::borrow::Borrow;
4+
use std::hash::{Hash, Hasher};
5+
6+
struct ExampleType {
7+
data: String,
8+
}
9+
10+
impl Hash for ExampleType {
11+
//~^ ERROR: can't
12+
fn hash<H: Hasher>(&self, state: &mut H) {
13+
self.data.hash(state);
14+
}
15+
}
16+
17+
impl Borrow<str> for ExampleType {
18+
fn borrow(&self) -> &str {
19+
&self.data
20+
}
21+
}
22+
23+
impl Borrow<[u8]> for ExampleType {
24+
fn borrow(&self) -> &[u8] {
25+
self.data.as_bytes()
26+
}
27+
}
28+
29+
struct ShouldNotRaiseForHash {}
30+
impl Hash for ShouldNotRaiseForHash {
31+
fn hash<H: Hasher>(&self, state: &mut H) {
32+
todo!();
33+
}
34+
}
35+
36+
struct ShouldNotRaiseForBorrow {}
37+
impl Borrow<str> for ShouldNotRaiseForBorrow {
38+
fn borrow(&self) -> &str {
39+
todo!();
40+
}
41+
}
42+
impl Borrow<[u8]> for ShouldNotRaiseForBorrow {
43+
fn borrow(&self) -> &[u8] {
44+
todo!();
45+
}
46+
}
47+
48+
struct ShouldNotRaiseForHashBorrowStr {}
49+
impl Hash for ShouldNotRaiseForHashBorrowStr {
50+
fn hash<H: Hasher>(&self, state: &mut H) {
51+
todo!();
52+
}
53+
}
54+
impl Borrow<str> for ShouldNotRaiseForHashBorrowStr {
55+
fn borrow(&self) -> &str {
56+
todo!();
57+
}
58+
}
59+
60+
struct ShouldNotRaiseForHashBorrowSlice {}
61+
impl Hash for ShouldNotRaiseForHashBorrowSlice {
62+
fn hash<H: Hasher>(&self, state: &mut H) {
63+
todo!();
64+
}
65+
}
66+
67+
impl Borrow<[u8]> for ShouldNotRaiseForHashBorrowSlice {
68+
fn borrow(&self) -> &[u8] {
69+
todo!();
70+
}
71+
}
72+
73+
#[derive(Hash)]
74+
//~^ ERROR: can't
75+
struct Derived {
76+
data: String,
77+
}
78+
79+
impl Borrow<str> for Derived {
80+
fn borrow(&self) -> &str {
81+
self.data.as_str()
82+
}
83+
}
84+
85+
impl Borrow<[u8]> for Derived {
86+
fn borrow(&self) -> &[u8] {
87+
self.data.as_bytes()
88+
}
89+
}
90+
91+
struct GenericExampleType<T> {
92+
data: T,
93+
}
94+
95+
impl<T: Hash> Hash for GenericExampleType<T> {
96+
fn hash<H: Hasher>(&self, state: &mut H) {
97+
self.data.hash(state);
98+
}
99+
}
100+
101+
impl Borrow<str> for GenericExampleType<String> {
102+
fn borrow(&self) -> &str {
103+
&self.data
104+
}
105+
}
106+
107+
impl Borrow<[u8]> for GenericExampleType<&'static [u8]> {
108+
fn borrow(&self) -> &[u8] {
109+
self.data
110+
}
111+
}
112+
113+
struct GenericExampleType2<T> {
114+
data: T,
115+
}
116+
117+
impl Hash for GenericExampleType2<String> {
118+
//~^ ERROR: can't
119+
// this is correctly throwing an error for generic with concrete impl
120+
// for all 3 types
121+
fn hash<H: Hasher>(&self, state: &mut H) {
122+
self.data.hash(state);
123+
}
124+
}
125+
126+
impl Borrow<str> for GenericExampleType2<String> {
127+
fn borrow(&self) -> &str {
128+
&self.data
129+
}
130+
}
131+
132+
impl Borrow<[u8]> for GenericExampleType2<String> {
133+
fn borrow(&self) -> &[u8] {
134+
self.data.as_bytes()
135+
}
136+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
error: the semantics of `Borrow<T>` around `Hash` can't be satisfied when both `Borrow<str>` and `Borrow<[u8]>` are implemented
2+
--> $DIR/impl_hash_with_borrow_str_and_bytes.rs:10:6
3+
|
4+
LL | impl Hash for ExampleType {
5+
| ^^^^
6+
|
7+
= note: the `Borrow` semantics require that `Hash` must behave the same for all implementations of Borrow<T>
8+
= note: however, the hash implementations of a string (`str`) and the bytes of a string `[u8]` do not behave the same ...
9+
= note: ... as (`hash("abc") != hash("abc".as_bytes())`
10+
= help: consider either removing one of the `Borrow` implementations (`Borrow<str>` or `Borrow<[u8]>`) ...
11+
= help: ... or not implementing `Hash` for this type
12+
= note: `-D clippy::impl-hash-borrow-with-str-and-bytes` implied by `-D warnings`
13+
= help: to override `-D warnings` add `#[allow(clippy::impl_hash_borrow_with_str_and_bytes)]`
14+
15+
error: the semantics of `Borrow<T>` around `Hash` can't be satisfied when both `Borrow<str>` and `Borrow<[u8]>` are implemented
16+
--> $DIR/impl_hash_with_borrow_str_and_bytes.rs:73:10
17+
|
18+
LL | #[derive(Hash)]
19+
| ^^^^
20+
|
21+
= note: the `Borrow` semantics require that `Hash` must behave the same for all implementations of Borrow<T>
22+
= note: however, the hash implementations of a string (`str`) and the bytes of a string `[u8]` do not behave the same ...
23+
= note: ... as (`hash("abc") != hash("abc".as_bytes())`
24+
= help: consider either removing one of the `Borrow` implementations (`Borrow<str>` or `Borrow<[u8]>`) ...
25+
= help: ... or not implementing `Hash` for this type
26+
= note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)
27+
28+
error: the semantics of `Borrow<T>` around `Hash` can't be satisfied when both `Borrow<str>` and `Borrow<[u8]>` are implemented
29+
--> $DIR/impl_hash_with_borrow_str_and_bytes.rs:117:6
30+
|
31+
LL | impl Hash for GenericExampleType2<String> {
32+
| ^^^^
33+
|
34+
= note: the `Borrow` semantics require that `Hash` must behave the same for all implementations of Borrow<T>
35+
= note: however, the hash implementations of a string (`str`) and the bytes of a string `[u8]` do not behave the same ...
36+
= note: ... as (`hash("abc") != hash("abc".as_bytes())`
37+
= help: consider either removing one of the `Borrow` implementations (`Borrow<str>` or `Borrow<[u8]>`) ...
38+
= help: ... or not implementing `Hash` for this type
39+
40+
error: aborting due to 3 previous errors
41+

0 commit comments

Comments
 (0)