-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(map_entry): provide explicit type when suggesting None
#15759
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
Conversation
rustbot has assigned @samueltardieu. Use |
peel_hir_expr_while, | ||
}; | ||
use core::fmt::{self, Write}; | ||
use core::fmt::Write; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, why not use std::fmt::Write
since std::fmt
is also imported?
"e.insert({})", | ||
snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0 | ||
); | ||
let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're cleaning up, "_" would be a better choice than ".." for a single value (even though this is never used…). Same below.
)); | ||
res.push_str(&snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0); | ||
let value_str = snippet_with_context(cx, insertion.value.span, ctxt, "..", app).0; | ||
let _: fmt::Result = write!(res, "{value_str}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not res.push_str(&value_str);
which is infallible?
fn issue15307<K: Eq + Hash, V>(mut m: HashMap<K, V>, k: K, v: V) { | ||
if let std::collections::hash_map::Entry::Vacant(e) = m.entry(k) { | ||
//~^ map_entry | ||
assert!({ e.insert(v); None::<V> }.is_none()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so ugly.
If the user is using the result of m.insert(…)
which we know to be None
(as opposed to dropping it), can't we just suggest the same thing as we currently do, but add that m.insert(…)
would always have returned None
, and suggest to rewrite the code to not depend on this value or to add an explicit type to None
if needed?
I dislike suggesting a type from a rustc_middle::ty::Ty
when it will not be what the user would write when aliases are involved for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More generally, I don't like us generating fixes that we know to be of a very bad style just to make it compatible with the surrounding code. In this case, we might as well warn about the performance issue, but not suggest an autofix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing this required some substantial changes, so I opened a v2 of the PR: #15808
Closed in favor of #15808 |
Fixes #15307
changelog: [
map_entry
]: provide explicit type when suggestingNone