11842: Fix duplicate type mismatches with blocks r=flodiebold a=flodiebold

E.g. when there's a type mismatch on the return value of a function. To fix this, we have to return the expected type as the type of the block when there's a mismatch. That meant some IDE code that expected otherwise had to be adapted, in particular the "add return type" assist. For the "wrap in Ok/Some" quickfix, this sadly means it usually can't be applied in all branches of an if expression at the same time anymore, because there's a type mismatch for each branch that has the wrong type.

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
This commit is contained in:
bors[bot] 2022-03-29 16:10:20 +00:00 committed by GitHub
commit 9eb7553d5a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 212 additions and 83 deletions

View file

@ -69,12 +69,11 @@ impl<'a> InferenceContext<'a> {
match self.coerce(Some(expr), &ty, &target) {
Ok(res) => res,
Err(_) => {
self.result
.type_mismatches
.insert(expr.into(), TypeMismatch { expected: target, actual: ty.clone() });
// Return actual type when type mismatch.
// This is needed for diagnostic when return type mismatch.
ty
self.result.type_mismatches.insert(
expr.into(),
TypeMismatch { expected: target.clone(), actual: ty.clone() },
);
target
}
}
} else {
@ -914,9 +913,16 @@ impl<'a> InferenceContext<'a> {
self.table.new_maybe_never_var()
} else {
if let Some(t) = expected.only_has_type(&mut self.table) {
let _ = self.coerce(Some(expr), &TyBuilder::unit(), &t);
if self.coerce(Some(expr), &TyBuilder::unit(), &t).is_err() {
self.result.type_mismatches.insert(
expr.into(),
TypeMismatch { expected: t.clone(), actual: TyBuilder::unit() },
);
}
t
} else {
TyBuilder::unit()
}
TyBuilder::unit()
}
}
}

View file

@ -8,6 +8,7 @@ mod method_resolution;
mod macros;
mod display_source_code;
mod incremental;
mod diagnostics;
use std::{collections::HashMap, env, sync::Arc};

View file

@ -2,12 +2,10 @@ use super::{check, check_no_mismatches, check_types};
#[test]
fn block_expr_type_mismatch() {
// FIXME fix double type mismatch
check(
r"
fn test() {
let a: i32 = { 1i64 };
// ^^^^^^^^ expected i32, got i64
// ^^^^ expected i32, got i64
}
",

View file

@ -0,0 +1,75 @@
use super::check;
#[test]
fn function_return_type_mismatch_1() {
check(
r#"
fn test() -> &'static str {
5
//^ expected &str, got i32
}
"#,
);
}
#[test]
fn function_return_type_mismatch_2() {
check(
r#"
fn test(x: bool) -> &'static str {
if x {
return 1;
//^ expected &str, got i32
}
"ok"
}
"#,
);
}
#[test]
fn function_return_type_mismatch_3() {
check(
r#"
fn test(x: bool) -> &'static str {
if x {
return "ok";
}
1
//^ expected &str, got i32
}
"#,
);
}
#[test]
fn function_return_type_mismatch_4() {
check(
r#"
fn test(x: bool) -> &'static str {
if x {
"ok"
} else {
1
//^ expected &str, got i32
}
}
"#,
);
}
#[test]
fn function_return_type_mismatch_5() {
check(
r#"
fn test(x: bool) -> &'static str {
if x {
1
//^ expected &str, got i32
} else {
"ok"
}
}
"#,
);
}

View file

@ -314,11 +314,10 @@ fn diverging_expression_2() {
expect![[r#"
11..84 '{ ..." }; }': ()
54..55 'x': u32
63..81 '{ loop...foo" }': &str
63..81 '{ loop...foo" }': u32
65..72 'loop {}': !
70..72 '{}': ()
74..79 '"foo"': &str
63..81: expected u32, got &str
74..79: expected u32, got &str
"#]],
);
@ -350,31 +349,30 @@ fn diverging_expression_3_break() {
let x: u32 = { while true { return; }; };
}
",
expect![[r"
expect![[r#"
11..85 '{ ...} }; }': ()
54..55 'x': u32
63..82 '{ loop...k; } }': ()
63..82 '{ loop...k; } }': u32
65..80 'loop { break; }': ()
70..80 '{ break; }': ()
72..77 'break': !
63..82: expected u32, got ()
65..80: expected u32, got ()
97..343 '{ ...; }; }': ()
140..141 'x': u32
149..175 '{ for ...; }; }': ()
149..175 '{ for ...; }; }': u32
151..172 'for a ...eak; }': ()
155..156 'a': {unknown}
160..161 'b': {unknown}
162..172 '{ break; }': ()
164..169 'break': !
226..227 'x': u32
235..253 '{ for ... {}; }': ()
235..253 '{ for ... {}; }': u32
237..250 'for a in b {}': ()
241..242 'a': {unknown}
246..247 'b': {unknown}
248..250 '{}': ()
304..305 'x': u32
313..340 '{ for ...; }; }': ()
313..340 '{ for ...; }; }': u32
315..337 'for a ...urn; }': ()
319..320 'a': {unknown}
324..325 'b': {unknown}
@ -385,18 +383,18 @@ fn diverging_expression_3_break() {
313..340: expected u32, got ()
355..654 '{ ...; }; }': ()
398..399 'x': u32
407..433 '{ whil...; }; }': ()
407..433 '{ whil...; }; }': u32
409..430 'while ...eak; }': ()
415..419 'true': bool
420..430 '{ break; }': ()
422..427 'break': !
537..538 'x': u32
546..564 '{ whil... {}; }': ()
546..564 '{ whil... {}; }': u32
548..561 'while true {}': ()
554..558 'true': bool
559..561 '{}': ()
615..616 'x': u32
624..651 '{ whil...; }; }': ()
624..651 '{ whil...; }; }': u32
626..648 'while ...urn; }': ()
632..636 'true': bool
637..648 '{ return; }': ()
@ -404,7 +402,7 @@ fn diverging_expression_3_break() {
407..433: expected u32, got ()
546..564: expected u32, got ()
624..651: expected u32, got ()
"]],
"#]],
);
}
@ -438,7 +436,7 @@ fn let_else_must_diverge() {
17..18 '1': i32
17..18 '1': i32
21..22 '2': i32
28..30 '{}': ()
28..30 '{}': !
28..30: expected !, got ()
"#]],
);

View file

@ -367,7 +367,7 @@ fn bug_1030() {
}
"#,
expect![[r#"
143..145 '{}': ()
143..145 '{}': HashSet<T, H>
168..197 '{ ...t(); }': ()
174..192 'FxHash...efault': fn default<{unknown}, FxHasher>() -> HashSet<{unknown}, FxHasher>
174..194 'FxHash...ault()': HashSet<{unknown}, FxHasher>
@ -831,7 +831,7 @@ fn issue_4966() {
"#,
expect![[r#"
225..229 'iter': T
244..246 '{}': ()
244..246 '{}': Vec<A>
258..402 '{ ...r(); }': ()
268..273 'inner': Map<|&f64| -> f64>
276..300 'Map { ... 0.0 }': Map<|&f64| -> f64>
@ -914,7 +914,7 @@ fn flush(&self) {
"#,
expect![[r#"
123..127 'self': &Mutex<T>
150..152 '{}': ()
150..152 '{}': MutexGuard<T>
234..238 'self': &{unknown}
240..290 '{ ...()); }': ()
250..251 'w': &Mutex<BufWriter>
@ -1039,18 +1039,18 @@ fn cfg_tail() {
}
"#,
expect![[r#"
14..53 '{ ...)] 9 }': &str
20..31 '{ "first" }': &str
14..53 '{ ...)] 9 }': ()
20..31 '{ "first" }': ()
22..29 '"first"': &str
72..190 '{ ...] 13 }': &str
72..190 '{ ...] 13 }': ()
78..88 '{ "fake" }': &str
80..86 '"fake"': &str
93..103 '{ "fake" }': &str
95..101 '"fake"': &str
108..120 '{ "second" }': &str
108..120 '{ "second" }': ()
110..118 '"second"': &str
210..273 '{ ... 15; }': &str
216..227 '{ "third" }': &str
210..273 '{ ... 15; }': ()
216..227 '{ "third" }': ()
218..225 '"third"': &str
293..357 '{ ...] 15 }': ()
299..311 '{ "fourth" }': &str

View file

@ -996,9 +996,9 @@ fn main(foo: Foo) {
50..106 'if tru... }': ()
53..57 'true': bool
58..66 '{ }': ()
72..106 'if fal... }': i32
72..106 'if fal... }': ()
75..80 'false': bool
81..106 '{ ... }': i32
81..106 '{ ... }': ()
91..94 'foo': Foo
91..100 'foo.field': i32
"#]],
@ -1094,10 +1094,10 @@ fn infer_inherent_method() {
expect![[r#"
31..35 'self': A
37..38 'x': u32
52..54 '{}': ()
52..54 '{}': i32
106..110 'self': &A
112..113 'x': u64
127..129 '{}': ()
127..129 '{}': i64
147..148 'a': A
153..201 '{ ...(1); }': ()
159..160 'a': A
@ -1129,7 +1129,7 @@ fn infer_inherent_method_str() {
"#,
expect![[r#"
39..43 'self': &str
52..54 '{}': ()
52..54 '{}': i32
68..88 '{ ...o(); }': ()
74..79 '"foo"': &str
74..85 '"foo".foo()': i32
@ -1419,7 +1419,7 @@ fn infer_impl_generics_basic() {
206..210 'self': A<X, Y>
206..212 'self.y': Y
214..215 't': T
244..341 '{ ...(1); }': ()
244..341 '{ ...(1); }': i128
254..255 'a': A<u64, i64>
258..280 'A { x:...1i64 }': A<u64, i64>
265..269 '1u64': u64
@ -1456,7 +1456,7 @@ fn infer_impl_generics_with_autoderef() {
"#,
expect![[r#"
77..81 'self': &Option<T>
97..99 '{}': ()
97..99 '{}': Option<&T>
110..111 'o': Option<u32>
126..164 '{ ...f(); }': ()
132..145 '(&o).as_ref()': Option<&u32>
@ -1852,7 +1852,7 @@ fn closure_return() {
}
"#,
expect![[r#"
16..58 '{ ...; }; }': ()
16..58 '{ ...; }; }': u32
26..27 'x': || -> usize
30..55 '|| -> ...n 1; }': || -> usize
42..55 '{ return 1; }': usize
@ -1871,7 +1871,7 @@ fn closure_return_unit() {
}
"#,
expect![[r#"
16..47 '{ ...; }; }': ()
16..47 '{ ...; }; }': u32
26..27 'x': || -> ()
30..44 '|| { return; }': || -> ()
33..44 '{ return; }': ()
@ -1889,7 +1889,7 @@ fn closure_return_inferred() {
}
"#,
expect![[r#"
16..46 '{ ..." }; }': ()
16..46 '{ ..." }; }': u32
26..27 'x': || -> &str
30..43 '|| { "test" }': || -> &str
33..43 '{ "test" }': &str
@ -2628,11 +2628,11 @@ fn main() {
expect![[r#"
104..108 'self': &Box<T>
188..192 'self': &Box<Foo<T>>
218..220 '{}': ()
218..220 '{}': &T
242..246 'self': &Box<Foo<T>>
275..277 '{}': ()
275..277 '{}': &Foo<T>
297..301 'self': Box<Foo<T>>
322..324 '{}': ()
322..324 '{}': Foo<T>
338..559 '{ ...r(); }': ()
348..353 'boxed': Box<Foo<i32>>
356..359 'Box': Box<Foo<i32>>(Foo<i32>) -> Box<Foo<i32>>

View file

@ -1280,7 +1280,7 @@ fn test(x: dyn Trait<u64>, y: &dyn Trait<u64>) {
expect![[r#"
29..33 'self': &Self
54..58 'self': &Self
97..99 '{}': ()
97..99 '{}': dyn Trait<u64>
109..110 'x': dyn Trait<u64>
128..129 'y': &dyn Trait<u64>
148..265 '{ ...2(); }': ()
@ -1361,10 +1361,10 @@ fn test(x: Trait, y: &Trait) -> u64 {
}"#,
expect![[r#"
26..30 'self': &Self
60..62 '{}': ()
60..62 '{}': dyn Trait
72..73 'x': dyn Trait
82..83 'y': &dyn Trait
100..175 '{ ...o(); }': ()
100..175 '{ ...o(); }': u64
106..107 'x': dyn Trait
113..114 'y': &dyn Trait
124..125 'z': dyn Trait
@ -1449,9 +1449,9 @@ fn test<T: Trait<Type = u32>>(x: T, y: impl Trait<Type = i64>) {
}"#,
expect![[r#"
49..50 't': T
77..79 '{}': ()
77..79 '{}': Trait::Type<T>
111..112 't': T
122..124 '{}': ()
122..124 '{}': U
154..155 't': T
165..168 '{t}': T
166..167 't': T
@ -1575,7 +1575,7 @@ fn test<T: Trait1, U: Trait2>(x: T, y: U) {
}"#,
expect![[r#"
49..53 'self': &Self
62..64 '{}': ()
62..64 '{}': u32
181..182 'x': T
187..188 'y': U
193..222 '{ ...o(); }': ()
@ -1604,7 +1604,7 @@ fn test(x: &impl Trait1) {
}"#,
expect![[r#"
49..53 'self': &Self
62..64 '{}': ()
62..64 '{}': u32
115..116 'x': &impl Trait1
132..148 '{ ...o(); }': ()
138..139 'x': &impl Trait1
@ -1653,7 +1653,7 @@ fn test() {
}"#,
expect![[r#"
102..103 't': T
113..115 '{}': ()
113..115 '{}': U
145..146 't': T
156..159 '{t}': T
157..158 't': T
@ -1786,9 +1786,9 @@ fn test() {
}"#,
expect![[r#"
36..40 'self': &Foo
51..53 '{}': ()
51..53 '{}': usize
131..132 'f': F
151..153 '{}': ()
151..153 '{}': Lazy<T, F>
251..497 '{ ...o(); }': ()
261..266 'lazy1': Lazy<Foo, || -> Foo>
283..292 'Lazy::new': fn new<Foo, || -> Foo>(|| -> Foo) -> Lazy<Foo, || -> Foo>
@ -1807,7 +1807,7 @@ fn test() {
478..480 'r2': usize
483..488 'lazy2': Lazy<Foo, fn() -> Foo>
483..494 'lazy2.foo()': usize
357..359 '{}': ()
357..359 '{}': Foo
"#]],
);
}
@ -2738,7 +2738,7 @@ fn test() {
expect![[r#"
9..11 '{}': ()
28..29 'T': {unknown}
36..38 '{}': ()
36..38 '{}': T
36..38: expected T, got ()
113..117 'self': &Self
169..249 '{ ...t(); }': ()
@ -3167,7 +3167,7 @@ fn f() {
}"#,
expect![[r#"
17..73 '{ ... } }': ()
39..71 '{ ... }': ()
39..71 '{ ... }': S
53..54 's': S
57..62 'inner': fn inner() -> S
57..64 'inner()': S

View file

@ -1,5 +1,5 @@
use hir::HirDisplay;
use syntax::{ast, AstNode, SyntaxKind, SyntaxToken, TextRange, TextSize};
use syntax::{ast, match_ast, AstNode, SyntaxKind, SyntaxToken, TextRange, TextSize};
use crate::{AssistContext, AssistId, AssistKind, Assists};
@ -18,7 +18,7 @@ use crate::{AssistContext, AssistId, AssistKind, Assists};
pub(crate) fn add_return_type(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
let (fn_type, tail_expr, builder_edit_pos) = extract_tail(ctx)?;
let module = ctx.sema.scope(tail_expr.syntax()).module()?;
let ty = ctx.sema.type_of_expr(&tail_expr)?.adjusted();
let ty = ctx.sema.type_of_expr(&peel_blocks(tail_expr.clone()))?.original();
if ty.is_unit() {
return None;
}
@ -93,6 +93,45 @@ enum FnType {
Closure { wrap_expr: bool },
}
/// If we're looking at a block that is supposed to return `()`, type inference
/// will just tell us it has type `()`. We have to look at the tail expression
/// to see the mismatched actual type. This 'unpeels' the various blocks to
/// hopefully let us see the type the user intends. (This still doesn't handle
/// all situations fully correctly; the 'ideal' way to handle this would be to
/// run type inference on the function again, but with a variable as the return
/// type.)
fn peel_blocks(mut expr: ast::Expr) -> ast::Expr {
loop {
match_ast! {
match (expr.syntax()) {
ast::BlockExpr(it) => {
if let Some(tail) = it.tail_expr() {
expr = tail.clone();
} else {
break;
}
},
ast::IfExpr(it) => {
if let Some(then_branch) = it.then_branch() {
expr = ast::Expr::BlockExpr(then_branch.clone());
} else {
break;
}
},
ast::MatchExpr(it) => {
if let Some(arm_expr) = it.match_arm_list().and_then(|l| l.arms().next()).and_then(|a| a.expr()) {
expr = arm_expr;
} else {
break;
}
},
_ => break,
}
}
}
expr
}
fn extract_tail(ctx: &AssistContext) -> Option<(FnType, ast::Expr, InsertOrReplace)> {
let (fn_type, tail_expr, return_type_range, action) =
if let Some(closure) = ctx.find_node_at_offset::<ast::ClosureExpr>() {
@ -248,6 +287,25 @@ mod tests {
);
}
#[test]
fn infer_return_type_nested_match() {
check_assist(
add_return_type,
r#"fn foo() {
match true {
true => { 3$0 },
false => { 5 },
}
}"#,
r#"fn foo() -> i32 {
match true {
true => { 3 },
false => { 5 },
}
}"#,
);
}
#[test]
fn not_applicable_ret_type_specified() {
cov_mark::check!(existing_ret_type);

View file

@ -4162,7 +4162,7 @@ fn main() {
match 6 {
100 => $0{ 100 }$0
_ => 0,
}
};
}
"#,
r#"
@ -4170,7 +4170,7 @@ fn main() {
match 6 {
100 => fun_name(),
_ => 0,
}
};
}
fn $0fun_name() -> i32 {
@ -4185,7 +4185,7 @@ fn main() {
match 6 {
100 => $0{ 100 }$0,
_ => 0,
}
};
}
"#,
r#"
@ -4193,7 +4193,7 @@ fn main() {
match 6 {
100 => fun_name(),
_ => 0,
}
};
}
fn $0fun_name() -> i32 {

View file

@ -1,8 +1,5 @@
use hir::{db::AstDatabase, HirDisplay, Type, TypeInfo};
use ide_db::{
famous_defs::FamousDefs, source_change::SourceChange,
syntax_helpers::node_ext::for_each_tail_expr,
};
use hir::{db::AstDatabase, HirDisplay, Type};
use ide_db::{famous_defs::FamousDefs, source_change::SourceChange};
use syntax::{
ast::{BlockExpr, ExprStmt},
AstNode,
@ -77,9 +74,9 @@ fn add_missing_ok_or_some(
acc: &mut Vec<Assist>,
) -> Option<()> {
let root = ctx.sema.db.parse_or_expand(d.expr.file_id)?;
let tail_expr = d.expr.value.to_node(&root);
let tail_expr_range = tail_expr.syntax().text_range();
let scope = ctx.sema.scope(tail_expr.syntax());
let expr = d.expr.value.to_node(&root);
let expr_range = expr.syntax().text_range();
let scope = ctx.sema.scope(expr.syntax());
let expected_adt = d.expected.as_adt()?;
let expected_enum = expected_adt.as_enum()?;
@ -101,16 +98,12 @@ fn add_missing_ok_or_some(
}
let mut builder = TextEdit::builder();
for_each_tail_expr(&tail_expr, &mut |expr| {
if ctx.sema.type_of_expr(expr).map(TypeInfo::adjusted).as_ref() != Some(&d.expected) {
builder.insert(expr.syntax().text_range().start(), format!("{}(", variant_name));
builder.insert(expr.syntax().text_range().end(), ")".to_string());
}
});
builder.insert(expr.syntax().text_range().start(), format!("{}(", variant_name));
builder.insert(expr.syntax().text_range().end(), ")".to_string());
let source_change =
SourceChange::from_text_edit(d.expr.file_id.original_file(ctx.sema.db), builder.finish());
let name = format!("Wrap in {}", variant_name);
acc.push(fix("wrap_tail_expr", &name, source_change, tail_expr_range));
acc.push(fix("wrap_in_constructor", &name, source_change, expr_range));
Some(())
}
@ -330,12 +323,12 @@ fn div(x: i32, y: i32) -> Option<i32> {
//- minicore: option, result
fn div(x: i32, y: i32) -> Option<i32> {
if y == 0 {
0
Some(0)
} else if true {
100
100$0
} else {
None
}$0
}
}
"#,
r#"

View file

@ -54,13 +54,13 @@ fn check_nth_fix(nth: usize, ra_fixture_before: &str, ra_fixture_after: &str) {
actual
};
assert_eq_text!(&after, &actual);
assert!(
fix.target.contains_inclusive(file_position.offset),
"diagnostic fix range {:?} does not touch cursor position {:?}",
fix.target,
file_position.offset
);
assert_eq_text!(&after, &actual);
}
/// Checks that there's a diagnostic *without* fix at `$0`.