From 0b95bed83fc8db897f54b350168567f14527e8de Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 23 May 2020 17:49:53 -0400 Subject: Add unsafe diagnostics and unsafe highlighting --- crates/ra_hir_ty/src/diagnostics.rs | 58 +++++++++++++++++++++++++++++++++++++ crates/ra_hir_ty/src/expr.rs | 24 ++++++++++++++- 2 files changed, 81 insertions(+), 1 deletion(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index ebd9cb08f..3469cc680 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -169,3 +169,61 @@ impl AstDiagnostic for BreakOutsideOfLoop { ast::Expr::cast(node).unwrap() } } + +#[derive(Debug)] +pub struct MissingUnsafe { + pub file: HirFileId, + pub fn_def: AstPtr, + pub fn_name: Name, +} + +impl Diagnostic for MissingUnsafe { + fn message(&self) -> String { + format!("Missing unsafe marker on fn `{}`", self.fn_name) + } + fn source(&self) -> InFile { + InFile { file_id: self.file, value: self.fn_def.clone().into() } + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + +impl AstDiagnostic for MissingUnsafe { + type AST = ast::FnDef; + + fn ast(&self, db: &impl AstDatabase) -> Self::AST { + let root = db.parse_or_expand(self.source().file_id).unwrap(); + let node = self.source().value.to_node(&root); + ast::FnDef::cast(node).unwrap() + } +} + +#[derive(Debug)] +pub struct UnnecessaryUnsafe { + pub file: HirFileId, + pub fn_def: AstPtr, + pub fn_name: Name, +} + +impl Diagnostic for UnnecessaryUnsafe { + fn message(&self) -> String { + format!("Unnecessary unsafe marker on fn `{}`", self.fn_name) + } + fn source(&self) -> InFile { + InFile { file_id: self.file, value: self.fn_def.clone().into() } + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + +impl AstDiagnostic for UnnecessaryUnsafe { + type AST = ast::FnDef; + + fn ast(&self, db: &impl AstDatabase) -> Self::AST { + let root = db.parse_or_expand(self.source().file_id).unwrap(); + let node = self.source().value.to_node(&root); + ast::FnDef::cast(node).unwrap() + } +} diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 7db928dde..795f1762c 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -2,7 +2,7 @@ use std::sync::Arc; -use hir_def::{path::path, resolver::HasResolver, AdtId, FunctionId}; +use hir_def::{path::path, resolver::HasResolver, AdtId, DefWithBodyId, FunctionId}; use hir_expand::diagnostics::DiagnosticSink; use ra_syntax::{ast, AstPtr}; use rustc_hash::FxHashSet; @@ -312,3 +312,25 @@ pub fn record_pattern_missing_fields( } Some((variant_def, missed_fields, exhaustive)) } + +pub fn unsafe_expressions( + db: &dyn HirDatabase, + infer: &InferenceResult, + def: DefWithBodyId, +) -> Vec { + let mut unsafe_expr_ids = vec![]; + let body = db.body(def); + for (id, expr) in body.exprs.iter() { + if let Expr::Call { callee, .. } = expr { + if infer + .method_resolution(*callee) + .map(|func| db.function_data(func).is_unsafe) + .unwrap_or(false) + { + unsafe_expr_ids.push(id); + } + } + } + + unsafe_expr_ids +} -- cgit v1.2.3 From daf1cac9f87023d37a4418ea24ed615c9706258b Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 01:33:22 -0400 Subject: Move diagnostics back into expr, add tests for diagnostics, fix logic to account for derefs of raw ptrs --- crates/ra_hir_ty/src/diagnostics.rs | 16 ++++---- crates/ra_hir_ty/src/expr.rs | 70 ++++++++++++++++++++++++++++----- crates/ra_hir_ty/src/test_db.rs | 10 ++++- crates/ra_hir_ty/src/tests.rs | 78 +++++++++++++++++++++++++++++++++++++ 4 files changed, 154 insertions(+), 20 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 3469cc680..c6ca322fa 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -3,7 +3,7 @@ use std::any::Any; use hir_expand::{db::AstDatabase, name::Name, HirFileId, InFile}; -use ra_syntax::{ast, AstNode, AstPtr, SyntaxNodePtr}; +use ra_syntax::{ast::{self, NameOwner}, AstNode, AstPtr, SyntaxNodePtr}; use stdx::format_to; pub use hir_def::{diagnostics::UnresolvedModule, expr::MatchArm, path::Path}; @@ -174,12 +174,11 @@ impl AstDiagnostic for BreakOutsideOfLoop { pub struct MissingUnsafe { pub file: HirFileId, pub fn_def: AstPtr, - pub fn_name: Name, } impl Diagnostic for MissingUnsafe { fn message(&self) -> String { - format!("Missing unsafe marker on fn `{}`", self.fn_name) + format!("Missing unsafe keyword on fn") } fn source(&self) -> InFile { InFile { file_id: self.file, value: self.fn_def.clone().into() } @@ -190,12 +189,12 @@ impl Diagnostic for MissingUnsafe { } impl AstDiagnostic for MissingUnsafe { - type AST = ast::FnDef; + type AST = ast::Name; fn ast(&self, db: &impl AstDatabase) -> Self::AST { let root = db.parse_or_expand(self.source().file_id).unwrap(); let node = self.source().value.to_node(&root); - ast::FnDef::cast(node).unwrap() + ast::FnDef::cast(node).unwrap().name().unwrap() } } @@ -203,12 +202,11 @@ impl AstDiagnostic for MissingUnsafe { pub struct UnnecessaryUnsafe { pub file: HirFileId, pub fn_def: AstPtr, - pub fn_name: Name, } impl Diagnostic for UnnecessaryUnsafe { fn message(&self) -> String { - format!("Unnecessary unsafe marker on fn `{}`", self.fn_name) + format!("Unnecessary unsafe keyword on fn") } fn source(&self) -> InFile { InFile { file_id: self.file, value: self.fn_def.clone().into() } @@ -219,11 +217,11 @@ impl Diagnostic for UnnecessaryUnsafe { } impl AstDiagnostic for UnnecessaryUnsafe { - type AST = ast::FnDef; + type AST = ast::Name; fn ast(&self, db: &impl AstDatabase) -> Self::AST { let root = db.parse_or_expand(self.source().file_id).unwrap(); let node = self.source().value.to_node(&root); - ast::FnDef::cast(node).unwrap() + ast::FnDef::cast(node).unwrap().name().unwrap() } } diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 795f1762c..7532e2dc7 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -2,14 +2,19 @@ use std::sync::Arc; -use hir_def::{path::path, resolver::HasResolver, AdtId, DefWithBodyId, FunctionId}; +use hir_def::{ + path::path, resolver::HasResolver, src::HasSource, AdtId, DefWithBodyId, FunctionId, Lookup, +}; use hir_expand::diagnostics::DiagnosticSink; use ra_syntax::{ast, AstPtr}; use rustc_hash::FxHashSet; use crate::{ db::HirDatabase, - diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields}, + diagnostics::{ + MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields, MissingUnsafe, + UnnecessaryUnsafe, + }, utils::variant_data, ApplicationTy, InferenceResult, Ty, TypeCtor, _match::{is_useful, MatchCheckCtx, Matrix, PatStack, Usefulness}, @@ -321,16 +326,63 @@ pub fn unsafe_expressions( let mut unsafe_expr_ids = vec![]; let body = db.body(def); for (id, expr) in body.exprs.iter() { - if let Expr::Call { callee, .. } = expr { - if infer - .method_resolution(*callee) - .map(|func| db.function_data(func).is_unsafe) - .unwrap_or(false) - { - unsafe_expr_ids.push(id); + match expr { + Expr::Call { callee, .. } => { + if infer + .method_resolution(*callee) + .map(|func| db.function_data(func).is_unsafe) + .unwrap_or(false) + { + unsafe_expr_ids.push(id); + } + } + Expr::UnaryOp { expr, op: UnaryOp::Deref } => { + if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { + unsafe_expr_ids.push(id); + } } + _ => {} } } unsafe_expr_ids } + +pub struct UnsafeValidator<'a, 'b: 'a> { + func: FunctionId, + infer: Arc, + sink: &'a mut DiagnosticSink<'b>, +} + +impl<'a, 'b> UnsafeValidator<'a, 'b> { + pub fn new( + func: FunctionId, + infer: Arc, + sink: &'a mut DiagnosticSink<'b>, + ) -> UnsafeValidator<'a, 'b> { + UnsafeValidator { func, infer, sink } + } + + pub fn validate_body(&mut self, db: &dyn HirDatabase) { + let def = self.func.into(); + let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); + let func_data = db.function_data(self.func); + let unnecessary = func_data.is_unsafe && unsafe_expressions.len() == 0; + let missing = !func_data.is_unsafe && unsafe_expressions.len() > 0; + if !(unnecessary || missing) { + return; + } + + let loc = self.func.lookup(db.upcast()); + let in_file = loc.source(db.upcast()); + + let file = in_file.file_id; + let fn_def = AstPtr::new(&in_file.value); + + if unnecessary { + self.sink.push(UnnecessaryUnsafe { file, fn_def }) + } else { + self.sink.push(MissingUnsafe { file, fn_def }) + } + } +} diff --git a/crates/ra_hir_ty/src/test_db.rs b/crates/ra_hir_ty/src/test_db.rs index ad04e3e0f..9ccf2aa37 100644 --- a/crates/ra_hir_ty/src/test_db.rs +++ b/crates/ra_hir_ty/src/test_db.rs @@ -11,7 +11,11 @@ use ra_db::{salsa, CrateId, FileId, FileLoader, FileLoaderDelegate, SourceDataba use rustc_hash::FxHashSet; use stdx::format_to; -use crate::{db::HirDatabase, diagnostics::Diagnostic, expr::ExprValidator}; +use crate::{ + db::HirDatabase, + diagnostics::Diagnostic, + expr::{ExprValidator, UnsafeValidator}, +}; #[salsa::database( ra_db::SourceDatabaseExtStorage, @@ -119,7 +123,9 @@ impl TestDB { let infer = self.infer(f.into()); let mut sink = DiagnosticSink::new(&mut cb); infer.add_diagnostics(self, f, &mut sink); - let mut validator = ExprValidator::new(f, infer, &mut sink); + let mut validator = ExprValidator::new(f, infer.clone(), &mut sink); + validator.validate_body(self); + let mut validator = UnsafeValidator::new(f, infer, &mut sink); validator.validate_body(self); } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 85ff26a36..4ff2b2d4a 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -538,6 +538,84 @@ fn missing_record_pat_field_no_diagnostic_if_not_exhaustive() { assert_snapshot!(diagnostics, @""); } +#[test] +fn missing_unsafe_diagnostic_with_raw_ptr() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +fn missing_unsafe() { + let x = &5 as *usize; + let y = *x; +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n let x = &5 as *usize;\n let y = *x;\n}": Missing unsafe keyword on fn"#); +} + +#[test] +fn missing_unsafe_diagnostic_with_unsafe_call() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +unsafe fn unsafe_fn() { + let x = &5 as *usize; + let y = *x; +} + +fn missing_unsafe() { + unsafe_fn(); +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n unsafe_fn();\n}": Missing unsafe keyword on fn"#); +} + +#[test] +fn missing_unsafe_diagnostic_with_unsafe_method_call() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +struct HasUnsafe; + +impl HasUnsafe { + unsafe fn unsafe_fn() { + let x = &5 as *usize; + let y = *x; + } +} + +fn missing_unsafe() { + HasUnsafe.unsafe_fn(); +} + +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n HasUnsafe.unsafe_fn();\n}": Missing unsafe keyword on fn"#); +} + +#[test] +fn unnecessary_unsafe_diagnostic() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +unsafe fn actually_safe_fn() {} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @r#""unsafe fn actually_safe_fn() {}": Unnecessary unsafe keyword on fn"#); +} + #[test] fn break_outside_of_loop() { let diagnostics = TestDB::with_files( -- cgit v1.2.3 From b358fbfdf82409700a8a328794429ec790306fc2 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 02:10:34 -0400 Subject: Add tests covering unsafe blocks, more attempts to get call expr tests passing --- crates/ra_hir_ty/src/expr.rs | 17 ++++++++++- crates/ra_hir_ty/src/tests.rs | 70 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 7532e2dc7..04668f486 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -329,13 +329,28 @@ pub fn unsafe_expressions( match expr { Expr::Call { callee, .. } => { if infer - .method_resolution(*callee) + .method_resolution(/* id */ *callee) .map(|func| db.function_data(func).is_unsafe) .unwrap_or(false) { unsafe_expr_ids.push(id); } } + Expr::MethodCall {/*_receiver, method_name,*/ .. } => { + // let receiver_ty = &infer.type_of_expr[*receiver]; + // receiver_ty + if infer + .method_resolution(id) + .map(|func| { + db.function_data(func).is_unsafe + }) + .unwrap_or_else(|| { + false + }) + { + unsafe_expr_ids.push(id); + } + } Expr::UnaryOp { expr, op: UnaryOp::Deref } => { if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { unsafe_expr_ids.push(id); diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 4ff2b2d4a..c1f6fbab8 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -602,6 +602,76 @@ fn missing_unsafe() { assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n HasUnsafe.unsafe_fn();\n}": Missing unsafe keyword on fn"#); } +#[test] +fn no_missing_unsafe_diagnostic_with_raw_ptr_in_unsafe_block() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +fn nothing_to_see_move_along() { + unsafe { + let x = &5 as *usize; + let y = *x; + } +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @""); +} + +#[test] +fn no_missing_unsafe_diagnostic_with_unsafe_call_in_unsafe_block() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +unsafe fn unsafe_fn() { + let x = &5 as *usize; + let y = *x; +} + +fn nothing_to_see_move_along() { + unsafe { + unsafe_fn(); + } +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @""); +} + +#[test] +fn no_missing_unsafe_diagnostic_with_unsafe_method_call_in_unsafe_block() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +struct HasUnsafe; + +impl HasUnsafe { + unsafe fn unsafe_fn() { + let x = &5 as *usize; + let y = *x; + } +} + +fn nothing_to_see_move_along() { + unsafe { + HasUnsafe.unsafe_fn(); + } +} + +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @""); +} + #[test] fn unnecessary_unsafe_diagnostic() { let diagnostics = TestDB::with_files( -- cgit v1.2.3 From 499d4c454d1a66d10c3cf4c9bacbdb15f295af39 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 13:18:31 -0400 Subject: Remove UnnecessaryUnsafe diagnostic, Fix Expr::Call unsafe analysis --- crates/ra_hir_ty/src/diagnostics.rs | 33 ++++----------------------------- crates/ra_hir_ty/src/expr.rs | 29 ++++++++++------------------- crates/ra_hir_ty/src/tests.rs | 28 +++++++--------------------- 3 files changed, 21 insertions(+), 69 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index c6ca322fa..0b7bcdc9d 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -3,7 +3,10 @@ use std::any::Any; use hir_expand::{db::AstDatabase, name::Name, HirFileId, InFile}; -use ra_syntax::{ast::{self, NameOwner}, AstNode, AstPtr, SyntaxNodePtr}; +use ra_syntax::{ + ast::{self, NameOwner}, + AstNode, AstPtr, SyntaxNodePtr, +}; use stdx::format_to; pub use hir_def::{diagnostics::UnresolvedModule, expr::MatchArm, path::Path}; @@ -197,31 +200,3 @@ impl AstDiagnostic for MissingUnsafe { ast::FnDef::cast(node).unwrap().name().unwrap() } } - -#[derive(Debug)] -pub struct UnnecessaryUnsafe { - pub file: HirFileId, - pub fn_def: AstPtr, -} - -impl Diagnostic for UnnecessaryUnsafe { - fn message(&self) -> String { - format!("Unnecessary unsafe keyword on fn") - } - fn source(&self) -> InFile { - InFile { file_id: self.file, value: self.fn_def.clone().into() } - } - fn as_any(&self) -> &(dyn Any + Send + 'static) { - self - } -} - -impl AstDiagnostic for UnnecessaryUnsafe { - type AST = ast::Name; - - fn ast(&self, db: &impl AstDatabase) -> Self::AST { - let root = db.parse_or_expand(self.source().file_id).unwrap(); - let node = self.source().value.to_node(&root); - ast::FnDef::cast(node).unwrap().name().unwrap() - } -} diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 04668f486..1a0710e0b 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -13,8 +13,8 @@ use crate::{ db::HirDatabase, diagnostics::{ MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields, MissingUnsafe, - UnnecessaryUnsafe, }, + lower::CallableDef, utils::variant_data, ApplicationTy, InferenceResult, Ty, TypeCtor, _match::{is_useful, MatchCheckCtx, Matrix, PatStack, Usefulness}, @@ -328,17 +328,14 @@ pub fn unsafe_expressions( for (id, expr) in body.exprs.iter() { match expr { Expr::Call { callee, .. } => { - if infer - .method_resolution(/* id */ *callee) - .map(|func| db.function_data(func).is_unsafe) - .unwrap_or(false) - { - unsafe_expr_ids.push(id); - } + let ty = &infer.type_of_expr[*callee]; + if let &Ty::Apply(ApplicationTy {ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), .. }) = ty { + if db.function_data(func).is_unsafe { + unsafe_expr_ids.push(id); + } + } } - Expr::MethodCall {/*_receiver, method_name,*/ .. } => { - // let receiver_ty = &infer.type_of_expr[*receiver]; - // receiver_ty + Expr::MethodCall { .. } => { if infer .method_resolution(id) .map(|func| { @@ -382,9 +379,7 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { let def = self.func.into(); let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); let func_data = db.function_data(self.func); - let unnecessary = func_data.is_unsafe && unsafe_expressions.len() == 0; - let missing = !func_data.is_unsafe && unsafe_expressions.len() > 0; - if !(unnecessary || missing) { + if func_data.is_unsafe || unsafe_expressions.len() == 0 { return; } @@ -394,10 +389,6 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { let file = in_file.file_id; let fn_def = AstPtr::new(&in_file.value); - if unnecessary { - self.sink.push(UnnecessaryUnsafe { file, fn_def }) - } else { - self.sink.push(MissingUnsafe { file, fn_def }) - } + self.sink.push(MissingUnsafe { file, fn_def }) } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index c1f6fbab8..28faccf38 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -544,7 +544,7 @@ fn missing_unsafe_diagnostic_with_raw_ptr() { r" //- /lib.rs fn missing_unsafe() { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } ", @@ -552,7 +552,7 @@ fn missing_unsafe() { .diagnostics() .0; - assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n let x = &5 as *usize;\n let y = *x;\n}": Missing unsafe keyword on fn"#); + assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n let x = &5 as *const usize;\n let y = *x;\n}": Missing unsafe keyword on fn"#); } #[test] @@ -561,7 +561,7 @@ fn missing_unsafe_diagnostic_with_unsafe_call() { r" //- /lib.rs unsafe fn unsafe_fn() { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } @@ -585,7 +585,7 @@ struct HasUnsafe; impl HasUnsafe { unsafe fn unsafe_fn() { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } } @@ -609,7 +609,7 @@ fn no_missing_unsafe_diagnostic_with_raw_ptr_in_unsafe_block() { //- /lib.rs fn nothing_to_see_move_along() { unsafe { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } } @@ -627,7 +627,7 @@ fn no_missing_unsafe_diagnostic_with_unsafe_call_in_unsafe_block() { r" //- /lib.rs unsafe fn unsafe_fn() { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } @@ -653,7 +653,7 @@ struct HasUnsafe; impl HasUnsafe { unsafe fn unsafe_fn() { - let x = &5 as *usize; + let x = &5 as *const usize; let y = *x; } } @@ -672,20 +672,6 @@ fn nothing_to_see_move_along() { assert_snapshot!(diagnostics, @""); } -#[test] -fn unnecessary_unsafe_diagnostic() { - let diagnostics = TestDB::with_files( - r" -//- /lib.rs -unsafe fn actually_safe_fn() {} -", - ) - .diagnostics() - .0; - - assert_snapshot!(diagnostics, @r#""unsafe fn actually_safe_fn() {}": Unnecessary unsafe keyword on fn"#); -} - #[test] fn break_outside_of_loop() { let diagnostics = TestDB::with_files( -- cgit v1.2.3 From 3df0f9ce7e6eea48b67dae8b26e83aa7bd36ff24 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 15:31:33 -0400 Subject: Add missing self param to test --- crates/ra_hir_ty/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 28faccf38..8889fa3ba 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -584,7 +584,7 @@ fn missing_unsafe_diagnostic_with_unsafe_method_call() { struct HasUnsafe; impl HasUnsafe { - unsafe fn unsafe_fn() { + unsafe fn unsafe_fn(&self) { let x = &5 as *const usize; let y = *x; } -- cgit v1.2.3 From 278cbf12cd0f76fc191d5ce7f130e6245596a578 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 16:24:36 -0400 Subject: Track unsafe blocks, don't trigger missing unsafe diagnostic when unsafe exprs within unsafe block --- crates/ra_hir_ty/src/expr.rs | 64 +++++++++++++++++++++++++++--------- crates/ra_hir_ty/src/infer/expr.rs | 1 + crates/ra_hir_ty/src/tests.rs | 22 ++++++++++++- crates/ra_hir_ty/src/tests/simple.rs | 1 + 4 files changed, 72 insertions(+), 16 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 1a0710e0b..d600aca21 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -318,46 +318,74 @@ pub fn record_pattern_missing_fields( Some((variant_def, missed_fields, exhaustive)) } +pub struct UnsafeExpr { + expr: ExprId, + inside_unsafe_block: bool, +} + +impl UnsafeExpr { + fn new(expr: ExprId) -> Self { + Self { expr, inside_unsafe_block: false } + } +} + pub fn unsafe_expressions( db: &dyn HirDatabase, infer: &InferenceResult, def: DefWithBodyId, -) -> Vec { - let mut unsafe_expr_ids = vec![]; +) -> Vec { + let mut unsafe_exprs = vec![]; + let mut unsafe_block_scopes = vec![]; let body = db.body(def); + let expr_scopes = db.expr_scopes(def); for (id, expr) in body.exprs.iter() { match expr { + Expr::UnsafeBlock { body } => { + if let Some(scope) = expr_scopes.scope_for(*body) { + unsafe_block_scopes.push(scope); + } + } Expr::Call { callee, .. } => { let ty = &infer.type_of_expr[*callee]; - if let &Ty::Apply(ApplicationTy {ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), .. }) = ty { + if let &Ty::Apply(ApplicationTy { + ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), + .. + }) = ty + { if db.function_data(func).is_unsafe { - unsafe_expr_ids.push(id); + unsafe_exprs.push(UnsafeExpr::new(id)); } - } + } } Expr::MethodCall { .. } => { if infer .method_resolution(id) - .map(|func| { - db.function_data(func).is_unsafe - }) - .unwrap_or_else(|| { - false - }) + .map(|func| db.function_data(func).is_unsafe) + .unwrap_or_else(|| false) { - unsafe_expr_ids.push(id); + unsafe_exprs.push(UnsafeExpr::new(id)); } } Expr::UnaryOp { expr, op: UnaryOp::Deref } => { if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { - unsafe_expr_ids.push(id); + unsafe_exprs.push(UnsafeExpr::new(id)); } } _ => {} } } - unsafe_expr_ids + 'unsafe_exprs: for unsafe_expr in &mut unsafe_exprs { + let scope = expr_scopes.scope_for(unsafe_expr.expr); + for scope in expr_scopes.scope_chain(scope) { + if unsafe_block_scopes.contains(&scope) { + unsafe_expr.inside_unsafe_block = true; + continue 'unsafe_exprs; + } + } + } + + unsafe_exprs } pub struct UnsafeValidator<'a, 'b: 'a> { @@ -379,7 +407,13 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { let def = self.func.into(); let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); let func_data = db.function_data(self.func); - if func_data.is_unsafe || unsafe_expressions.len() == 0 { + if func_data.is_unsafe + || unsafe_expressions + .into_iter() + .filter(|unsafe_expr| !unsafe_expr.inside_unsafe_block) + .count() + == 0 + { return; } diff --git a/crates/ra_hir_ty/src/infer/expr.rs b/crates/ra_hir_ty/src/infer/expr.rs index a9565a58d..df7bcb5fa 100644 --- a/crates/ra_hir_ty/src/infer/expr.rs +++ b/crates/ra_hir_ty/src/infer/expr.rs @@ -142,6 +142,7 @@ impl<'a> InferenceContext<'a> { // FIXME: Breakable block inference self.infer_block(statements, *tail, expected) } + Expr::UnsafeBlock { body } => self.infer_expr(*body, expected), Expr::TryBlock { body } => { let _inner = self.infer_expr(*body, expected); // FIXME should be std::result::Result<{inner}, _> diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 8889fa3ba..4bc2e8b27 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -608,10 +608,30 @@ fn no_missing_unsafe_diagnostic_with_raw_ptr_in_unsafe_block() { r" //- /lib.rs fn nothing_to_see_move_along() { + let x = &5 as *const usize; + unsafe { + let y = *x; + } +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @""); +} + +#[test] +fn missing_unsafe_diagnostic_with_raw_ptr_outside_unsafe_block() { + let diagnostics = TestDB::with_files( + r" +//- /lib.rs +fn nothing_to_see_move_along() { + let x = &5 as *const usize; unsafe { - let x = &5 as *const usize; let y = *x; } + let z = *x; } ", ) diff --git a/crates/ra_hir_ty/src/tests/simple.rs b/crates/ra_hir_ty/src/tests/simple.rs index 7d8197f8b..cd919466f 100644 --- a/crates/ra_hir_ty/src/tests/simple.rs +++ b/crates/ra_hir_ty/src/tests/simple.rs @@ -1880,6 +1880,7 @@ fn main() { @r###" 10..130 '{ ...2 }; }': () 20..21 'x': i32 + 24..37 'unsafe { 92 }': i32 31..37 '{ 92 }': i32 33..35 '92': i32 47..48 'y': {unknown} -- cgit v1.2.3 From b9569886a915f2411adaecbcad28eda8b163c3e3 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 16:28:02 -0400 Subject: Rename Expr::UnsafeBlock to Expr::Unsafe --- crates/ra_hir_ty/src/expr.rs | 2 +- crates/ra_hir_ty/src/infer/expr.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index d600aca21..ce73251b8 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -340,7 +340,7 @@ pub fn unsafe_expressions( let expr_scopes = db.expr_scopes(def); for (id, expr) in body.exprs.iter() { match expr { - Expr::UnsafeBlock { body } => { + Expr::Unsafe { body } => { if let Some(scope) = expr_scopes.scope_for(*body) { unsafe_block_scopes.push(scope); } diff --git a/crates/ra_hir_ty/src/infer/expr.rs b/crates/ra_hir_ty/src/infer/expr.rs index df7bcb5fa..61af5f064 100644 --- a/crates/ra_hir_ty/src/infer/expr.rs +++ b/crates/ra_hir_ty/src/infer/expr.rs @@ -142,7 +142,7 @@ impl<'a> InferenceContext<'a> { // FIXME: Breakable block inference self.infer_block(statements, *tail, expected) } - Expr::UnsafeBlock { body } => self.infer_expr(*body, expected), + Expr::Unsafe { body } => self.infer_expr(*body, expected), Expr::TryBlock { body } => { let _inner = self.infer_expr(*body, expected); // FIXME should be std::result::Result<{inner}, _> -- cgit v1.2.3 From 9ce44be2ab7e9a99eece1c2e254f15ad1c6d73c5 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 27 May 2020 08:51:08 -0400 Subject: Address review comments, have MissingUnsafe diagnostic point to each unsafe use, update tests --- crates/ra_hir_ty/src/diagnostics.rs | 15 ++++++--------- crates/ra_hir_ty/src/expr.rs | 23 ++++++++++------------- crates/ra_hir_ty/src/tests.rs | 6 +++--- 3 files changed, 19 insertions(+), 25 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 0b7bcdc9d..a59efb347 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -3,10 +3,7 @@ use std::any::Any; use hir_expand::{db::AstDatabase, name::Name, HirFileId, InFile}; -use ra_syntax::{ - ast::{self, NameOwner}, - AstNode, AstPtr, SyntaxNodePtr, -}; +use ra_syntax::{ast, AstNode, AstPtr, SyntaxNodePtr}; use stdx::format_to; pub use hir_def::{diagnostics::UnresolvedModule, expr::MatchArm, path::Path}; @@ -176,15 +173,15 @@ impl AstDiagnostic for BreakOutsideOfLoop { #[derive(Debug)] pub struct MissingUnsafe { pub file: HirFileId, - pub fn_def: AstPtr, + pub expr: AstPtr, } impl Diagnostic for MissingUnsafe { fn message(&self) -> String { - format!("Missing unsafe keyword on fn") + format!("This operation is unsafe and requires an unsafe function or block") } fn source(&self) -> InFile { - InFile { file_id: self.file, value: self.fn_def.clone().into() } + InFile { file_id: self.file, value: self.expr.clone().into() } } fn as_any(&self) -> &(dyn Any + Send + 'static) { self @@ -192,11 +189,11 @@ impl Diagnostic for MissingUnsafe { } impl AstDiagnostic for MissingUnsafe { - type AST = ast::Name; + type AST = ast::Expr; fn ast(&self, db: &impl AstDatabase) -> Self::AST { let root = db.parse_or_expand(self.source().file_id).unwrap(); let node = self.source().value.to_node(&root); - ast::FnDef::cast(node).unwrap().name().unwrap() + ast::Expr::cast(node).unwrap() } } diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index ce73251b8..5f332aadb 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -2,9 +2,7 @@ use std::sync::Arc; -use hir_def::{ - path::path, resolver::HasResolver, src::HasSource, AdtId, DefWithBodyId, FunctionId, Lookup, -}; +use hir_def::{path::path, resolver::HasResolver, AdtId, DefWithBodyId, FunctionId}; use hir_expand::diagnostics::DiagnosticSink; use ra_syntax::{ast, AstPtr}; use rustc_hash::FxHashSet; @@ -346,7 +344,7 @@ pub fn unsafe_expressions( } } Expr::Call { callee, .. } => { - let ty = &infer.type_of_expr[*callee]; + let ty = &infer[*callee]; if let &Ty::Apply(ApplicationTy { ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), .. @@ -361,7 +359,7 @@ pub fn unsafe_expressions( if infer .method_resolution(id) .map(|func| db.function_data(func).is_unsafe) - .unwrap_or_else(|| false) + .unwrap_or(false) { unsafe_exprs.push(UnsafeExpr::new(id)); } @@ -409,7 +407,7 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { let func_data = db.function_data(self.func); if func_data.is_unsafe || unsafe_expressions - .into_iter() + .iter() .filter(|unsafe_expr| !unsafe_expr.inside_unsafe_block) .count() == 0 @@ -417,12 +415,11 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { return; } - let loc = self.func.lookup(db.upcast()); - let in_file = loc.source(db.upcast()); - - let file = in_file.file_id; - let fn_def = AstPtr::new(&in_file.value); - - self.sink.push(MissingUnsafe { file, fn_def }) + let (_, body_source) = db.body_with_source_map(def); + for unsafe_expr in unsafe_expressions { + if let Ok(in_file) = body_source.as_ref().expr_syntax(unsafe_expr.expr) { + self.sink.push(MissingUnsafe { file: in_file.file_id, expr: in_file.value }) + } + } } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 4bc2e8b27..26b3aeb50 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -552,7 +552,7 @@ fn missing_unsafe() { .diagnostics() .0; - assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n let x = &5 as *const usize;\n let y = *x;\n}": Missing unsafe keyword on fn"#); + assert_snapshot!(diagnostics, @r#""*x": This operation is unsafe and requires an unsafe function or block"#); } #[test] @@ -573,7 +573,7 @@ fn missing_unsafe() { .diagnostics() .0; - assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n unsafe_fn();\n}": Missing unsafe keyword on fn"#); + assert_snapshot!(diagnostics, @r#""unsafe_fn()": This operation is unsafe and requires an unsafe function or block"#); } #[test] @@ -599,7 +599,7 @@ fn missing_unsafe() { .diagnostics() .0; - assert_snapshot!(diagnostics, @r#""fn missing_unsafe() {\n HasUnsafe.unsafe_fn();\n}": Missing unsafe keyword on fn"#); + assert_snapshot!(diagnostics, @r#""HasUnsafe.unsafe_fn()": This operation is unsafe and requires an unsafe function or block"#); } #[test] -- cgit v1.2.3 From 7f2219dc76d6cddc46eadb2b2dd674795e0efce8 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 27 May 2020 22:21:20 -0400 Subject: Track expr parents during lowering, use parent map when checking if unsafe exprs are within unsafe blocks --- crates/ra_hir_ty/src/expr.rs | 22 +++++++++++----------- crates/ra_hir_ty/src/tests.rs | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 5f332aadb..3942aada5 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -333,15 +333,12 @@ pub fn unsafe_expressions( def: DefWithBodyId, ) -> Vec { let mut unsafe_exprs = vec![]; - let mut unsafe_block_scopes = vec![]; + let mut unsafe_block_exprs = FxHashSet::default(); let body = db.body(def); - let expr_scopes = db.expr_scopes(def); for (id, expr) in body.exprs.iter() { match expr { - Expr::Unsafe { body } => { - if let Some(scope) = expr_scopes.scope_for(*body) { - unsafe_block_scopes.push(scope); - } + Expr::Unsafe { .. } => { + unsafe_block_exprs.insert(id); } Expr::Call { callee, .. } => { let ty = &infer[*callee]; @@ -374,12 +371,13 @@ pub fn unsafe_expressions( } 'unsafe_exprs: for unsafe_expr in &mut unsafe_exprs { - let scope = expr_scopes.scope_for(unsafe_expr.expr); - for scope in expr_scopes.scope_chain(scope) { - if unsafe_block_scopes.contains(&scope) { + let mut child = unsafe_expr.expr; + while let Some(parent) = body.parent_map.get(&child) { + if unsafe_block_exprs.contains(parent) { unsafe_expr.inside_unsafe_block = true; continue 'unsafe_exprs; } + child = *parent; } } @@ -417,8 +415,10 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { let (_, body_source) = db.body_with_source_map(def); for unsafe_expr in unsafe_expressions { - if let Ok(in_file) = body_source.as_ref().expr_syntax(unsafe_expr.expr) { - self.sink.push(MissingUnsafe { file: in_file.file_id, expr: in_file.value }) + if !unsafe_expr.inside_unsafe_block { + if let Ok(in_file) = body_source.as_ref().expr_syntax(unsafe_expr.expr) { + self.sink.push(MissingUnsafe { file: in_file.file_id, expr: in_file.value }) + } } } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 26b3aeb50..496cb428b 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -638,7 +638,7 @@ fn nothing_to_see_move_along() { .diagnostics() .0; - assert_snapshot!(diagnostics, @""); + assert_snapshot!(diagnostics, @r#""*x": This operation is unsafe and requires an unsafe function or block"#); } #[test] -- cgit v1.2.3 From f678e0d837e472dc2f1421f89f794d33f3ade55c Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 28 May 2020 09:30:19 -0400 Subject: Add HighlightTag::Operator, use it for unsafe deref. Move unsafe validation to its own file --- crates/ra_hir_ty/src/expr.rs | 48 ++--------------------- crates/ra_hir_ty/src/lib.rs | 1 + crates/ra_hir_ty/src/test_db.rs | 5 +-- crates/ra_hir_ty/src/unsafe_validation.rs | 63 +++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 48 deletions(-) create mode 100644 crates/ra_hir_ty/src/unsafe_validation.rs (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 3942aada5..99eed949f 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -9,9 +9,7 @@ use rustc_hash::FxHashSet; use crate::{ db::HirDatabase, - diagnostics::{ - MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields, MissingUnsafe, - }, + diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields}, lower::CallableDef, utils::variant_data, ApplicationTy, InferenceResult, Ty, TypeCtor, @@ -317,8 +315,8 @@ pub fn record_pattern_missing_fields( } pub struct UnsafeExpr { - expr: ExprId, - inside_unsafe_block: bool, + pub expr: ExprId, + pub inside_unsafe_block: bool, } impl UnsafeExpr { @@ -383,43 +381,3 @@ pub fn unsafe_expressions( unsafe_exprs } - -pub struct UnsafeValidator<'a, 'b: 'a> { - func: FunctionId, - infer: Arc, - sink: &'a mut DiagnosticSink<'b>, -} - -impl<'a, 'b> UnsafeValidator<'a, 'b> { - pub fn new( - func: FunctionId, - infer: Arc, - sink: &'a mut DiagnosticSink<'b>, - ) -> UnsafeValidator<'a, 'b> { - UnsafeValidator { func, infer, sink } - } - - pub fn validate_body(&mut self, db: &dyn HirDatabase) { - let def = self.func.into(); - let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); - let func_data = db.function_data(self.func); - if func_data.is_unsafe - || unsafe_expressions - .iter() - .filter(|unsafe_expr| !unsafe_expr.inside_unsafe_block) - .count() - == 0 - { - return; - } - - let (_, body_source) = db.body_with_source_map(def); - for unsafe_expr in unsafe_expressions { - if !unsafe_expr.inside_unsafe_block { - if let Ok(in_file) = body_source.as_ref().expr_syntax(unsafe_expr.expr) { - self.sink.push(MissingUnsafe { file: in_file.file_id, expr: in_file.value }) - } - } - } - } -} diff --git a/crates/ra_hir_ty/src/lib.rs b/crates/ra_hir_ty/src/lib.rs index f22232324..414158139 100644 --- a/crates/ra_hir_ty/src/lib.rs +++ b/crates/ra_hir_ty/src/lib.rs @@ -37,6 +37,7 @@ pub(crate) mod utils; pub mod db; pub mod diagnostics; pub mod expr; +pub mod unsafe_validation; #[cfg(test)] mod tests; diff --git a/crates/ra_hir_ty/src/test_db.rs b/crates/ra_hir_ty/src/test_db.rs index 9ccf2aa37..9c2c6959d 100644 --- a/crates/ra_hir_ty/src/test_db.rs +++ b/crates/ra_hir_ty/src/test_db.rs @@ -12,9 +12,8 @@ use rustc_hash::FxHashSet; use stdx::format_to; use crate::{ - db::HirDatabase, - diagnostics::Diagnostic, - expr::{ExprValidator, UnsafeValidator}, + db::HirDatabase, diagnostics::Diagnostic, expr::ExprValidator, + unsafe_validation::UnsafeValidator, }; #[salsa::database( diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs new file mode 100644 index 000000000..55dbe23fa --- /dev/null +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -0,0 +1,63 @@ +//! Provides validations for unsafe code. Currently checks if unsafe functions are missing +//! unsafe blocks. + +use std::sync::Arc; + +use hir_def::FunctionId; +use hir_expand::diagnostics::DiagnosticSink; + +use crate::{ + db::HirDatabase, diagnostics::MissingUnsafe, expr::unsafe_expressions, InferenceResult, +}; + +pub use hir_def::{ + body::{ + scope::{ExprScopes, ScopeEntry, ScopeId}, + Body, BodySourceMap, ExprPtr, ExprSource, PatPtr, PatSource, + }, + expr::{ + ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, + MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, UnaryOp, + }, + LocalFieldId, VariantId, +}; + +pub struct UnsafeValidator<'a, 'b: 'a> { + func: FunctionId, + infer: Arc, + sink: &'a mut DiagnosticSink<'b>, +} + +impl<'a, 'b> UnsafeValidator<'a, 'b> { + pub fn new( + func: FunctionId, + infer: Arc, + sink: &'a mut DiagnosticSink<'b>, + ) -> UnsafeValidator<'a, 'b> { + UnsafeValidator { func, infer, sink } + } + + pub fn validate_body(&mut self, db: &dyn HirDatabase) { + let def = self.func.into(); + let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); + let func_data = db.function_data(self.func); + if func_data.is_unsafe + || unsafe_expressions + .iter() + .filter(|unsafe_expr| !unsafe_expr.inside_unsafe_block) + .count() + == 0 + { + return; + } + + let (_, body_source) = db.body_with_source_map(def); + for unsafe_expr in unsafe_expressions { + if !unsafe_expr.inside_unsafe_block { + if let Ok(in_file) = body_source.as_ref().expr_syntax(unsafe_expr.expr) { + self.sink.push(MissingUnsafe { file: in_file.file_id, expr: in_file.value }) + } + } + } + } +} -- cgit v1.2.3 From 2608a6fd3a024206d4776cc391e46ef28c018434 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Fri, 29 May 2020 08:55:47 -0400 Subject: unsafe: Clean up, improve tracking, add debug_assert Move unsafe_expressions to unsafe_validation.rs, replace vec tracking of child exprs with inline macro, add debug assert to ensure tracked children match walked children exactly --- crates/ra_hir_ty/src/expr.rs | 71 +---------------------------- crates/ra_hir_ty/src/unsafe_validation.rs | 75 ++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 72 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 99eed949f..7db928dde 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -2,7 +2,7 @@ use std::sync::Arc; -use hir_def::{path::path, resolver::HasResolver, AdtId, DefWithBodyId, FunctionId}; +use hir_def::{path::path, resolver::HasResolver, AdtId, FunctionId}; use hir_expand::diagnostics::DiagnosticSink; use ra_syntax::{ast, AstPtr}; use rustc_hash::FxHashSet; @@ -10,7 +10,6 @@ use rustc_hash::FxHashSet; use crate::{ db::HirDatabase, diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields}, - lower::CallableDef, utils::variant_data, ApplicationTy, InferenceResult, Ty, TypeCtor, _match::{is_useful, MatchCheckCtx, Matrix, PatStack, Usefulness}, @@ -313,71 +312,3 @@ pub fn record_pattern_missing_fields( } Some((variant_def, missed_fields, exhaustive)) } - -pub struct UnsafeExpr { - pub expr: ExprId, - pub inside_unsafe_block: bool, -} - -impl UnsafeExpr { - fn new(expr: ExprId) -> Self { - Self { expr, inside_unsafe_block: false } - } -} - -pub fn unsafe_expressions( - db: &dyn HirDatabase, - infer: &InferenceResult, - def: DefWithBodyId, -) -> Vec { - let mut unsafe_exprs = vec![]; - let mut unsafe_block_exprs = FxHashSet::default(); - let body = db.body(def); - for (id, expr) in body.exprs.iter() { - match expr { - Expr::Unsafe { .. } => { - unsafe_block_exprs.insert(id); - } - Expr::Call { callee, .. } => { - let ty = &infer[*callee]; - if let &Ty::Apply(ApplicationTy { - ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), - .. - }) = ty - { - if db.function_data(func).is_unsafe { - unsafe_exprs.push(UnsafeExpr::new(id)); - } - } - } - Expr::MethodCall { .. } => { - if infer - .method_resolution(id) - .map(|func| db.function_data(func).is_unsafe) - .unwrap_or(false) - { - unsafe_exprs.push(UnsafeExpr::new(id)); - } - } - Expr::UnaryOp { expr, op: UnaryOp::Deref } => { - if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { - unsafe_exprs.push(UnsafeExpr::new(id)); - } - } - _ => {} - } - } - - 'unsafe_exprs: for unsafe_expr in &mut unsafe_exprs { - let mut child = unsafe_expr.expr; - while let Some(parent) = body.parent_map.get(&child) { - if unsafe_block_exprs.contains(parent) { - unsafe_expr.inside_unsafe_block = true; - continue 'unsafe_exprs; - } - child = *parent; - } - } - - unsafe_exprs -} diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index 55dbe23fa..430182803 100644 --- a/crates/ra_hir_ty/src/unsafe_validation.rs +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -3,13 +3,16 @@ use std::sync::Arc; -use hir_def::FunctionId; +use hir_def::{DefWithBodyId, FunctionId}; use hir_expand::diagnostics::DiagnosticSink; use crate::{ - db::HirDatabase, diagnostics::MissingUnsafe, expr::unsafe_expressions, InferenceResult, + db::HirDatabase, diagnostics::MissingUnsafe, lower::CallableDef, ApplicationTy, + InferenceResult, Ty, TypeCtor, }; +use rustc_hash::FxHashSet; + pub use hir_def::{ body::{ scope::{ExprScopes, ScopeEntry, ScopeId}, @@ -61,3 +64,71 @@ impl<'a, 'b> UnsafeValidator<'a, 'b> { } } } + +pub struct UnsafeExpr { + pub expr: ExprId, + pub inside_unsafe_block: bool, +} + +impl UnsafeExpr { + fn new(expr: ExprId) -> Self { + Self { expr, inside_unsafe_block: false } + } +} + +pub fn unsafe_expressions( + db: &dyn HirDatabase, + infer: &InferenceResult, + def: DefWithBodyId, +) -> Vec { + let mut unsafe_exprs = vec![]; + let mut unsafe_block_exprs = FxHashSet::default(); + let body = db.body(def); + for (id, expr) in body.exprs.iter() { + match expr { + Expr::Unsafe { .. } => { + unsafe_block_exprs.insert(id); + } + Expr::Call { callee, .. } => { + let ty = &infer[*callee]; + if let &Ty::Apply(ApplicationTy { + ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), + .. + }) = ty + { + if db.function_data(func).is_unsafe { + unsafe_exprs.push(UnsafeExpr::new(id)); + } + } + } + Expr::MethodCall { .. } => { + if infer + .method_resolution(id) + .map(|func| db.function_data(func).is_unsafe) + .unwrap_or(false) + { + unsafe_exprs.push(UnsafeExpr::new(id)); + } + } + Expr::UnaryOp { expr, op: UnaryOp::Deref } => { + if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { + unsafe_exprs.push(UnsafeExpr::new(id)); + } + } + _ => {} + } + } + + 'unsafe_exprs: for unsafe_expr in &mut unsafe_exprs { + let mut child = unsafe_expr.expr; + while let Some(parent) = body.parent_map.get(child) { + if unsafe_block_exprs.contains(parent) { + unsafe_expr.inside_unsafe_block = true; + continue 'unsafe_exprs; + } + child = *parent; + } + } + + unsafe_exprs +} -- cgit v1.2.3 From 2fc92fa28cda858c25fea2e378c2eb73e7f65247 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Tue, 2 Jun 2020 18:44:04 -0400 Subject: Remove track_parent and parent_map, replace with simple walk in missign unsafe validator --- crates/ra_hir_ty/src/unsafe_validation.rs | 42 +++++++++++++++++-------------- 1 file changed, 23 insertions(+), 19 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index 430182803..f3ce7112a 100644 --- a/crates/ra_hir_ty/src/unsafe_validation.rs +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -13,16 +13,9 @@ use crate::{ use rustc_hash::FxHashSet; -pub use hir_def::{ - body::{ - scope::{ExprScopes, ScopeEntry, ScopeId}, - Body, BodySourceMap, ExprPtr, ExprSource, PatPtr, PatSource, - }, - expr::{ - ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, - MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, UnaryOp, - }, - LocalFieldId, VariantId, +use hir_def::{ + body::Body, + expr::{Expr, ExprId, UnaryOp}, }; pub struct UnsafeValidator<'a, 'b: 'a> { @@ -119,16 +112,27 @@ pub fn unsafe_expressions( } } - 'unsafe_exprs: for unsafe_expr in &mut unsafe_exprs { - let mut child = unsafe_expr.expr; - while let Some(parent) = body.parent_map.get(child) { - if unsafe_block_exprs.contains(parent) { - unsafe_expr.inside_unsafe_block = true; - continue 'unsafe_exprs; - } - child = *parent; - } + for unsafe_expr in &mut unsafe_exprs { + unsafe_expr.inside_unsafe_block = + is_in_unsafe(&body, body.body_expr, unsafe_expr.expr, false); } unsafe_exprs } + +fn is_in_unsafe(body: &Body, current: ExprId, needle: ExprId, within_unsafe: bool) -> bool { + if current == needle { + return within_unsafe; + } + + let expr = &body.exprs[current]; + if let &Expr::Unsafe { body: child } = expr { + return is_in_unsafe(body, child, needle, true); + } + + let mut found = false; + expr.walk_child_exprs(|child| { + found = found || is_in_unsafe(body, child, needle, within_unsafe); + }); + found +} -- cgit v1.2.3 From b1992b469cae689f7de01ea9031962735a409198 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 11:20:02 -0400 Subject: Remove unneeded code, filename from tests, fix rebasing issues --- crates/ra_hir_ty/src/tests.rs | 5 ----- crates/ra_hir_ty/src/unsafe_validation.rs | 17 +++++------------ 2 files changed, 5 insertions(+), 17 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 496cb428b..2a85ce85d 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -580,7 +580,6 @@ fn missing_unsafe() { fn missing_unsafe_diagnostic_with_unsafe_method_call() { let diagnostics = TestDB::with_files( r" -//- /lib.rs struct HasUnsafe; impl HasUnsafe { @@ -606,7 +605,6 @@ fn missing_unsafe() { fn no_missing_unsafe_diagnostic_with_raw_ptr_in_unsafe_block() { let diagnostics = TestDB::with_files( r" -//- /lib.rs fn nothing_to_see_move_along() { let x = &5 as *const usize; unsafe { @@ -625,7 +623,6 @@ fn nothing_to_see_move_along() { fn missing_unsafe_diagnostic_with_raw_ptr_outside_unsafe_block() { let diagnostics = TestDB::with_files( r" -//- /lib.rs fn nothing_to_see_move_along() { let x = &5 as *const usize; unsafe { @@ -645,7 +642,6 @@ fn nothing_to_see_move_along() { fn no_missing_unsafe_diagnostic_with_unsafe_call_in_unsafe_block() { let diagnostics = TestDB::with_files( r" -//- /lib.rs unsafe fn unsafe_fn() { let x = &5 as *const usize; let y = *x; @@ -668,7 +664,6 @@ fn nothing_to_see_move_along() { fn no_missing_unsafe_diagnostic_with_unsafe_method_call_in_unsafe_block() { let diagnostics = TestDB::with_files( r" -//- /lib.rs struct HasUnsafe; impl HasUnsafe { diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index f3ce7112a..e2353404b 100644 --- a/crates/ra_hir_ty/src/unsafe_validation.rs +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -3,7 +3,11 @@ use std::sync::Arc; -use hir_def::{DefWithBodyId, FunctionId}; +use hir_def::{ + body::Body, + expr::{Expr, ExprId, UnaryOp}, + DefWithBodyId, FunctionId, +}; use hir_expand::diagnostics::DiagnosticSink; use crate::{ @@ -11,13 +15,6 @@ use crate::{ InferenceResult, Ty, TypeCtor, }; -use rustc_hash::FxHashSet; - -use hir_def::{ - body::Body, - expr::{Expr, ExprId, UnaryOp}, -}; - pub struct UnsafeValidator<'a, 'b: 'a> { func: FunctionId, infer: Arc, @@ -75,13 +72,9 @@ pub fn unsafe_expressions( def: DefWithBodyId, ) -> Vec { let mut unsafe_exprs = vec![]; - let mut unsafe_block_exprs = FxHashSet::default(); let body = db.body(def); for (id, expr) in body.exprs.iter() { match expr { - Expr::Unsafe { .. } => { - unsafe_block_exprs.insert(id); - } Expr::Call { callee, .. } => { let ty = &infer[*callee]; if let &Ty::Apply(ApplicationTy { -- cgit v1.2.3 From b7e25ba854a5ca0f1dee7082c113170876358632 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 11:55:54 -0400 Subject: Improve perf of finding unsafe exprs --- crates/ra_hir_ty/src/unsafe_validation.rs | 88 ++++++++++++++----------------- 1 file changed, 39 insertions(+), 49 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index e2353404b..aad13d99c 100644 --- a/crates/ra_hir_ty/src/unsafe_validation.rs +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -60,12 +60,6 @@ pub struct UnsafeExpr { pub inside_unsafe_block: bool, } -impl UnsafeExpr { - fn new(expr: ExprId) -> Self { - Self { expr, inside_unsafe_block: false } - } -} - pub fn unsafe_expressions( db: &dyn HirDatabase, infer: &InferenceResult, @@ -73,59 +67,55 @@ pub fn unsafe_expressions( ) -> Vec { let mut unsafe_exprs = vec![]; let body = db.body(def); - for (id, expr) in body.exprs.iter() { - match expr { - Expr::Call { callee, .. } => { - let ty = &infer[*callee]; - if let &Ty::Apply(ApplicationTy { - ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), - .. - }) = ty - { - if db.function_data(func).is_unsafe { - unsafe_exprs.push(UnsafeExpr::new(id)); - } + walk_unsafe(&mut unsafe_exprs, db, infer, &body, body.body_expr, false); + + unsafe_exprs +} + +fn walk_unsafe( + unsafe_exprs: &mut Vec, + db: &dyn HirDatabase, + infer: &InferenceResult, + body: &Body, + current: ExprId, + inside_unsafe_block: bool, +) { + let expr = &body.exprs[current]; + match expr { + Expr::Call { callee, .. } => { + let ty = &infer[*callee]; + if let &Ty::Apply(ApplicationTy { + ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)), + .. + }) = ty + { + if db.function_data(func).is_unsafe { + unsafe_exprs.push(UnsafeExpr { expr: current, inside_unsafe_block }); } } - Expr::MethodCall { .. } => { - if infer - .method_resolution(id) - .map(|func| db.function_data(func).is_unsafe) - .unwrap_or(false) - { - unsafe_exprs.push(UnsafeExpr::new(id)); - } + } + Expr::MethodCall { .. } => { + if infer + .method_resolution(current) + .map(|func| db.function_data(func).is_unsafe) + .unwrap_or(false) + { + unsafe_exprs.push(UnsafeExpr { expr: current, inside_unsafe_block }); } - Expr::UnaryOp { expr, op: UnaryOp::Deref } => { - if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { - unsafe_exprs.push(UnsafeExpr::new(id)); - } + } + Expr::UnaryOp { expr, op: UnaryOp::Deref } => { + if let Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }) = &infer[*expr] { + unsafe_exprs.push(UnsafeExpr { expr: current, inside_unsafe_block }); } - _ => {} } + _ => {} } - for unsafe_expr in &mut unsafe_exprs { - unsafe_expr.inside_unsafe_block = - is_in_unsafe(&body, body.body_expr, unsafe_expr.expr, false); - } - - unsafe_exprs -} - -fn is_in_unsafe(body: &Body, current: ExprId, needle: ExprId, within_unsafe: bool) -> bool { - if current == needle { - return within_unsafe; - } - - let expr = &body.exprs[current]; if let &Expr::Unsafe { body: child } = expr { - return is_in_unsafe(body, child, needle, true); + return walk_unsafe(unsafe_exprs, db, infer, body, child, true); } - let mut found = false; expr.walk_child_exprs(|child| { - found = found || is_in_unsafe(body, child, needle, within_unsafe); + walk_unsafe(unsafe_exprs, db, infer, body, child, inside_unsafe_block); }); - found } -- cgit v1.2.3 From 68a649d547c844cb44ee619b7f45d1193dad2b02 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 12:00:46 -0400 Subject: Simplify unsafe expr collection match --- crates/ra_hir_ty/src/unsafe_validation.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index aad13d99c..c512c4f8e 100644 --- a/crates/ra_hir_ty/src/unsafe_validation.rs +++ b/crates/ra_hir_ty/src/unsafe_validation.rs @@ -108,13 +108,12 @@ fn walk_unsafe( unsafe_exprs.push(UnsafeExpr { expr: current, inside_unsafe_block }); } } + Expr::Unsafe { body: child } => { + return walk_unsafe(unsafe_exprs, db, infer, body, *child, true); + } _ => {} } - if let &Expr::Unsafe { body: child } = expr { - return walk_unsafe(unsafe_exprs, db, infer, body, child, true); - } - expr.walk_child_exprs(|child| { walk_unsafe(unsafe_exprs, db, infer, body, child, inside_unsafe_block); }); -- cgit v1.2.3