From d00a285fa757307bbe0f8dac9e49ac247cf9dab1 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sat, 10 Aug 2019 16:40:48 +0100 Subject: Initial implementation of Ok-wrapping --- crates/ra_hir/src/expr/validation.rs | 56 ++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index 62f7d41f5..f5e641557 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -6,11 +6,12 @@ use ra_syntax::ast::{AstNode, RecordLit}; use super::{Expr, ExprId, RecordLitField}; use crate::{ adt::AdtDef, - diagnostics::{DiagnosticSink, MissingFields}, + diagnostics::{DiagnosticSink, MissingFields, MissingOkInTailExpr}, expr::AstPtr, - ty::InferenceResult, + ty::{InferenceResult, Ty, TypeCtor}, Function, HasSource, HirDatabase, Name, Path, }; +use ra_syntax::ast; pub(crate) struct ExprValidator<'a, 'b: 'a> { func: Function, @@ -29,11 +30,23 @@ impl<'a, 'b> ExprValidator<'a, 'b> { pub(crate) fn validate_body(&mut self, db: &impl HirDatabase) { let body = self.func.body(db); + + // The final expr in the function body is the whole body, + // so the expression being returned is the penultimate expr. + let mut penultimate_expr = None; + let mut final_expr = None; + for e in body.exprs() { + penultimate_expr = final_expr; + final_expr = Some(e); + if let (id, Expr::RecordLit { path, fields, spread }) = e { self.validate_record_literal(id, path, fields, *spread, db); } } + if let Some(e) = penultimate_expr { + self.validate_results_in_tail_expr(e.0, db); + } } fn validate_record_literal( @@ -87,4 +100,43 @@ impl<'a, 'b> ExprValidator<'a, 'b> { }) } } + + fn validate_results_in_tail_expr(&mut self, id: ExprId, db: &impl HirDatabase) { + let expr_ty = &self.infer[id]; + let func_ty = self.func.ty(db); + let func_sig = func_ty.callable_sig(db).unwrap(); + let ret = func_sig.ret(); + let ret = match ret { + Ty::Apply(t) => t, + _ => return, + }; + let ret_enum = match ret.ctor { + TypeCtor::Adt(AdtDef::Enum(e)) => e, + _ => return, + }; + let enum_name = ret_enum.name(db); + if enum_name.is_none() || enum_name.unwrap().to_string() != "Result" { + return; + } + let params = &ret.parameters; + if params.len() == 2 && ¶ms[0] == expr_ty { + let source_map = self.func.body_source_map(db); + let file_id = self.func.source(db).file_id; + let parse = db.parse(file_id.original_file(db)); + let source_file = parse.tree(); + let expr_syntax = source_map.expr_syntax(id); + if expr_syntax.is_none() { + return; + } + let expr_syntax = expr_syntax.unwrap(); + let node = expr_syntax.to_node(source_file.syntax()); + let ast = ast::Expr::cast(node); + if ast.is_none() { + return; + } + let ast = ast.unwrap(); + + self.sink.push(MissingOkInTailExpr { file: file_id, expr: AstPtr::new(&ast) }); + } + } } -- cgit v1.2.3 From bacb938ab096e3e2885e7bbb5e2cdbebe53292ea Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 11:40:08 +0100 Subject: Add type_mismatches to InferenceResult and use this in ok-wrapping code fix --- crates/ra_hir/src/expr/validation.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index f5e641557..0e7b08c54 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -102,14 +102,16 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } fn validate_results_in_tail_expr(&mut self, id: ExprId, db: &impl HirDatabase) { - let expr_ty = &self.infer[id]; - let func_ty = self.func.ty(db); - let func_sig = func_ty.callable_sig(db).unwrap(); - let ret = func_sig.ret(); - let ret = match ret { + let mismatch = match self.infer.type_mismatch_for_expr(id) { + Some(m) => m, + None => return, + }; + + let ret = match &mismatch.expected { Ty::Apply(t) => t, _ => return, }; + let ret_enum = match ret.ctor { TypeCtor::Adt(AdtDef::Enum(e)) => e, _ => return, @@ -119,7 +121,7 @@ impl<'a, 'b> ExprValidator<'a, 'b> { return; } let params = &ret.parameters; - if params.len() == 2 && ¶ms[0] == expr_ty { + if params.len() == 2 && ¶ms[0] == &mismatch.actual { let source_map = self.func.body_source_map(db); let file_id = self.func.source(db).file_id; let parse = db.parse(file_id.original_file(db)); -- cgit v1.2.3 From d025016f92866a932729394c22603b615cb458df Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 13:25:36 +0100 Subject: Mock std String and Result types in tests for ok-wrapping diagnostic --- crates/ra_hir/src/expr/validation.rs | 2 -- 1 file changed, 2 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index 0e7b08c54..ca7db61bc 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -106,12 +106,10 @@ impl<'a, 'b> ExprValidator<'a, 'b> { Some(m) => m, None => return, }; - let ret = match &mismatch.expected { Ty::Apply(t) => t, _ => return, }; - let ret_enum = match ret.ctor { TypeCtor::Adt(AdtDef::Enum(e)) => e, _ => return, -- cgit v1.2.3 From a40e390860987a23f9b899abc5947f1525d3709c Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 15:00:37 +0100 Subject: Check type rather than just name in ok-wrapping diagnostic. Add test for handling generic functions (which currently fails) --- crates/ra_hir/src/expr/validation.rs | 46 +++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 8 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index ca7db61bc..339a7b848 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -6,10 +6,13 @@ use ra_syntax::ast::{AstNode, RecordLit}; use super::{Expr, ExprId, RecordLitField}; use crate::{ adt::AdtDef, + code_model::Enum, diagnostics::{DiagnosticSink, MissingFields, MissingOkInTailExpr}, expr::AstPtr, + name, + path::{PathKind, PathSegment}, ty::{InferenceResult, Ty, TypeCtor}, - Function, HasSource, HirDatabase, Name, Path, + Function, HasSource, HirDatabase, ModuleDef, Name, Path, PerNs, Resolution }; use ra_syntax::ast; @@ -106,18 +109,45 @@ impl<'a, 'b> ExprValidator<'a, 'b> { Some(m) => m, None => return, }; - let ret = match &mismatch.expected { - Ty::Apply(t) => t, - _ => return, + + let std_result_path = Path { + kind: PathKind::Abs, + segments: vec![ + PathSegment { name: name::STD, args_and_bindings: None }, + PathSegment { name: name::RESULT_MOD, args_and_bindings: None }, + PathSegment { name: name::RESULT_TYPE, args_and_bindings: None }, + ] }; - let ret_enum = match ret.ctor { - TypeCtor::Adt(AdtDef::Enum(e)) => e, + + let resolver = self.func.resolver(db); + let std_result_enum = match resolver.resolve_path_segments(db, &std_result_path).into_fully_resolved() { + PerNs { types: Some(Resolution::Def(ModuleDef::Enum(e))), .. } => e, _ => return, }; - let enum_name = ret_enum.name(db); - if enum_name.is_none() || enum_name.unwrap().to_string() != "Result" { + + let std_result_type = std_result_enum.ty(db); + + fn enum_from_type(ty: &Ty) -> Option { + match ty { + Ty::Apply(t) => { + match t.ctor { + TypeCtor::Adt(AdtDef::Enum(e)) => Some(e), + _ => None, + } + } + _ => None + } + } + + if enum_from_type(&mismatch.expected) != enum_from_type(&std_result_type) { return; } + + let ret = match &mismatch.expected { + Ty::Apply(t) => t, + _ => return, + }; + let params = &ret.parameters; if params.len() == 2 && ¶ms[0] == &mismatch.actual { let source_map = self.func.body_source_map(db); -- cgit v1.2.3 From 4f6f3933ec04df55a39368d591c91cf81d980237 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 15:04:08 +0100 Subject: cargo format --- crates/ra_hir/src/expr/validation.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index 339a7b848..f0da3d169 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -12,7 +12,7 @@ use crate::{ name, path::{PathKind, PathSegment}, ty::{InferenceResult, Ty, TypeCtor}, - Function, HasSource, HirDatabase, ModuleDef, Name, Path, PerNs, Resolution + Function, HasSource, HirDatabase, ModuleDef, Name, Path, PerNs, Resolution, }; use ra_syntax::ast; @@ -116,26 +116,25 @@ impl<'a, 'b> ExprValidator<'a, 'b> { PathSegment { name: name::STD, args_and_bindings: None }, PathSegment { name: name::RESULT_MOD, args_and_bindings: None }, PathSegment { name: name::RESULT_TYPE, args_and_bindings: None }, - ] + ], }; let resolver = self.func.resolver(db); - let std_result_enum = match resolver.resolve_path_segments(db, &std_result_path).into_fully_resolved() { - PerNs { types: Some(Resolution::Def(ModuleDef::Enum(e))), .. } => e, - _ => return, - }; + let std_result_enum = + match resolver.resolve_path_segments(db, &std_result_path).into_fully_resolved() { + PerNs { types: Some(Resolution::Def(ModuleDef::Enum(e))), .. } => e, + _ => return, + }; let std_result_type = std_result_enum.ty(db); fn enum_from_type(ty: &Ty) -> Option { match ty { - Ty::Apply(t) => { - match t.ctor { - TypeCtor::Adt(AdtDef::Enum(e)) => Some(e), - _ => None, - } - } - _ => None + Ty::Apply(t) => match t.ctor { + TypeCtor::Adt(AdtDef::Enum(e)) => Some(e), + _ => None, + }, + _ => None, } } -- cgit v1.2.3 From c8911e872eb46f811f645190e154b504fac157df Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Mon, 12 Aug 2019 20:21:29 +0100 Subject: Remove reliance on expr ordering --- crates/ra_hir/src/expr/validation.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index f0da3d169..fce1c2f14 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -34,21 +34,15 @@ impl<'a, 'b> ExprValidator<'a, 'b> { pub(crate) fn validate_body(&mut self, db: &impl HirDatabase) { let body = self.func.body(db); - // The final expr in the function body is the whole body, - // so the expression being returned is the penultimate expr. - let mut penultimate_expr = None; - let mut final_expr = None; - for e in body.exprs() { - penultimate_expr = final_expr; - final_expr = Some(e); - if let (id, Expr::RecordLit { path, fields, spread }) = e { self.validate_record_literal(id, path, fields, *spread, db); } } - if let Some(e) = penultimate_expr { - self.validate_results_in_tail_expr(e.0, db); + + let body_expr = &body[body.body_expr()]; + if let Expr::Block { statements: _, tail: Some(t) } = body_expr { + self.validate_results_in_tail_expr(*t, db); } } -- cgit v1.2.3 From 200470692ff1023024f0d4a6c35deb43d3045f10 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Mon, 12 Aug 2019 20:39:11 +0100 Subject: Cast SyntaxNodePtr to AstPtr directly --- crates/ra_hir/src/expr/validation.rs | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index fce1c2f14..e35990d2e 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -145,21 +145,10 @@ impl<'a, 'b> ExprValidator<'a, 'b> { if params.len() == 2 && ¶ms[0] == &mismatch.actual { let source_map = self.func.body_source_map(db); let file_id = self.func.source(db).file_id; - let parse = db.parse(file_id.original_file(db)); - let source_file = parse.tree(); - let expr_syntax = source_map.expr_syntax(id); - if expr_syntax.is_none() { - return; - } - let expr_syntax = expr_syntax.unwrap(); - let node = expr_syntax.to_node(source_file.syntax()); - let ast = ast::Expr::cast(node); - if ast.is_none() { - return; - } - let ast = ast.unwrap(); - self.sink.push(MissingOkInTailExpr { file: file_id, expr: AstPtr::new(&ast) }); + if let Some(expr) = source_map.expr_syntax(id).and_then(|n| n.cast::()) { + self.sink.push(MissingOkInTailExpr { file: file_id, expr }); + } } } } -- cgit v1.2.3 From 6620949caee25128416acf285590b0d5558e4597 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sat, 17 Aug 2019 06:31:53 +0100 Subject: Simplify checking return type, add new test --- crates/ra_hir/src/expr/validation.rs | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) (limited to 'crates/ra_hir/src/expr') diff --git a/crates/ra_hir/src/expr/validation.rs b/crates/ra_hir/src/expr/validation.rs index e35990d2e..5d9d59ff8 100644 --- a/crates/ra_hir/src/expr/validation.rs +++ b/crates/ra_hir/src/expr/validation.rs @@ -6,12 +6,11 @@ use ra_syntax::ast::{AstNode, RecordLit}; use super::{Expr, ExprId, RecordLitField}; use crate::{ adt::AdtDef, - code_model::Enum, diagnostics::{DiagnosticSink, MissingFields, MissingOkInTailExpr}, expr::AstPtr, name, path::{PathKind, PathSegment}, - ty::{InferenceResult, Ty, TypeCtor}, + ty::{ApplicationTy, InferenceResult, Ty, TypeCtor}, Function, HasSource, HirDatabase, ModuleDef, Name, Path, PerNs, Resolution, }; use ra_syntax::ast; @@ -120,28 +119,12 @@ impl<'a, 'b> ExprValidator<'a, 'b> { _ => return, }; - let std_result_type = std_result_enum.ty(db); - - fn enum_from_type(ty: &Ty) -> Option { - match ty { - Ty::Apply(t) => match t.ctor { - TypeCtor::Adt(AdtDef::Enum(e)) => Some(e), - _ => None, - }, - _ => None, - } - } - - if enum_from_type(&mismatch.expected) != enum_from_type(&std_result_type) { - return; - } - - let ret = match &mismatch.expected { - Ty::Apply(t) => t, + let std_result_ctor = TypeCtor::Adt(AdtDef::Enum(std_result_enum)); + let params = match &mismatch.expected { + Ty::Apply(ApplicationTy { ctor, parameters }) if ctor == &std_result_ctor => parameters, _ => return, }; - let params = &ret.parameters; if params.len() == 2 && ¶ms[0] == &mismatch.actual { let source_map = self.func.body_source_map(db); let file_id = self.func.source(db).file_id; -- cgit v1.2.3