From: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Date: Tue, 29 Mar 2022 16:10:20 +0000 (+0000) Subject: Merge #11842 X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=9eb7553d5ab7cd512b7edf8d8a30b191be0dc761;hp=89d495eb306544ecb1c3e9471725cefb4c87d203;p=rust.git Merge #11842 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 --- diff --git a/crates/hir_ty/src/infer/expr.rs b/crates/hir_ty/src/infer/expr.rs index 238aff75c45..0b67f2c32e5 100644 --- a/crates/hir_ty/src/infer/expr.rs +++ b/crates/hir_ty/src/infer/expr.rs @@ -69,12 +69,11 @@ pub(super) fn infer_expr_coerce(&mut self, expr: ExprId, expected: &Expectation) 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 @@ fn infer_block( 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() } } } diff --git a/crates/hir_ty/src/tests.rs b/crates/hir_ty/src/tests.rs index 18523d2db59..d2f13e4351c 100644 --- a/crates/hir_ty/src/tests.rs +++ b/crates/hir_ty/src/tests.rs @@ -8,6 +8,7 @@ mod macros; mod display_source_code; mod incremental; +mod diagnostics; use std::{collections::HashMap, env, sync::Arc}; diff --git a/crates/hir_ty/src/tests/coercion.rs b/crates/hir_ty/src/tests/coercion.rs index c0dddb608ea..268faf8cb3a 100644 --- a/crates/hir_ty/src/tests/coercion.rs +++ b/crates/hir_ty/src/tests/coercion.rs @@ -2,12 +2,10 @@ #[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 } ", diff --git a/crates/hir_ty/src/tests/diagnostics.rs b/crates/hir_ty/src/tests/diagnostics.rs new file mode 100644 index 00000000000..f00fa972948 --- /dev/null +++ b/crates/hir_ty/src/tests/diagnostics.rs @@ -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" + } +} +"#, + ); +} diff --git a/crates/hir_ty/src/tests/never_type.rs b/crates/hir_ty/src/tests/never_type.rs index 8be2b8ec108..3ca0a5b391d 100644 --- a/crates/hir_ty/src/tests/never_type.rs +++ b/crates/hir_ty/src/tests/never_type.rs @@ -314,11 +314,10 @@ fn test1() { 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 test3() { 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 test3() { 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 test3() { 407..433: expected u32, got () 546..564: expected u32, got () 624..651: expected u32, got () - "]], + "#]], ); } @@ -438,7 +436,7 @@ fn f() { 17..18 '1': i32 17..18 '1': i32 21..22 '2': i32 - 28..30 '{}': () + 28..30 '{}': ! 28..30: expected !, got () "#]], ); diff --git a/crates/hir_ty/src/tests/regression.rs b/crates/hir_ty/src/tests/regression.rs index 14c6af943ab..2809b1e912a 100644 --- a/crates/hir_ty/src/tests/regression.rs +++ b/crates/hir_ty/src/tests/regression.rs @@ -367,7 +367,7 @@ pub fn main_loop() { } "#, expect![[r#" - 143..145 '{}': () + 143..145 '{}': HashSet 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 main() { "#, expect![[r#" 225..229 'iter': T - 244..246 '{}': () + 244..246 '{}': Vec 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 - 150..152 '{}': () + 150..152 '{}': MutexGuard 234..238 'self': &{unknown} 240..290 '{ ...()); }': () 250..251 'w': &Mutex @@ -1039,18 +1039,18 @@ fn no_actual_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 diff --git a/crates/hir_ty/src/tests/simple.rs b/crates/hir_ty/src/tests/simple.rs index 675f9038f09..df7b3df3d5b 100644 --- a/crates/hir_ty/src/tests/simple.rs +++ b/crates/hir_ty/src/tests/simple.rs @@ -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 test(a: A) { 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 test() { "#, 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 test() -> i128 { 206..210 'self': A 206..212 'self.y': Y 214..215 't': T - 244..341 '{ ...(1); }': () + 244..341 '{ ...(1); }': i128 254..255 'a': A 258..280 'A { x:...1i64 }': A 265..269 '1u64': u64 @@ -1456,7 +1456,7 @@ fn test(o: Option) { "#, expect![[r#" 77..81 'self': &Option - 97..99 '{}': () + 97..99 '{}': Option<&T> 110..111 'o': Option 126..164 '{ ...f(); }': () 132..145 '(&o).as_ref()': Option<&u32> @@ -1852,7 +1852,7 @@ fn foo() -> u32 { } "#, 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 foo() -> u32 { } "#, expect![[r#" - 16..47 '{ ...; }; }': () + 16..47 '{ ...; }; }': u32 26..27 'x': || -> () 30..44 '|| { return; }': || -> () 33..44 '{ return; }': () @@ -1889,7 +1889,7 @@ fn foo() -> u32 { } "#, 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 188..192 'self': &Box> - 218..220 '{}': () + 218..220 '{}': &T 242..246 'self': &Box> - 275..277 '{}': () + 275..277 '{}': &Foo 297..301 'self': Box> - 322..324 '{}': () + 322..324 '{}': Foo 338..559 '{ ...r(); }': () 348..353 'boxed': Box> 356..359 'Box': Box>(Foo) -> Box> diff --git a/crates/hir_ty/src/tests/traits.rs b/crates/hir_ty/src/tests/traits.rs index a82b8cb466f..5e58d5ad838 100644 --- a/crates/hir_ty/src/tests/traits.rs +++ b/crates/hir_ty/src/tests/traits.rs @@ -1280,7 +1280,7 @@ fn test(x: dyn Trait, y: &dyn Trait) { expect![[r#" 29..33 'self': &Self 54..58 'self': &Self - 97..99 '{}': () + 97..99 '{}': dyn Trait 109..110 'x': dyn Trait 128..129 'y': &dyn Trait 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>(x: T, y: impl Trait) { }"#, expect![[r#" 49..50 't': T - 77..79 '{}': () + 77..79 '{}': Trait::Type 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(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 make_foo_fn() -> Foo {} }"#, expect![[r#" 36..40 'self': &Foo - 51..53 '{}': () + 51..53 '{}': usize 131..132 'f': F - 151..153 '{}': () + 151..153 '{}': Lazy 251..497 '{ ...o(); }': () 261..266 'lazy1': Lazy Foo> 283..292 'Lazy::new': fn new Foo>(|| -> Foo) -> Lazy Foo> @@ -1807,7 +1807,7 @@ fn make_foo_fn() -> Foo {} 478..480 'r2': usize 483..488 'lazy2': Lazy 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 inner() -> S { }"#, expect![[r#" 17..73 '{ ... } }': () - 39..71 '{ ... }': () + 39..71 '{ ... }': S 53..54 's': S 57..62 'inner': fn inner() -> S 57..64 'inner()': S diff --git a/crates/ide_assists/src/handlers/add_return_type.rs b/crates/ide_assists/src/handlers/add_return_type.rs index 2c5b61eddb7..c7172741e46 100644 --- a/crates/ide_assists/src/handlers/add_return_type.rs +++ b/crates/ide_assists/src/handlers/add_return_type.rs @@ -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 @@ 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::() { @@ -248,6 +287,25 @@ fn infer_return_type_nested() { ); } + #[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); diff --git a/crates/ide_assists/src/handlers/extract_function.rs b/crates/ide_assists/src/handlers/extract_function.rs index 8352774be42..fcb4aaf065d 100644 --- a/crates/ide_assists/src/handlers/extract_function.rs +++ b/crates/ide_assists/src/handlers/extract_function.rs @@ -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 { diff --git a/crates/ide_diagnostics/src/handlers/type_mismatch.rs b/crates/ide_diagnostics/src/handlers/type_mismatch.rs index 040dcbd1d95..8ae9a0be538 100644 --- a/crates/ide_diagnostics/src/handlers/type_mismatch.rs +++ b/crates/ide_diagnostics/src/handlers/type_mismatch.rs @@ -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, ) -> 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 test_wrap_return_type_option_tails() { //- minicore: option, result fn div(x: i32, y: i32) -> Option { if y == 0 { - 0 + Some(0) } else if true { - 100 + 100$0 } else { None - }$0 + } } "#, r#" diff --git a/crates/ide_diagnostics/src/tests.rs b/crates/ide_diagnostics/src/tests.rs index 7d06e9d36ff..7cd79c7ceef 100644 --- a/crates/ide_diagnostics/src/tests.rs +++ b/crates/ide_diagnostics/src/tests.rs @@ -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`.