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/diagnostics.rs | 31 ++++++++++++++++++++ crates/ra_hir/src/expr/validation.rs | 56 ++++++++++++++++++++++++++++++++++-- crates/ra_hir/src/ty.rs | 2 +- crates/ra_ide_api/src/diagnostics.rs | 50 ++++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 3 deletions(-) (limited to 'crates') diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index 301109cb8..718345d75 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -143,3 +143,34 @@ impl AstDiagnostic for MissingFields { ast::RecordFieldList::cast(node).unwrap() } } + +#[derive(Debug)] +pub struct MissingOkInTailExpr { + pub file: HirFileId, + pub expr: AstPtr, +} + +impl Diagnostic for MissingOkInTailExpr { + fn message(&self) -> String { + "wrap return expression in Ok".to_string() + } + fn file(&self) -> HirFileId { + self.file + } + fn syntax_node_ptr(&self) -> SyntaxNodePtr { + self.expr.into() + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + +impl AstDiagnostic for MissingOkInTailExpr { + type AST = ast::Expr; + + fn ast(&self, db: &impl HirDatabase) -> Self::AST { + let root = db.parse_or_expand(self.file()).unwrap(); + let node = self.syntax_node_ptr().to_node(&root); + ast::Expr::cast(node).unwrap() + } +} 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) }); + } + } } diff --git a/crates/ra_hir/src/ty.rs b/crates/ra_hir/src/ty.rs index b54c80318..0efd94cef 100644 --- a/crates/ra_hir/src/ty.rs +++ b/crates/ra_hir/src/ty.rs @@ -516,7 +516,7 @@ impl Ty { } } - fn callable_sig(&self, db: &impl HirDatabase) -> Option { + pub fn callable_sig(&self, db: &impl HirDatabase) -> Option { match self { Ty::Apply(a_ty) => match a_ty.ctor { TypeCtor::FnPtr { .. } => Some(FnSig::from_fn_ptr_substs(&a_ty.parameters)), diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index c2b959cb3..be5197767 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -75,6 +75,19 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec severity: Severity::Error, fix: Some(fix), }) + }) + .on::(|d| { + let node = d.ast(db); + let mut builder = TextEditBuilder::default(); + let replacement = format!("Ok({})", node.syntax().text()); + builder.replace(node.syntax().text_range(), replacement); + let fix = SourceChange::source_file_edit_from("wrap with ok", file_id, builder.finish()); + res.borrow_mut().push(Diagnostic { + range: d.highlight_range(), + message: d.message(), + severity: Severity::Error, + fix: Some(fix), + }) }); if let Some(m) = source_binder::module_from_file_id(db, file_id) { m.diagnostics(db, &mut sink); @@ -218,6 +231,43 @@ mod tests { assert_eq!(diagnostics.len(), 0); } + #[test] + fn test_wrap_return_type() { + let before = r#" + enum Result { Ok(T), Err(E) } + struct String { } + + fn div(x: i32, y: i32) -> Result { + if y == 0 { + return Err("div by zero".into()); + } + x / y + } + "#; + let after = r#" + enum Result { Ok(T), Err(E) } + struct String { } + + fn div(x: i32, y: i32) -> Result { + if y == 0 { + return Err("div by zero".into()); + } + Ok(x / y) + } + "#; + check_apply_diagnostic_fix(before, after); + } + + #[test] + fn test_wrap_return_type_not_applicable() { + let content = r#" + fn foo() -> Result { + 0 + } + "#; + check_no_diagnostic(content); + } + #[test] fn test_fill_struct_fields_empty() { let before = r" -- 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 ++++++++------ crates/ra_hir/src/ty.rs | 2 +- crates/ra_hir/src/ty/infer.rs | 19 ++++++++++++++++++- 3 files changed, 27 insertions(+), 8 deletions(-) (limited to 'crates') 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)); diff --git a/crates/ra_hir/src/ty.rs b/crates/ra_hir/src/ty.rs index 0efd94cef..b54c80318 100644 --- a/crates/ra_hir/src/ty.rs +++ b/crates/ra_hir/src/ty.rs @@ -516,7 +516,7 @@ impl Ty { } } - pub fn callable_sig(&self, db: &impl HirDatabase) -> Option { + fn callable_sig(&self, db: &impl HirDatabase) -> Option { match self { Ty::Apply(a_ty) => match a_ty.ctor { TypeCtor::FnPtr { .. } => Some(FnSig::from_fn_ptr_substs(&a_ty.parameters)), diff --git a/crates/ra_hir/src/ty/infer.rs b/crates/ra_hir/src/ty/infer.rs index b33de5687..d94e8154b 100644 --- a/crates/ra_hir/src/ty/infer.rs +++ b/crates/ra_hir/src/ty/infer.rs @@ -106,6 +106,13 @@ impl Default for BindingMode { } } +/// A mismatch between an expected and an inferred type. +#[derive(Clone, PartialEq, Eq, Debug, Hash)] +pub struct TypeMismatch { + pub expected: Ty, + pub actual: Ty, +} + /// The result of type inference: A mapping from expressions and patterns to types. #[derive(Clone, PartialEq, Eq, Debug, Default)] pub struct InferenceResult { @@ -120,6 +127,7 @@ pub struct InferenceResult { diagnostics: Vec, pub(super) type_of_expr: ArenaMap, pub(super) type_of_pat: ArenaMap, + pub(super) type_mismatches: ArenaMap, } impl InferenceResult { @@ -141,6 +149,9 @@ impl InferenceResult { pub fn assoc_resolutions_for_pat(&self, id: PatId) -> Option { self.assoc_resolutions.get(&id.into()).copied() } + pub fn type_mismatch_for_expr(&self, expr: ExprId) -> Option<&TypeMismatch> { + self.type_mismatches.get(expr) + } pub(crate) fn add_diagnostics( &self, db: &impl HirDatabase, @@ -1345,9 +1356,15 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> { }; // use a new type variable if we got Ty::Unknown here let ty = self.insert_type_vars_shallow(ty); - self.unify(&ty, &expected.ty); + let could_unify = self.unify(&ty, &expected.ty); let ty = self.resolve_ty_as_possible(&mut vec![], ty); self.write_expr_ty(tgt_expr, ty.clone()); + if !could_unify { + self.result.type_mismatches.insert( + tgt_expr, + TypeMismatch { expected: expected.ty.clone(), actual: ty.clone() }, + ); + } ty } -- 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 -- crates/ra_ide_api/src/diagnostics.rs | 62 ++++++++++++++++++++++++++-------- crates/ra_ide_api/src/mock_analysis.rs | 17 ++++++++++ 3 files changed, 65 insertions(+), 16 deletions(-) (limited to 'crates') 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, diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index be5197767..84d2b7fb1 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -187,7 +187,7 @@ mod tests { use ra_syntax::SourceFile; use test_utils::assert_eq_text; - use crate::mock_analysis::single_file; + use crate::mock_analysis::{fixture_with_target_file, single_file}; use super::*; @@ -216,6 +216,15 @@ mod tests { assert_eq_text!(after, &actual); } + fn check_apply_diagnostic_fix_for_target_file(target_file: &str, fixture: &str, after: &str) { + let (analysis, file_id, target_file_contents) = fixture_with_target_file(fixture, target_file); + let diagnostic = analysis.diagnostics(file_id).unwrap().pop().unwrap(); + let mut fix = diagnostic.fix.unwrap(); + let edit = fix.source_file_edits.pop().unwrap().edit; + let actual = edit.apply(&target_file_contents); + assert_eq_text!(after, &actual); + } + fn check_apply_diagnostic_fix(before: &str, after: &str) { let (analysis, file_id) = single_file(before); let diagnostic = analysis.diagnostics(file_id).unwrap().pop().unwrap(); @@ -225,6 +234,12 @@ mod tests { assert_eq_text!(after, &actual); } + fn check_no_diagnostic_for_target_file(target_file: &str, fixture: &str) { + let (analysis, file_id, _) = fixture_with_target_file(fixture, target_file); + let diagnostics = analysis.diagnostics(file_id).unwrap(); + assert_eq!(diagnostics.len(), 0); + } + fn check_no_diagnostic(content: &str) { let (analysis, file_id) = single_file(content); let diagnostics = analysis.diagnostics(file_id).unwrap(); @@ -234,8 +249,8 @@ mod tests { #[test] fn test_wrap_return_type() { let before = r#" - enum Result { Ok(T), Err(E) } - struct String { } + //- /main.rs + use std::{string::String, result::Result::{self, Ok, Err}}; fn div(x: i32, y: i32) -> Result { if y == 0 { @@ -243,29 +258,48 @@ mod tests { } x / y } - "#; - let after = r#" - enum Result { Ok(T), Err(E) } - struct String { } - fn div(x: i32, y: i32) -> Result { - if y == 0 { - return Err("div by zero".into()); - } - Ok(x / y) + //- /std/lib.rs + pub mod string { + pub struct String { } + } + pub mod result { + pub enum Result { Ok(T), Err(E) } } "#; - check_apply_diagnostic_fix(before, after); +// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - +// it strips empty lines and leading whitespace. The important part of this test is that the final +// `x / y` expr is now wrapped in `Ok(..)` + let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; +fn div(x: i32, y: i32) -> Result { + if y == 0 { + return Err("div by zero".into()); + } + Ok(x / y) +} +"#; + check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); } #[test] fn test_wrap_return_type_not_applicable() { let content = r#" + //- /main.rs + use std::{string::String, result::Result::{self, Ok, Err}}; + fn foo() -> Result { 0 } + + //- /std/lib.rs + pub mod string { + pub struct String { } + } + pub mod result { + pub enum Result { Ok(T), Err(E) } + } "#; - check_no_diagnostic(content); + check_no_diagnostic_for_target_file("/main.rs", content); } #[test] diff --git a/crates/ra_ide_api/src/mock_analysis.rs b/crates/ra_ide_api/src/mock_analysis.rs index 132f6f875..0eaf5d15a 100644 --- a/crates/ra_ide_api/src/mock_analysis.rs +++ b/crates/ra_ide_api/src/mock_analysis.rs @@ -80,6 +80,15 @@ impl MockAnalysis { .expect("no file in this mock"); FileId(idx as u32 + 1) } + pub fn id_and_contents_of(&self, path: &str) -> (FileId, String) { + let (idx, contents) = self + .files + .iter() + .enumerate() + .find(|(_, (p, _text))| path == p) + .expect("no file in this mock"); + (FileId(idx as u32 + 1), contents.1.to_string()) + } pub fn analysis_host(self) -> AnalysisHost { let mut host = AnalysisHost::default(); let source_root = SourceRootId(0); @@ -124,6 +133,14 @@ pub fn single_file(code: &str) -> (Analysis, FileId) { (mock.analysis(), file_id) } +/// Creates analysis from a fixture with multiple files +/// and returns the file id and contents of the target file. +pub fn fixture_with_target_file(fixture: &str, target_file: &str) -> (Analysis, FileId, String) { + let mock = MockAnalysis::with_files(fixture); + let (target_file_id, target_file_contents) = mock.id_and_contents_of(target_file); + (mock.analysis(), target_file_id, target_file_contents) +} + /// Creates analysis for a single file, returns position marked with <|>. pub fn single_file_with_position(code: &str) -> (Analysis, FilePosition) { let mut mock = MockAnalysis::new(); -- cgit v1.2.3 From 62c2002e2b21b3a74a4e2205ccc40fa93f722b34 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 13:47:33 +0100 Subject: Add test that ok-wrapping handles type aliases --- crates/ra_ide_api/src/diagnostics.rs | 38 ++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) (limited to 'crates') diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 84d2b7fb1..5e25991c6 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -281,6 +281,44 @@ fn div(x: i32, y: i32) -> Result { check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); } + #[test] + fn test_wrap_return_type_handles_type_aliases() { + let before = r#" + //- /main.rs + use std::{string::String, result::Result::{self, Ok, Err}}; + + type MyResult = Result; + + fn div(x: i32, y: i32) -> MyResult { + if y == 0 { + return Err("div by zero".into()); + } + x / y + } + + //- /std/lib.rs + pub mod string { + pub struct String { } + } + pub mod result { + pub enum Result { Ok(T), Err(E) } + } + "#; +// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - +// it strips empty lines and leading whitespace. The important part of this test is that the final +// `x / y` expr is now wrapped in `Ok(..)` + let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; +type MyResult = Result; +fn div(x: i32, y: i32) -> MyResult { + if y == 0 { + return Err("div by zero".into()); + } + Ok(x / y) +} +"#; + check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); + } + #[test] fn test_wrap_return_type_not_applicable() { let content = r#" -- 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 +++++++++++++++++++++++++++++------- crates/ra_hir/src/name.rs | 2 ++ crates/ra_ide_api/src/diagnostics.rs | 37 +++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 8 deletions(-) (limited to 'crates') 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); diff --git a/crates/ra_hir/src/name.rs b/crates/ra_hir/src/name.rs index 6d14eea8e..9c4822d91 100644 --- a/crates/ra_hir/src/name.rs +++ b/crates/ra_hir/src/name.rs @@ -120,6 +120,8 @@ pub(crate) const TRY: Name = Name::new(SmolStr::new_inline_from_ascii(3, b"Try") pub(crate) const OK: Name = Name::new(SmolStr::new_inline_from_ascii(2, b"Ok")); pub(crate) const FUTURE_MOD: Name = Name::new(SmolStr::new_inline_from_ascii(6, b"future")); pub(crate) const FUTURE_TYPE: Name = Name::new(SmolStr::new_inline_from_ascii(6, b"Future")); +pub(crate) const RESULT_MOD: Name = Name::new(SmolStr::new_inline_from_ascii(6, b"result")); +pub(crate) const RESULT_TYPE: Name = Name::new(SmolStr::new_inline_from_ascii(6, b"Result")); pub(crate) const OUTPUT: Name = Name::new(SmolStr::new_inline_from_ascii(6, b"Output")); fn resolve_name(text: &SmolStr) -> SmolStr { diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 5e25991c6..57454719c 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -281,6 +281,43 @@ fn div(x: i32, y: i32) -> Result { check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); } + #[test] + fn test_wrap_return_type_handles_generic_functions() { + let before = r#" + //- /main.rs + use std::{default::Default, result::Result::{self, Ok, Err}}; + + fn div(x: i32) -> Result { + if x == 0 { + return Err(7); + } + T::default() + } + + //- /std/lib.rs + pub mod result { + pub enum Result { Ok(T), Err(E) } + } + pub mod default { + pub trait Default { + fn default() -> Self; + } + } + "#; +// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - +// it strips empty lines and leading whitespace. The important part of this test is that the final +// `x / y` expr is now wrapped in `Ok(..)` + let after = r#"use std::{default::Default, result::Result::{self, Ok, Err}}; +fn div(x: i32) -> Result { + if x == 0 { + return Err(7); + } + Ok(T::default()) +} +"#; + check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); + } + #[test] fn test_wrap_return_type_handles_type_aliases() { let before = r#" -- cgit v1.2.3 From 456e72c4e472d9ea0717c64a75bd02e94b6e90e8 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 11 Aug 2019 15:03:27 +0100 Subject: Change test to not rely on trait inference --- crates/ra_ide_api/src/diagnostics.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'crates') diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 57454719c..9841fbdf3 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -285,34 +285,29 @@ fn div(x: i32, y: i32) -> Result { fn test_wrap_return_type_handles_generic_functions() { let before = r#" //- /main.rs - use std::{default::Default, result::Result::{self, Ok, Err}}; + use std::result::Result::{self, Ok, Err}; - fn div(x: i32) -> Result { + fn div(x: T) -> Result { if x == 0 { return Err(7); } - T::default() + x } //- /std/lib.rs pub mod result { pub enum Result { Ok(T), Err(E) } } - pub mod default { - pub trait Default { - fn default() -> Self; - } - } "#; // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - // it strips empty lines and leading whitespace. The important part of this test is that the final -// `x / y` expr is now wrapped in `Ok(..)` - let after = r#"use std::{default::Default, result::Result::{self, Ok, Err}}; -fn div(x: i32) -> Result { +// expr is now wrapped in `Ok(..)` + let after = r#"use std::result::Result::{self, Ok, Err}; +fn div(x: T) -> Result { if x == 0 { return Err(7); } - Ok(T::default()) + Ok(x) } "#; check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); -- 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 ++++++++++++------------- crates/ra_ide_api/src/diagnostics.rs | 21 +++++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) (limited to 'crates') 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, } } diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 9841fbdf3..0b9bb5a66 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -217,7 +217,8 @@ mod tests { } fn check_apply_diagnostic_fix_for_target_file(target_file: &str, fixture: &str, after: &str) { - let (analysis, file_id, target_file_contents) = fixture_with_target_file(fixture, target_file); + let (analysis, file_id, target_file_contents) = + fixture_with_target_file(fixture, target_file); let diagnostic = analysis.diagnostics(file_id).unwrap().pop().unwrap(); let mut fix = diagnostic.fix.unwrap(); let edit = fix.source_file_edits.pop().unwrap().edit; @@ -267,9 +268,9 @@ mod tests { pub enum Result { Ok(T), Err(E) } } "#; -// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - -// it strips empty lines and leading whitespace. The important part of this test is that the final -// `x / y` expr is now wrapped in `Ok(..)` + // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - + // it strips empty lines and leading whitespace. The important part of this test is that the final + // `x / y` expr is now wrapped in `Ok(..)` let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; fn div(x: i32, y: i32) -> Result { if y == 0 { @@ -299,9 +300,9 @@ fn div(x: i32, y: i32) -> Result { pub enum Result { Ok(T), Err(E) } } "#; -// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - -// it strips empty lines and leading whitespace. The important part of this test is that the final -// expr is now wrapped in `Ok(..)` + // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - + // it strips empty lines and leading whitespace. The important part of this test is that the final + // expr is now wrapped in `Ok(..)` let after = r#"use std::result::Result::{self, Ok, Err}; fn div(x: T) -> Result { if x == 0 { @@ -336,9 +337,9 @@ fn div(x: T) -> Result { pub enum Result { Ok(T), Err(E) } } "#; -// The formatting here is a bit odd due to how the parse_fixture function works in test_utils - -// it strips empty lines and leading whitespace. The important part of this test is that the final -// `x / y` expr is now wrapped in `Ok(..)` + // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - + // it strips empty lines and leading whitespace. The important part of this test is that the final + // `x / y` expr is now wrapped in `Ok(..)` let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; type MyResult = Result; fn div(x: i32, y: i32) -> MyResult { -- 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') 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 +++-------------- crates/ra_syntax/src/ptr.rs | 7 +++++++ 2 files changed, 10 insertions(+), 14 deletions(-) (limited to 'crates') 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 }); + } } } } diff --git a/crates/ra_syntax/src/ptr.rs b/crates/ra_syntax/src/ptr.rs index d24660ac3..992034ef0 100644 --- a/crates/ra_syntax/src/ptr.rs +++ b/crates/ra_syntax/src/ptr.rs @@ -31,6 +31,13 @@ impl SyntaxNodePtr { pub fn kind(self) -> SyntaxKind { self.kind } + + pub fn cast(self) -> Option> { + if !N::can_cast(self.kind()) { + return None; + } + Some(AstPtr { raw: self, _ty: PhantomData }) + } } /// Like `SyntaxNodePtr`, but remembers the type of node -- cgit v1.2.3 From 6a04e9ce147cba1ae4da177f271a29eb296a9bef Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Tue, 13 Aug 2019 07:31:06 +0100 Subject: Fix build for Diagnostic type change --- crates/ra_hir/src/diagnostics.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'crates') diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index 718345d75..475dd5766 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -154,11 +154,8 @@ impl Diagnostic for MissingOkInTailExpr { fn message(&self) -> String { "wrap return expression in Ok".to_string() } - fn file(&self) -> HirFileId { - self.file - } - fn syntax_node_ptr(&self) -> SyntaxNodePtr { - self.expr.into() + fn source(&self) -> Source { + Source { file_id: self.file, ast: self.expr.into() } } fn as_any(&self) -> &(dyn Any + Send + 'static) { self @@ -169,8 +166,8 @@ impl AstDiagnostic for MissingOkInTailExpr { type AST = ast::Expr; fn ast(&self, db: &impl HirDatabase) -> Self::AST { - let root = db.parse_or_expand(self.file()).unwrap(); - let node = self.syntax_node_ptr().to_node(&root); + let root = db.parse_or_expand(self.file).unwrap(); + let node = self.source().ast.to_node(&root); ast::Expr::cast(node).unwrap() } } -- 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 ++++--------------------- crates/ra_ide_api/src/diagnostics.rs | 30 ++++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 23 deletions(-) (limited to 'crates') 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; diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 0b9bb5a66..94424dc16 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -79,7 +79,7 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec .on::(|d| { let node = d.ast(db); let mut builder = TextEditBuilder::default(); - let replacement = format!("Ok({})", node.syntax().text()); + let replacement = format!("Ok({})", node.syntax()); builder.replace(node.syntax().text_range(), replacement); let fix = SourceChange::source_file_edit_from("wrap with ok", file_id, builder.finish()); res.borrow_mut().push(Diagnostic { @@ -353,7 +353,7 @@ fn div(x: i32, y: i32) -> MyResult { } #[test] - fn test_wrap_return_type_not_applicable() { + fn test_wrap_return_type_not_applicable_when_expr_type_does_not_match_ok_type() { let content = r#" //- /main.rs use std::{string::String, result::Result::{self, Ok, Err}}; @@ -373,6 +373,32 @@ fn div(x: i32, y: i32) -> MyResult { check_no_diagnostic_for_target_file("/main.rs", content); } + #[test] + fn test_wrap_return_type_not_applicable_when_return_type_is_not_result() { + let content = r#" + //- /main.rs + use std::{string::String, result::Result::{self, Ok, Err}}; + + enum SomeOtherEnum { + Ok(i32), + Err(String), + } + + fn foo() -> SomeOtherEnum { + 0 + } + + //- /std/lib.rs + pub mod string { + pub struct String { } + } + pub mod result { + pub enum Result { Ok(T), Err(E) } + } + "#; + check_no_diagnostic_for_target_file("/main.rs", content); + } + #[test] fn test_fill_struct_fields_empty() { let before = r" -- cgit v1.2.3 From 59dd30402b7a1924a0f49d1c902e799654a54f5a Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sat, 17 Aug 2019 07:12:50 +0100 Subject: Specify cursor position in ok-wrapping tests, and switch to using analysis_and_position function --- crates/ra_ide_api/src/diagnostics.rs | 49 +++++++++++++++++++++------------- crates/ra_ide_api/src/mock_analysis.rs | 17 ------------ 2 files changed, 31 insertions(+), 35 deletions(-) (limited to 'crates') diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 94424dc16..4e1f47db6 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -187,7 +187,7 @@ mod tests { use ra_syntax::SourceFile; use test_utils::assert_eq_text; - use crate::mock_analysis::{fixture_with_target_file, single_file}; + use crate::mock_analysis::{analysis_and_position, single_file}; use super::*; @@ -216,14 +216,25 @@ mod tests { assert_eq_text!(after, &actual); } - fn check_apply_diagnostic_fix_for_target_file(target_file: &str, fixture: &str, after: &str) { - let (analysis, file_id, target_file_contents) = - fixture_with_target_file(fixture, target_file); - let diagnostic = analysis.diagnostics(file_id).unwrap().pop().unwrap(); + /// Takes a multi-file input fixture with annotated cursor positions, + /// and checks that: + /// * a diagnostic is produced + /// * this diagnostic touches the input cursor position + /// * that the contents of the file containing the cursor match `after` after the diagnostic fix is applied + fn check_apply_diagnostic_fix_from_position(fixture: &str, after: &str) { + let (analysis, file_position) = analysis_and_position(fixture); + let diagnostic = analysis.diagnostics(file_position.file_id).unwrap().pop().unwrap(); let mut fix = diagnostic.fix.unwrap(); let edit = fix.source_file_edits.pop().unwrap().edit; + let target_file_contents = analysis.file_text(file_position.file_id).unwrap(); let actual = edit.apply(&target_file_contents); assert_eq_text!(after, &actual); + assert!( + diagnostic.range.start() <= file_position.offset && diagnostic.range.end() >= file_position.offset, + "diagnostic range {} does not touch cursor position {}", + diagnostic.range, + file_position.offset + ); } fn check_apply_diagnostic_fix(before: &str, after: &str) { @@ -235,9 +246,11 @@ mod tests { assert_eq_text!(after, &actual); } - fn check_no_diagnostic_for_target_file(target_file: &str, fixture: &str) { - let (analysis, file_id, _) = fixture_with_target_file(fixture, target_file); - let diagnostics = analysis.diagnostics(file_id).unwrap(); + /// Takes a multi-file input fixture with annotated cursor position and checks that no diagnostics + /// apply to the file containing the cursor. + fn check_no_diagnostic_for_target_file(fixture: &str) { + let (analysis, file_position) = analysis_and_position(fixture); + let diagnostics = analysis.diagnostics(file_position.file_id).unwrap(); assert_eq!(diagnostics.len(), 0); } @@ -257,7 +270,7 @@ mod tests { if y == 0 { return Err("div by zero".into()); } - x / y + x / y<|> } //- /std/lib.rs @@ -279,7 +292,7 @@ fn div(x: i32, y: i32) -> Result { Ok(x / y) } "#; - check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); + check_apply_diagnostic_fix_from_position(before, after); } #[test] @@ -292,7 +305,7 @@ fn div(x: i32, y: i32) -> Result { if x == 0 { return Err(7); } - x + <|>x } //- /std/lib.rs @@ -311,7 +324,7 @@ fn div(x: T) -> Result { Ok(x) } "#; - check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); + check_apply_diagnostic_fix_from_position(before, after); } #[test] @@ -326,7 +339,7 @@ fn div(x: T) -> Result { if y == 0 { return Err("div by zero".into()); } - x / y + x <|>/ y } //- /std/lib.rs @@ -349,7 +362,7 @@ fn div(x: i32, y: i32) -> MyResult { Ok(x / y) } "#; - check_apply_diagnostic_fix_for_target_file("/main.rs", before, after); + check_apply_diagnostic_fix_from_position(before, after); } #[test] @@ -359,7 +372,7 @@ fn div(x: i32, y: i32) -> MyResult { use std::{string::String, result::Result::{self, Ok, Err}}; fn foo() -> Result { - 0 + 0<|> } //- /std/lib.rs @@ -370,7 +383,7 @@ fn div(x: i32, y: i32) -> MyResult { pub enum Result { Ok(T), Err(E) } } "#; - check_no_diagnostic_for_target_file("/main.rs", content); + check_no_diagnostic_for_target_file(content); } #[test] @@ -385,7 +398,7 @@ fn div(x: i32, y: i32) -> MyResult { } fn foo() -> SomeOtherEnum { - 0 + 0<|> } //- /std/lib.rs @@ -396,7 +409,7 @@ fn div(x: i32, y: i32) -> MyResult { pub enum Result { Ok(T), Err(E) } } "#; - check_no_diagnostic_for_target_file("/main.rs", content); + check_no_diagnostic_for_target_file(content); } #[test] diff --git a/crates/ra_ide_api/src/mock_analysis.rs b/crates/ra_ide_api/src/mock_analysis.rs index 0eaf5d15a..132f6f875 100644 --- a/crates/ra_ide_api/src/mock_analysis.rs +++ b/crates/ra_ide_api/src/mock_analysis.rs @@ -80,15 +80,6 @@ impl MockAnalysis { .expect("no file in this mock"); FileId(idx as u32 + 1) } - pub fn id_and_contents_of(&self, path: &str) -> (FileId, String) { - let (idx, contents) = self - .files - .iter() - .enumerate() - .find(|(_, (p, _text))| path == p) - .expect("no file in this mock"); - (FileId(idx as u32 + 1), contents.1.to_string()) - } pub fn analysis_host(self) -> AnalysisHost { let mut host = AnalysisHost::default(); let source_root = SourceRootId(0); @@ -133,14 +124,6 @@ pub fn single_file(code: &str) -> (Analysis, FileId) { (mock.analysis(), file_id) } -/// Creates analysis from a fixture with multiple files -/// and returns the file id and contents of the target file. -pub fn fixture_with_target_file(fixture: &str, target_file: &str) -> (Analysis, FileId, String) { - let mock = MockAnalysis::with_files(fixture); - let (target_file_id, target_file_contents) = mock.id_and_contents_of(target_file); - (mock.analysis(), target_file_id, target_file_contents) -} - /// Creates analysis for a single file, returns position marked with <|>. pub fn single_file_with_position(code: &str) -> (Analysis, FilePosition) { let mut mock = MockAnalysis::new(); -- cgit v1.2.3 From 14a23d1bde8493df9e5196973132144060a61709 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sat, 17 Aug 2019 07:37:37 +0100 Subject: Strip indents and empty lines in check_apply_diagnostic_fix_from_position --- crates/ra_ide_api/src/diagnostics.rs | 91 ++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 36 deletions(-) (limited to 'crates') diff --git a/crates/ra_ide_api/src/diagnostics.rs b/crates/ra_ide_api/src/diagnostics.rs index 4e1f47db6..1a4882824 100644 --- a/crates/ra_ide_api/src/diagnostics.rs +++ b/crates/ra_ide_api/src/diagnostics.rs @@ -184,6 +184,7 @@ fn check_struct_shorthand_initialization( #[cfg(test)] mod tests { use insta::assert_debug_snapshot_matches; + use join_to_string::join; use ra_syntax::SourceFile; use test_utils::assert_eq_text; @@ -228,9 +229,30 @@ mod tests { let edit = fix.source_file_edits.pop().unwrap().edit; let target_file_contents = analysis.file_text(file_position.file_id).unwrap(); let actual = edit.apply(&target_file_contents); - assert_eq_text!(after, &actual); + + // Strip indent and empty lines from `after`, to match the behaviour of + // `parse_fixture` called from `analysis_and_position`. + let margin = fixture + .lines() + .filter(|it| it.trim_start().starts_with("//-")) + .map(|it| it.len() - it.trim_start().len()) + .next() + .expect("empty fixture"); + let after = join(after.lines().filter_map(|line| { + if line.len() > margin { + Some(&line[margin..]) + } else { + None + } + })) + .separator("\n") + .suffix("\n") + .to_string(); + + assert_eq_text!(&after, &actual); assert!( - diagnostic.range.start() <= file_position.offset && diagnostic.range.end() >= file_position.offset, + diagnostic.range.start() <= file_position.offset + && diagnostic.range.end() >= file_position.offset, "diagnostic range {} does not touch cursor position {}", diagnostic.range, file_position.offset @@ -281,17 +303,16 @@ mod tests { pub enum Result { Ok(T), Err(E) } } "#; - // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - - // it strips empty lines and leading whitespace. The important part of this test is that the final - // `x / y` expr is now wrapped in `Ok(..)` - let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; -fn div(x: i32, y: i32) -> Result { - if y == 0 { - return Err("div by zero".into()); - } - Ok(x / y) -} -"#; + let after = r#" + use std::{string::String, result::Result::{self, Ok, Err}}; + + fn div(x: i32, y: i32) -> Result { + if y == 0 { + return Err("div by zero".into()); + } + Ok(x / y) + } + "#; check_apply_diagnostic_fix_from_position(before, after); } @@ -313,17 +334,16 @@ fn div(x: i32, y: i32) -> Result { pub enum Result { Ok(T), Err(E) } } "#; - // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - - // it strips empty lines and leading whitespace. The important part of this test is that the final - // expr is now wrapped in `Ok(..)` - let after = r#"use std::result::Result::{self, Ok, Err}; -fn div(x: T) -> Result { - if x == 0 { - return Err(7); - } - Ok(x) -} -"#; + let after = r#" + use std::result::Result::{self, Ok, Err}; + + fn div(x: T) -> Result { + if x == 0 { + return Err(7); + } + Ok(x) + } + "#; check_apply_diagnostic_fix_from_position(before, after); } @@ -350,18 +370,17 @@ fn div(x: T) -> Result { pub enum Result { Ok(T), Err(E) } } "#; - // The formatting here is a bit odd due to how the parse_fixture function works in test_utils - - // it strips empty lines and leading whitespace. The important part of this test is that the final - // `x / y` expr is now wrapped in `Ok(..)` - let after = r#"use std::{string::String, result::Result::{self, Ok, Err}}; -type MyResult = Result; -fn div(x: i32, y: i32) -> MyResult { - if y == 0 { - return Err("div by zero".into()); - } - Ok(x / y) -} -"#; + let after = r#" + use std::{string::String, result::Result::{self, Ok, Err}}; + + type MyResult = Result; + fn div(x: i32, y: i32) -> MyResult { + if y == 0 { + return Err("div by zero".into()); + } + Ok(x / y) + } + "#; check_apply_diagnostic_fix_from_position(before, after); } -- cgit v1.2.3