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

fix: Detect "bound more than once" error and suppress need-mut for it. #14970

Merged
merged 1 commit into from Jun 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 55 additions & 7 deletions crates/hir-def/src/body/lower.rs
Expand Up @@ -30,9 +30,9 @@ use crate::{
db::DefDatabase,
expander::Expander,
hir::{
dummy_expr_id, Array, Binding, BindingAnnotation, BindingId, CaptureBy, ClosureKind, Expr,
ExprId, Label, LabelId, Literal, LiteralOrConst, MatchArm, Movability, Pat, PatId,
RecordFieldPat, RecordLitField, Statement,
dummy_expr_id, Array, Binding, BindingAnnotation, BindingId, BindingProblems, CaptureBy,
ClosureKind, Expr, ExprId, Label, LabelId, Literal, LiteralOrConst, MatchArm, Movability,
Pat, PatId, RecordFieldPat, RecordLitField, Statement,
},
item_scope::BuiltinShadowMode,
lang_item::LangItem,
Expand Down Expand Up @@ -141,6 +141,8 @@ impl RibKind {
#[derive(Debug, Default)]
struct BindingList {
map: FxHashMap<Name, BindingId>,
is_used: FxHashMap<BindingId, bool>,
reject_new: bool,
}

impl BindingList {
Expand All @@ -150,7 +152,27 @@ impl BindingList {
name: Name,
mode: BindingAnnotation,
) -> BindingId {
*self.map.entry(name).or_insert_with_key(|n| ec.alloc_binding(n.clone(), mode))
let id = *self.map.entry(name).or_insert_with_key(|n| ec.alloc_binding(n.clone(), mode));
if ec.body.bindings[id].mode != mode {
ec.body.bindings[id].problems = Some(BindingProblems::BoundInconsistently);
}
self.check_is_used(ec, id);
id
}

fn check_is_used(&mut self, ec: &mut ExprCollector<'_>, id: BindingId) {
match self.is_used.get(&id) {
None => {
if self.reject_new {
ec.body.bindings[id].problems = Some(BindingProblems::NotBoundAcrossAll);
}
}
Some(true) => {
ec.body.bindings[id].problems = Some(BindingProblems::BoundMoreThanOnce);
}
Some(false) => {}
}
self.is_used.insert(id, true);
}
}

Expand Down Expand Up @@ -1208,9 +1230,34 @@ impl ExprCollector<'_> {
p.path().and_then(|path| self.expander.parse_path(self.db, path)).map(Box::new);
path.map(Pat::Path).unwrap_or(Pat::Missing)
}
ast::Pat::OrPat(p) => {
let pats = p.pats().map(|p| self.collect_pat(p, binding_list)).collect();
Pat::Or(pats)
ast::Pat::OrPat(p) => 'b: {
let prev_is_used = mem::take(&mut binding_list.is_used);
let prev_reject_new = mem::take(&mut binding_list.reject_new);
let mut pats = Vec::with_capacity(p.pats().count());
let mut it = p.pats();
let Some(first) = it.next() else {
break 'b Pat::Or(Box::new([]));
};
pats.push(self.collect_pat(first, binding_list));
binding_list.reject_new = true;
for rest in it {
for (_, x) in binding_list.is_used.iter_mut() {
*x = false;
}
pats.push(self.collect_pat(rest, binding_list));
for (&id, &x) in binding_list.is_used.iter() {
if !x {
self.body.bindings[id].problems =
Some(BindingProblems::NotBoundAcrossAll);
}
}
}
binding_list.reject_new = prev_reject_new;
let current_is_used = mem::replace(&mut binding_list.is_used, prev_is_used);
for (id, _) in current_is_used.into_iter() {
binding_list.check_is_used(self, id);
}
Pat::Or(pats.into())
}
ast::Pat::ParenPat(p) => return self.collect_pat_opt(p.pat(), binding_list),
ast::Pat::TuplePat(p) => {
Expand Down Expand Up @@ -1499,6 +1546,7 @@ impl ExprCollector<'_> {
mode,
definitions: SmallVec::new(),
owner: self.current_binding_owner,
problems: None,
})
}

Expand Down
11 changes: 11 additions & 0 deletions crates/hir-def/src/hir.rs
Expand Up @@ -486,6 +486,16 @@ impl BindingAnnotation {
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub enum BindingProblems {
/// https://doc.rust-lang.org/stable/error_codes/E0416.html
BoundMoreThanOnce,
/// https://doc.rust-lang.org/stable/error_codes/E0409.html
BoundInconsistently,
/// https://doc.rust-lang.org/stable/error_codes/E0408.html
NotBoundAcrossAll,
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct Binding {
pub name: Name,
Expand All @@ -494,6 +504,7 @@ pub struct Binding {
/// Id of the closure/generator that owns this binding. If it is owned by the
/// top level expression, this field would be `None`.
pub owner: Option<ExprId>,
pub problems: Option<BindingProblems>,
}

impl Binding {
Expand Down
6 changes: 5 additions & 1 deletion crates/hir/src/lib.rs
Expand Up @@ -1643,7 +1643,11 @@ impl DefWithBody {
)
}
let mol = &borrowck_result.mutability_of_locals;
for (binding_id, _) in hir_body.bindings.iter() {
for (binding_id, binding_data) in hir_body.bindings.iter() {
if binding_data.problems.is_some() {
// We should report specific diagnostics for these problems, not `need-mut` and `unused-mut`.
continue;
}
let Some(&local) = mir_body.binding_locals.get(binding_id) else {
continue;
};
Expand Down
13 changes: 13 additions & 0 deletions crates/ide-diagnostics/src/handlers/mutability_errors.rs
Expand Up @@ -620,6 +620,19 @@ fn f((x, y): (i32, i32)) {
);
}

#[test]
fn no_diagnostics_in_case_of_multiple_bounds() {
check_diagnostics(
r#"
fn f() {
let (b, a, b) = (2, 3, 5);
a = 8;
//^^^^^ 💡 error: cannot mutate immutable variable `a`
}
"#,
);
}

#[test]
fn for_loop() {
check_diagnostics(
Expand Down