Skip to content

Commit 4b9b101

Browse files
VeykrilVeykril
committed
fix: Fix upvar analysis of nested closures
1 parent eb05888 commit 4b9b101

File tree

4 files changed

+92
-66
lines changed

4 files changed

+92
-66
lines changed

‎crates/hir-def/src/expr_store.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -474,8 +474,8 @@ impl ExpressionStore {
474474
match expr_only.binding_owners.get(&binding) {
475475
Some(it) => {
476476
// We assign expression ids in a way that outer closures will receive
477-
// a lower id
478-
it.into_raw() < relative_to.into_raw()
477+
// a higher id (allocated after their body is collected)
478+
it.into_raw() > relative_to.into_raw()
479479
}
480480
None => true,
481481
}

‎crates/hir-ty/src/tests/closure_captures.rs‎

Lines changed: 60 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ fn check_closure_captures(#[rust_analyzer::rust_fixture] ra_fixture: &str, expec
135135
fn deref_in_let() {
136136
check_closure_captures(
137137
r#"
138-
//- minicore:copy
138+
//- minicore:copy, fn
139139
fn main() {
140140
let a = &mut true;
141141
let closure = || { let b = *a; };
@@ -149,7 +149,7 @@ fn main() {
149149
fn deref_then_ref_pattern() {
150150
check_closure_captures(
151151
r#"
152-
//- minicore:copy
152+
//- minicore:copy, fn
153153
fn main() {
154154
let a = &mut true;
155155
let closure = || { let &mut ref b = a; };
@@ -159,7 +159,7 @@ fn main() {
159159
);
160160
check_closure_captures(
161161
r#"
162-
//- minicore:copy
162+
//- minicore:copy, fn
163163
fn main() {
164164
let a = &mut true;
165165
let closure = || { let &mut ref mut b = a; };
@@ -173,7 +173,7 @@ fn main() {
173173
fn unique_borrow() {
174174
check_closure_captures(
175175
r#"
176-
//- minicore:copy
176+
//- minicore:copy, fn
177177
fn main() {
178178
let a = &mut true;
179179
let closure = || { *a = false; };
@@ -187,7 +187,7 @@ fn main() {
187187
fn deref_ref_mut() {
188188
check_closure_captures(
189189
r#"
190-
//- minicore:copy
190+
//- minicore:copy, fn
191191
fn main() {
192192
let a = &mut true;
193193
let closure = || { let ref mut b = *a; };
@@ -201,7 +201,7 @@ fn main() {
201201
fn let_else_not_consuming() {
202202
check_closure_captures(
203203
r#"
204-
//- minicore:copy
204+
//- minicore:copy, fn
205205
fn main() {
206206
let a = &mut true;
207207
let closure = || { let _ = *a else { return; }; };
@@ -215,7 +215,7 @@ fn main() {
215215
fn consume() {
216216
check_closure_captures(
217217
r#"
218-
//- minicore:copy
218+
//- minicore:copy, fn
219219
struct NonCopy;
220220
fn main() {
221221
let a = NonCopy;
@@ -230,7 +230,7 @@ fn main() {
230230
fn ref_to_upvar() {
231231
check_closure_captures(
232232
r#"
233-
//- minicore:copy
233+
//- minicore:copy, fn
234234
struct NonCopy;
235235
fn main() {
236236
let mut a = NonCopy;
@@ -248,7 +248,7 @@ fn main() {
248248
fn field() {
249249
check_closure_captures(
250250
r#"
251-
//- minicore:copy
251+
//- minicore:copy, fn
252252
struct Foo { a: i32, b: i32 }
253253
fn main() {
254254
let a = Foo { a: 0, b: 0 };
@@ -263,7 +263,7 @@ fn main() {
263263
fn fields_different_mode() {
264264
check_closure_captures(
265265
r#"
266-
//- minicore:copy
266+
//- minicore:copy, fn
267267
struct NonCopy;
268268
struct Foo { a: i32, b: i32, c: NonCopy, d: bool }
269269
fn main() {
@@ -286,7 +286,7 @@ fn main() {
286286
fn autoref() {
287287
check_closure_captures(
288288
r#"
289-
//- minicore:copy
289+
//- minicore:copy, fn
290290
struct Foo;
291291
impl Foo {
292292
fn imm(&self) {}
@@ -308,7 +308,7 @@ fn main() {
308308
fn captures_priority() {
309309
check_closure_captures(
310310
r#"
311-
//- minicore:copy
311+
//- minicore:copy, fn
312312
struct NonCopy;
313313
fn main() {
314314
let mut a = &mut true;
@@ -336,7 +336,7 @@ fn main() {
336336
fn let_underscore() {
337337
check_closure_captures(
338338
r#"
339-
//- minicore:copy
339+
//- minicore:copy, fn
340340
fn main() {
341341
let mut a = true;
342342
let closure = || { let _ = a; };
@@ -350,7 +350,7 @@ fn main() {
350350
fn match_wildcard() {
351351
check_closure_captures(
352352
r#"
353-
//- minicore:copy
353+
//- minicore:copy, fn
354354
struct NonCopy;
355355
fn main() {
356356
let mut a = NonCopy;
@@ -375,7 +375,7 @@ fn main() {
375375
fn multiple_bindings() {
376376
check_closure_captures(
377377
r#"
378-
//- minicore:copy
378+
//- minicore:copy, fn
379379
fn main() {
380380
let mut a = false;
381381
let mut closure = || { let (b | b) = a; };
@@ -389,7 +389,7 @@ fn main() {
389389
fn multiple_usages() {
390390
check_closure_captures(
391391
r#"
392-
//- minicore:copy
392+
//- minicore:copy, fn
393393
fn main() {
394394
let mut a = false;
395395
let mut closure = || {
@@ -410,7 +410,7 @@ fn main() {
410410
fn ref_then_deref() {
411411
check_closure_captures(
412412
r#"
413-
//- minicore:copy
413+
//- minicore:copy, fn
414414
fn main() {
415415
let mut a = false;
416416
let mut closure = || { let b = *&mut a; };
@@ -424,7 +424,7 @@ fn main() {
424424
fn ref_of_ref() {
425425
check_closure_captures(
426426
r#"
427-
//- minicore:copy
427+
//- minicore:copy, fn
428428
fn main() {
429429
let mut a = &false;
430430
let closure = || { let b = &a; };
@@ -446,7 +446,7 @@ fn main() {
446446
fn multiple_capture_usages() {
447447
check_closure_captures(
448448
r#"
449-
//- minicore:copy
449+
//- minicore:copy, fn
450450
struct A { a: i32, b: bool }
451451
fn main() {
452452
let mut a = A { a: 123, b: false };
@@ -465,7 +465,7 @@ fn main() {
465465
fn let_binding_is_a_ref_capture_in_ref_binding() {
466466
check_closure_captures(
467467
r#"
468-
//- minicore:copy
468+
//- minicore:copy, fn
469469
struct S;
470470
fn main() {
471471
let mut s = S;
@@ -489,7 +489,7 @@ fn main() {
489489
fn let_binding_is_a_value_capture_in_binding() {
490490
check_closure_captures(
491491
r#"
492-
//- minicore:copy, option
492+
//- minicore:copy, fn, option
493493
struct Box(i32);
494494
fn main() {
495495
let b = Some(Box(0));
@@ -508,7 +508,7 @@ fn main() {
508508
fn alias_needs_to_be_normalized() {
509509
check_closure_captures(
510510
r#"
511-
//- minicore:copy
511+
//- minicore:copy, fn
512512
trait Trait {
513513
type Associated;
514514
}
@@ -528,3 +528,41 @@ fn main() {
528528
expect!["220..257;174..175;245..250 ByRef(Shared) c.b.x &'? i32"],
529529
);
530530
}
531+
532+
#[test]
533+
fn nested_ref_captures_from_outer() {
534+
check_closure_captures(
535+
r#"
536+
//- minicore:copy, fn
537+
fn f() {
538+
let a = 1;
539+
let a_closure = || {
540+
let b_closure = || {
541+
{ a };
542+
};
543+
};
544+
}
545+
"#,
546+
expect![[r#"
547+
44..113;17..18;92..93 ByRef(Shared) a &'? i32
548+
73..106;17..18;92..93 ByRef(Shared) a &'? i32"#]],
549+
);
550+
}
551+
552+
#[test]
553+
fn nested_ref_captures() {
554+
check_closure_captures(
555+
r#"
556+
//- minicore:copy, fn
557+
fn f() {
558+
let a_closure = || {
559+
let b = 2;
560+
let b_closure = || {
561+
{ b };
562+
};
563+
};
564+
}
565+
"#,
566+
expect!["77..110;46..47;96..97 ByRef(Shared) b &'? i32"],
567+
);
568+
}

‎crates/ide-diagnostics/src/handlers/mutability_errors.rs‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
#![expect(unused, reason = "diagnostics is temporarily disabled due to too many false positives")]
2-
31
use hir::db::ExpandDatabase;
42
use ide_db::source_change::SourceChange;
53
use ide_db::text_edit::TextEdit;
@@ -90,25 +88,23 @@ pub(crate) fn unused_mut(ctx: &DiagnosticsContext<'_>, d: &hir::UnusedMut) -> Op
9088
)])
9189
})();
9290
let ast = d.local.primary_source(ctx.sema.db).syntax_ptr();
93-
// Some(
94-
// Diagnostic::new_with_syntax_node_ptr(
95-
// ctx,
96-
// DiagnosticCode::RustcLint("unused_mut"),
97-
// "variable does not need to be mutable",
98-
// ast,
99-
// )
100-
// // Not supporting `#[allow(unused_mut)]` in proc macros leads to false positive, hence not stable.
101-
// .with_fixes(fixes),
102-
// )
103-
None
91+
Some(
92+
Diagnostic::new_with_syntax_node_ptr(
93+
ctx,
94+
DiagnosticCode::RustcLint("unused_mut"),
95+
"variable does not need to be mutable",
96+
ast,
97+
)
98+
// Not supporting `#[allow(unused_mut)]` in proc macros leads to false positive, hence not stable.
99+
.with_fixes(fixes),
100+
)
104101
}
105102

106103
pub(super) fn token(parent: &SyntaxNode, kind: SyntaxKind) -> Option<SyntaxToken> {
107104
parent.children_with_tokens().filter_map(|it| it.into_token()).find(|it| it.kind() == kind)
108105
}
109106

110107
#[cfg(test)]
111-
#[cfg(false)] // Diagnostic temporarily disabled
112108
mod tests {
113109
use crate::tests::{check_diagnostics, check_diagnostics_with_disabled, check_fix};
114110

@@ -999,10 +995,6 @@ fn fn_once(mut x: impl FnOnce(u8) -> u8) -> u8 {
999995
}
1000996
"#,
1001997
);
1002-
// FIXME: There should be no "unused variable" here, and there should be a mutability error,
1003-
// but our MIR infra is horribly broken and due to the order in which expressions are lowered
1004-
// there is no `StorageLive` for `x` in the closure (in fact, `x` should not even be a variable
1005-
// of the closure, the environment should be, but as I said, our MIR infra is horribly broken).
1006998
check_diagnostics(
1007999
r#"
10081000
//- minicore: copy, fn
@@ -1011,8 +1003,8 @@ fn f() {
10111003
|| {
10121004
|| {
10131005
let x = 2;
1014-
// ^ 💡 warn: unused variable
10151006
|| { || { x = 5; } }
1007+
//^^^^^ 💡 error: cannot mutate immutable variable `x`
10161008
}
10171009
}
10181010
};

‎crates/ide-diagnostics/src/handlers/unused_variables.rs‎

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
#![expect(unused, reason = "diagnostics is temporarily disabled due to too many false positives")]
2-
31
use hir::Name;
42
use ide_db::text_edit::TextEdit;
53
use ide_db::{
@@ -42,26 +40,25 @@ pub(crate) fn unused_variables(
4240
.and_then(syntax::ast::RecordPatField::cast)
4341
.is_some_and(|field| field.colon_token().is_none());
4442
let var_name = d.local.name(ctx.sema.db);
45-
// Some(
46-
// Diagnostic::new_with_syntax_node_ptr(
47-
// ctx,
48-
// DiagnosticCode::RustcLint("unused_variables"),
49-
// "unused variable",
50-
// ast,
51-
// )
52-
// .with_fixes(name_range.and_then(|it| {
53-
// fixes(
54-
// ctx.sema.db,
55-
// var_name,
56-
// it.range,
57-
// diagnostic_range,
58-
// ast.file_id.is_macro(),
59-
// is_shorthand_field,
60-
// ctx.edition,
61-
// )
62-
// })),
63-
// )
64-
None
43+
Some(
44+
Diagnostic::new_with_syntax_node_ptr(
45+
ctx,
46+
DiagnosticCode::RustcLint("unused_variables"),
47+
"unused variable",
48+
ast,
49+
)
50+
.with_fixes(name_range.and_then(|it| {
51+
fixes(
52+
ctx.sema.db,
53+
var_name,
54+
it.range,
55+
diagnostic_range,
56+
ast.file_id.is_macro(),
57+
is_shorthand_field,
58+
ctx.edition,
59+
)
60+
})),
61+
)
6562
}
6663

6764
fn fixes(
@@ -94,7 +91,6 @@ fn fixes(
9491
}
9592

9693
#[cfg(test)]
97-
#[cfg(false)] // Diagnostic temporarily disabled
9894
mod tests {
9995
use crate::tests::{check_diagnostics, check_fix};
10096

0 commit comments

Comments
 (0)