From 0b95bed83fc8db897f54b350168567f14527e8de Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 23 May 2020 17:49:53 -0400 Subject: [PATCH 01/21] Add unsafe diagnostics and unsafe highlighting --- crates/ra_hir/src/code_model.rs | 5 +- crates/ra_hir/src/diagnostics.rs | 50 ++++++++++++++++ crates/ra_hir_ty/src/diagnostics.rs | 58 +++++++++++++++++++ crates/ra_hir_ty/src/expr.rs | 24 +++++++- .../ra_ide/src/syntax_highlighting/tests.rs | 28 +++++++++ 5 files changed, 163 insertions(+), 2 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index a379b9f49b..131180a638 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -36,6 +36,7 @@ use rustc_hash::FxHashSet; use crate::{ db::{DefDatabase, HirDatabase}, + diagnostics::UnsafeValidator, has_source::HasSource, CallableDef, HirDisplay, InFile, Name, }; @@ -677,7 +678,9 @@ impl Function { let _p = profile("Function::diagnostics"); let infer = db.infer(self.id.into()); infer.add_diagnostics(db, self.id, sink); - let mut validator = ExprValidator::new(self.id, infer, sink); + let mut validator = ExprValidator::new(self.id, infer.clone(), sink); + validator.validate_body(db); + let mut validator = UnsafeValidator::new(&self, infer, sink); validator.validate_body(db); } } diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index c82883d0c1..562f3fe5c4 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -2,3 +2,53 @@ pub use hir_def::diagnostics::UnresolvedModule; pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink}; pub use hir_ty::diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, NoSuchField}; + +use std::sync::Arc; + +use crate::code_model::Function; +use crate::db::HirDatabase; +use crate::has_source::HasSource; +use hir_ty::{ + diagnostics::{MissingUnsafe, UnnecessaryUnsafe}, + expr::unsafe_expressions, + InferenceResult, +}; +use ra_syntax::AstPtr; + +pub struct UnsafeValidator<'a, 'b: 'a> { + func: &'a Function, + infer: Arc, + sink: &'a mut DiagnosticSink<'b>, +} + +impl<'a, 'b> UnsafeValidator<'a, 'b> { + pub fn new( + func: &'a Function, + 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.id.into(); + let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); + let func_data = db.function_data(self.func.id); + 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 in_file = self.func.source(db); + let file = in_file.file_id; + let fn_def = AstPtr::new(&in_file.value); + let fn_name = func_data.name.clone().into(); + + if unnecessary { + self.sink.push(UnnecessaryUnsafe { file, fn_def, fn_name }) + } else { + self.sink.push(MissingUnsafe { file, fn_def, fn_name }) + } + } +} diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index ebd9cb08f9..3469cc6806 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 7db928dded..795f1762c5 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 +} diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs index b7fad97197..39cd74ac3a 100644 --- a/crates/ra_ide/src/syntax_highlighting/tests.rs +++ b/crates/ra_ide/src/syntax_highlighting/tests.rs @@ -370,3 +370,31 @@ fn check_highlighting(ra_fixture: &str, snapshot: &str, rainbow: bool) { fs::write(dst_file, &actual_html).unwrap(); assert_eq_text!(expected_html, actual_html); } + +#[test] +fn test_unsafe_highlighting() { + let (analysis, file_id) = single_file( + r#" +unsafe fn unsafe_fn() {} + +struct HasUnsafeFn; + +impl HasUnsafeFn { + unsafe fn unsafe_method(&self) {} +} + +fn main() { + unsafe { + unsafe_fn(); + HasUnsafeFn.unsafe_method(); + } +} +"# + .trim(), + ); + let dst_file = project_dir().join("crates/ra_ide/src/snapshots/highlight_unsafe.html"); + let actual_html = &analysis.highlight_as_html(file_id, false).unwrap(); + let expected_html = &read_text(&dst_file); + fs::write(dst_file, &actual_html).unwrap(); + assert_eq_text!(expected_html, actual_html); +} From daf1cac9f87023d37a4418ea24ed615c9706258b Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 01:33:22 -0400 Subject: [PATCH 02/21] Move diagnostics back into expr, add tests for diagnostics, fix logic to account for derefs of raw ptrs --- crates/ra_hir/src/code_model.rs | 5 +- crates/ra_hir/src/diagnostics.rs | 50 ------------ 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 +++++++++++++++++++ .../ra_ide/src/syntax_highlighting/tests.rs | 2 + 7 files changed, 158 insertions(+), 73 deletions(-) diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 131180a638..13e763e520 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -25,7 +25,7 @@ use hir_expand::{ use hir_ty::{ autoderef, display::{HirDisplayError, HirFormatter}, - expr::ExprValidator, + expr::{ExprValidator, UnsafeValidator}, method_resolution, ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs, TraitEnvironment, Ty, TyDefId, TypeCtor, }; @@ -36,7 +36,6 @@ use rustc_hash::FxHashSet; use crate::{ db::{DefDatabase, HirDatabase}, - diagnostics::UnsafeValidator, has_source::HasSource, CallableDef, HirDisplay, InFile, Name, }; @@ -680,7 +679,7 @@ impl Function { infer.add_diagnostics(db, self.id, sink); let mut validator = ExprValidator::new(self.id, infer.clone(), sink); validator.validate_body(db); - let mut validator = UnsafeValidator::new(&self, infer, sink); + let mut validator = UnsafeValidator::new(self.id, infer, sink); validator.validate_body(db); } } diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index 562f3fe5c4..c82883d0c1 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -2,53 +2,3 @@ pub use hir_def::diagnostics::UnresolvedModule; pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink}; pub use hir_ty::diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, NoSuchField}; - -use std::sync::Arc; - -use crate::code_model::Function; -use crate::db::HirDatabase; -use crate::has_source::HasSource; -use hir_ty::{ - diagnostics::{MissingUnsafe, UnnecessaryUnsafe}, - expr::unsafe_expressions, - InferenceResult, -}; -use ra_syntax::AstPtr; - -pub struct UnsafeValidator<'a, 'b: 'a> { - func: &'a Function, - infer: Arc, - sink: &'a mut DiagnosticSink<'b>, -} - -impl<'a, 'b> UnsafeValidator<'a, 'b> { - pub fn new( - func: &'a Function, - 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.id.into(); - let unsafe_expressions = unsafe_expressions(db, self.infer.as_ref(), def); - let func_data = db.function_data(self.func.id); - 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 in_file = self.func.source(db); - let file = in_file.file_id; - let fn_def = AstPtr::new(&in_file.value); - let fn_name = func_data.name.clone().into(); - - if unnecessary { - self.sink.push(UnnecessaryUnsafe { file, fn_def, fn_name }) - } else { - self.sink.push(MissingUnsafe { file, fn_def, fn_name }) - } - } -} diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 3469cc6806..c6ca322fa3 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 795f1762c5..7532e2dc7c 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 ad04e3e0f9..9ccf2aa377 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 85ff26a368..4ff2b2d4a5 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( diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs index 39cd74ac3a..43f554a292 100644 --- a/crates/ra_ide/src/syntax_highlighting/tests.rs +++ b/crates/ra_ide/src/syntax_highlighting/tests.rs @@ -384,9 +384,11 @@ impl HasUnsafeFn { } fn main() { + let x = &5 as *usize; unsafe { unsafe_fn(); HasUnsafeFn.unsafe_method(); + let y = *x; } } "# From b358fbfdf82409700a8a328794429ec790306fc2 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 02:10:34 -0400 Subject: [PATCH 03/21] 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(-) diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 7532e2dc7c..04668f486c 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 4ff2b2d4a5..c1f6fbab83 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( From 499d4c454d1a66d10c3cf4c9bacbdb15f295af39 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 13:18:31 -0400 Subject: [PATCH 04/21] 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 +++++------------ crates/ra_syntax/src/ast/generated/nodes.rs | 2 +- 4 files changed, 22 insertions(+), 70 deletions(-) diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index c6ca322fa3..0b7bcdc9de 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 04668f486c..1a0710e0b6 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 c1f6fbab83..28faccf383 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( diff --git a/crates/ra_syntax/src/ast/generated/nodes.rs b/crates/ra_syntax/src/ast/generated/nodes.rs index 58141da114..a2366f9977 100644 --- a/crates/ra_syntax/src/ast/generated/nodes.rs +++ b/crates/ra_syntax/src/ast/generated/nodes.rs @@ -899,7 +899,7 @@ impl ast::LoopBodyOwner for LoopExpr {} impl LoopExpr { pub fn loop_token(&self) -> Option { support::token(&self.syntax, T![loop]) } } -/// Block expression with an optional prefix (label, try ketword, +/// Block expression with an optional prefix (label, try keyword, /// unsafe keyword, async keyword...). /// /// ``` From c622551ec27c9d68e8310c2408317253952b6f44 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 13:20:03 -0400 Subject: [PATCH 05/21] Fix typo in test --- crates/ra_ide/src/syntax_highlighting/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs index 43f554a292..e9a6fbcdfe 100644 --- a/crates/ra_ide/src/syntax_highlighting/tests.rs +++ b/crates/ra_ide/src/syntax_highlighting/tests.rs @@ -384,7 +384,7 @@ impl HasUnsafeFn { } fn main() { - let x = &5 as *usize; + let x = &5 as *const usize; unsafe { unsafe_fn(); HasUnsafeFn.unsafe_method(); From 3df0f9ce7e6eea48b67dae8b26e83aa7bd36ff24 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 15:31:33 -0400 Subject: [PATCH 06/21] Add missing self param to test --- crates/ra_hir_ty/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 28faccf383..8889fa3ba5 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; } From 278cbf12cd0f76fc191d5ce7f130e6245596a578 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 16:24:36 -0400 Subject: [PATCH 07/21] Track unsafe blocks, don't trigger missing unsafe diagnostic when unsafe exprs within unsafe block --- crates/ra_hir_def/src/body/lower.rs | 6 +- crates/ra_hir_def/src/expr.rs | 5 +- 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 + crates/ra_syntax/src/ast/generated/nodes.rs | 2 +- 7 files changed, 82 insertions(+), 19 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index a7e2e09822..174bbf9f47 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -218,8 +218,12 @@ impl ExprCollector<'_> { let body = self.collect_block_opt(e.block_expr()); self.alloc_expr(Expr::TryBlock { body }, syntax_ptr) } + ast::Effect::Unsafe(_) => { + let body = self.collect_block_opt(e.block_expr()); + self.alloc_expr(Expr::UnsafeBlock { body }, syntax_ptr) + } // FIXME: we need to record these effects somewhere... - ast::Effect::Async(_) | ast::Effect::Label(_) | ast::Effect::Unsafe(_) => { + ast::Effect::Async(_) | ast::Effect::Label(_) => { self.collect_block_opt(e.block_expr()) } }, diff --git a/crates/ra_hir_def/src/expr.rs b/crates/ra_hir_def/src/expr.rs index ca49b26d15..f5e3e74fb5 100644 --- a/crates/ra_hir_def/src/expr.rs +++ b/crates/ra_hir_def/src/expr.rs @@ -150,6 +150,9 @@ pub enum Expr { Tuple { exprs: Vec, }, + UnsafeBlock { + body: ExprId, + }, Array(Array), Literal(Literal), } @@ -247,7 +250,7 @@ impl Expr { f(*expr); } } - Expr::TryBlock { body } => f(*body), + Expr::TryBlock { body } | Expr::UnsafeBlock { body } => f(*body), Expr::Loop { body, .. } => f(*body), Expr::While { condition, body, .. } => { f(*condition); diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 1a0710e0b6..d600aca215 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 a9565a58d1..df7bcb5fa9 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 8889fa3ba5..4bc2e8b276 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -608,8 +608,8 @@ 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 x = &5 as *const usize; let y = *x; } } @@ -621,6 +621,26 @@ fn nothing_to_see_move_along() { 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 y = *x; + } + let z = *x; +} +", + ) + .diagnostics() + .0; + + assert_snapshot!(diagnostics, @""); +} + #[test] fn no_missing_unsafe_diagnostic_with_unsafe_call_in_unsafe_block() { let diagnostics = TestDB::with_files( diff --git a/crates/ra_hir_ty/src/tests/simple.rs b/crates/ra_hir_ty/src/tests/simple.rs index 7d8197f8b0..cd919466f4 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} diff --git a/crates/ra_syntax/src/ast/generated/nodes.rs b/crates/ra_syntax/src/ast/generated/nodes.rs index a2366f9977..58141da114 100644 --- a/crates/ra_syntax/src/ast/generated/nodes.rs +++ b/crates/ra_syntax/src/ast/generated/nodes.rs @@ -899,7 +899,7 @@ impl ast::LoopBodyOwner for LoopExpr {} impl LoopExpr { pub fn loop_token(&self) -> Option { support::token(&self.syntax, T![loop]) } } -/// Block expression with an optional prefix (label, try keyword, +/// Block expression with an optional prefix (label, try ketword, /// unsafe keyword, async keyword...). /// /// ``` From b9569886a915f2411adaecbcad28eda8b163c3e3 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sun, 24 May 2020 16:28:02 -0400 Subject: [PATCH 08/21] Rename Expr::UnsafeBlock to Expr::Unsafe --- crates/ra_hir_def/src/body/lower.rs | 2 +- crates/ra_hir_def/src/expr.rs | 4 ++-- crates/ra_hir_ty/src/expr.rs | 2 +- crates/ra_hir_ty/src/infer/expr.rs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 174bbf9f47..6eb72e9504 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -220,7 +220,7 @@ impl ExprCollector<'_> { } ast::Effect::Unsafe(_) => { let body = self.collect_block_opt(e.block_expr()); - self.alloc_expr(Expr::UnsafeBlock { body }, syntax_ptr) + self.alloc_expr(Expr::Unsafe { body }, syntax_ptr) } // FIXME: we need to record these effects somewhere... ast::Effect::Async(_) | ast::Effect::Label(_) => { diff --git a/crates/ra_hir_def/src/expr.rs b/crates/ra_hir_def/src/expr.rs index f5e3e74fb5..e41cfc16b9 100644 --- a/crates/ra_hir_def/src/expr.rs +++ b/crates/ra_hir_def/src/expr.rs @@ -150,7 +150,7 @@ pub enum Expr { Tuple { exprs: Vec, }, - UnsafeBlock { + Unsafe { body: ExprId, }, Array(Array), @@ -250,7 +250,7 @@ impl Expr { f(*expr); } } - Expr::TryBlock { body } | Expr::UnsafeBlock { body } => f(*body), + Expr::TryBlock { body } | Expr::Unsafe { body } => f(*body), Expr::Loop { body, .. } => f(*body), Expr::While { condition, body, .. } => { f(*condition); diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index d600aca215..ce73251b88 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 df7bcb5fa9..61af5f0645 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}, _> From 9ce44be2ab7e9a99eece1c2e254f15ad1c6d73c5 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 27 May 2020 08:51:08 -0400 Subject: [PATCH 09/21] 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(-) diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 0b7bcdc9de..a59efb3476 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 ce73251b88..5f332aadbb 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 4bc2e8b276..26b3aeb50e 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] From 7f2219dc76d6cddc46eadb2b2dd674795e0efce8 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 27 May 2020 22:21:20 -0400 Subject: [PATCH 10/21] Track expr parents during lowering, use parent map when checking if unsafe exprs are within unsafe blocks --- crates/ra_hir_def/src/body.rs | 1 + crates/ra_hir_def/src/body/lower.rs | 252 ++++++++++++++++++---------- crates/ra_hir_ty/src/expr.rs | 22 +-- crates/ra_hir_ty/src/tests.rs | 2 +- 4 files changed, 177 insertions(+), 100 deletions(-) diff --git a/crates/ra_hir_def/src/body.rs b/crates/ra_hir_def/src/body.rs index 4f2350915d..076d1a4fa9 100644 --- a/crates/ra_hir_def/src/body.rs +++ b/crates/ra_hir_def/src/body.rs @@ -184,6 +184,7 @@ pub struct Body { /// The `ExprId` of the actual body expression. pub body_expr: ExprId, pub item_scope: ItemScope, + pub parent_map: FxHashMap, } pub type ExprPtr = AstPtr; diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 6eb72e9504..a1678adeb8 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -15,6 +15,7 @@ use ra_syntax::{ }, AstNode, AstPtr, }; +use rustc_hash::FxHashMap; use test_utils::mark; use crate::{ @@ -74,6 +75,7 @@ pub(super) fn lower( params: Vec::new(), body_expr: dummy_expr_id(), item_scope: Default::default(), + parent_map: FxHashMap::default(), }, item_trees: { let mut map = FxHashMap::default(); @@ -171,11 +173,28 @@ impl ExprCollector<'_> { id } + fn update_parent_map( + &mut self, + (parent_expr, children_exprs): (ExprId, Vec), + ) -> ExprId { + for child_expr in children_exprs { + self.body.parent_map.insert(child_expr, parent_expr); + } + + parent_expr + } + fn collect_expr(&mut self, expr: ast::Expr) -> ExprId { + let parent_and_children = self.collect_expr_inner(expr); + self.update_parent_map(parent_and_children) + } + + fn collect_expr_inner(&mut self, expr: ast::Expr) -> (ExprId, Vec) { let syntax_ptr = AstPtr::new(&expr); if !self.expander.is_cfg_enabled(&expr) { - return self.missing_expr(); + return (self.missing_expr(), vec![]); } + match expr { ast::Expr::IfExpr(e) => { let then_branch = self.collect_block_opt(e.then_branch()); @@ -205,32 +224,48 @@ impl ExprCollector<'_> { guard: None, }, ]; - return self - .alloc_expr(Expr::Match { expr: match_expr, arms }, syntax_ptr); + let children_exprs = if let Some(else_branch) = else_branch { + vec![match_expr, then_branch, else_branch] + } else { + vec![match_expr, then_branch] + }; + return ( + self.alloc_expr(Expr::Match { expr: match_expr, arms }, syntax_ptr), + children_exprs, + ); } }, }; - self.alloc_expr(Expr::If { condition, then_branch, else_branch }, syntax_ptr) + let children_exprs = if let Some(else_branch) = else_branch { + vec![then_branch, else_branch, condition] + } else { + vec![then_branch, condition] + }; + + ( + self.alloc_expr(Expr::If { condition, then_branch, else_branch }, syntax_ptr), + children_exprs, + ) } ast::Expr::EffectExpr(e) => match e.effect() { ast::Effect::Try(_) => { let body = self.collect_block_opt(e.block_expr()); - self.alloc_expr(Expr::TryBlock { body }, syntax_ptr) + (self.alloc_expr(Expr::TryBlock { body }, syntax_ptr), vec![body]) } ast::Effect::Unsafe(_) => { let body = self.collect_block_opt(e.block_expr()); - self.alloc_expr(Expr::Unsafe { body }, syntax_ptr) + (self.alloc_expr(Expr::Unsafe { body }, syntax_ptr), vec![body]) } // FIXME: we need to record these effects somewhere... ast::Effect::Async(_) | ast::Effect::Label(_) => { - self.collect_block_opt(e.block_expr()) + (self.collect_block_opt(e.block_expr()), vec![]) } }, - ast::Expr::BlockExpr(e) => self.collect_block(e), + ast::Expr::BlockExpr(e) => (self.collect_block(e), vec![]), ast::Expr::LoopExpr(e) => { let body = self.collect_block_opt(e.loop_body()); - self.alloc_expr( + (self.alloc_expr( Expr::Loop { body, label: e @@ -239,7 +274,7 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ) + ), vec![body]) } ast::Expr::WhileExpr(e) => { let body = self.collect_block_opt(e.loop_body()); @@ -250,6 +285,7 @@ impl ExprCollector<'_> { None => self.collect_expr_opt(condition.expr()), // if let -- desugar to match Some(pat) => { + // FIXME(pfaria) track the break and arms parents here? mark::hit!(infer_resolve_while_let); let pat = self.collect_pat(pat); let match_expr = self.collect_expr_opt(condition.expr()); @@ -262,7 +298,7 @@ impl ExprCollector<'_> { ]; let match_expr = self.alloc_expr_desugared(Expr::Match { expr: match_expr, arms }); - return self.alloc_expr( + return (self.alloc_expr( Expr::Loop { body: match_expr, label: e @@ -271,12 +307,12 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ); + ), vec![match_expr]); } }, }; - self.alloc_expr( + (self.alloc_expr( Expr::While { condition, body, @@ -286,13 +322,13 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ) + ), vec![body, condition]) } ast::Expr::ForExpr(e) => { let iterable = self.collect_expr_opt(e.iterable()); let pat = self.collect_pat_opt(e.pat()); let body = self.collect_block_opt(e.loop_body()); - self.alloc_expr( + (self.alloc_expr( Expr::For { iterable, pat, @@ -303,7 +339,7 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ) + ), vec![iterable, body]) } ast::Expr::CallExpr(e) => { let callee = self.collect_expr_opt(e.expr()); @@ -312,41 +348,56 @@ impl ExprCollector<'_> { } else { Vec::new() }; - self.alloc_expr(Expr::Call { callee, args }, syntax_ptr) + let mut children_exprs = args.clone(); + children_exprs.push(callee); + (self.alloc_expr(Expr::Call { callee, args }, syntax_ptr), children_exprs) } ast::Expr::MethodCallExpr(e) => { let receiver = self.collect_expr_opt(e.expr()); let args = if let Some(arg_list) = e.arg_list() { arg_list.args().map(|e| self.collect_expr(e)).collect() } else { - Vec::new() + vec![] }; let method_name = e.name_ref().map(|nr| nr.as_name()).unwrap_or_else(Name::missing); let generic_args = e.type_arg_list().and_then(|it| GenericArgs::from_ast(&self.ctx(), it)); - self.alloc_expr( - Expr::MethodCall { receiver, method_name, args, generic_args }, - syntax_ptr, + let mut children_exprs = args.clone(); + children_exprs.push(receiver); + ( + self.alloc_expr( + Expr::MethodCall { receiver, method_name, args, generic_args }, + syntax_ptr, + ), + children_exprs, ) } ast::Expr::MatchExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - let arms = if let Some(match_arm_list) = e.match_arm_list() { - match_arm_list - .arms() - .map(|arm| MatchArm { - pat: self.collect_pat_opt(arm.pat()), - expr: self.collect_expr_opt(arm.expr()), - guard: arm - .guard() - .and_then(|guard| guard.expr()) - .map(|e| self.collect_expr(e)), - }) - .collect() - } else { - Vec::new() - }; - self.alloc_expr(Expr::Match { expr, arms }, syntax_ptr) + let (arms, mut children_exprs): (Vec<_>, Vec<_>) = + if let Some(match_arm_list) = e.match_arm_list() { + match_arm_list + .arms() + .map(|arm| { + let expr = self.collect_expr_opt(arm.expr()); + ( + MatchArm { + pat: self.collect_pat_opt(arm.pat()), + expr, + guard: arm + .guard() + .and_then(|guard| guard.expr()) + .map(|e| self.collect_expr(e)), + }, + expr, + ) + }) + .unzip() + } else { + (vec![], vec![]) + }; + children_exprs.push(expr); + (self.alloc_expr(Expr::Match { expr, arms }, syntax_ptr), children_exprs) } ast::Expr::PathExpr(e) => { let path = e @@ -354,35 +405,35 @@ impl ExprCollector<'_> { .and_then(|path| self.expander.parse_path(path)) .map(Expr::Path) .unwrap_or(Expr::Missing); - self.alloc_expr(path, syntax_ptr) + (self.alloc_expr(path, syntax_ptr), vec![]) } - ast::Expr::ContinueExpr(e) => self.alloc_expr( + ast::Expr::ContinueExpr(e) => (self.alloc_expr( Expr::Continue { label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, syntax_ptr, - ), + ), vec![]), ast::Expr::BreakExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - self.alloc_expr( + (self.alloc_expr( Expr::Break { expr, label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, syntax_ptr, - ) + ), expr.into_iter().collect()) } ast::Expr::ParenExpr(e) => { let inner = self.collect_expr_opt(e.expr()); // make the paren expr point to the inner expression as well let src = self.expander.to_source(syntax_ptr); self.source_map.expr_map.insert(src, inner); - inner + (inner, vec![]) } ast::Expr::ReturnExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - self.alloc_expr(Expr::Return { expr }, syntax_ptr) + (self.alloc_expr(Expr::Return { expr }, syntax_ptr), expr.into_iter().collect()) } ast::Expr::RecordLit(e) => { let path = e.path().and_then(|path| self.expander.parse_path(path)); let mut field_ptrs = Vec::new(); - let record_lit = if let Some(nfl) = e.record_field_list() { - let fields = nfl + let (record_lit, children) = if let Some(nfl) = e.record_field_list() { + let (fields, children): (Vec<_>, Vec<_>) = nfl .fields() .inspect(|field| field_ptrs.push(AstPtr::new(field))) .filter_map(|field| { @@ -391,19 +442,20 @@ impl ExprCollector<'_> { } let name = field.field_name()?.as_name(); - Some(RecordLitField { - name, - expr: match field.expr() { - Some(e) => self.collect_expr(e), - None => self.missing_expr(), - }, - }) + let expr = match field.expr() { + Some(e) => self.collect_expr(e), + None => self.missing_expr(), + }; + Some((RecordLitField { name, expr }, expr)) }) - .collect(); + .unzip(); let spread = nfl.spread().map(|s| self.collect_expr(s)); - Expr::RecordLit { path, fields, spread } + ( + Expr::RecordLit { path, fields, spread: spread }, + children.into_iter().chain(spread.into_iter()).collect(), + ) } else { - Expr::RecordLit { path, fields: Vec::new(), spread: None } + (Expr::RecordLit { path, fields: Vec::new(), spread: None }, vec![]) }; let res = self.alloc_expr(record_lit, syntax_ptr); @@ -411,7 +463,7 @@ impl ExprCollector<'_> { let src = self.expander.to_source(ptr); self.source_map.field_map.insert((res, i), src); } - res + (res, children) } ast::Expr::FieldExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -419,20 +471,20 @@ impl ExprCollector<'_> { Some(kind) => kind.as_name(), _ => Name::missing(), }; - self.alloc_expr(Expr::Field { expr, name }, syntax_ptr) + (self.alloc_expr(Expr::Field { expr, name }, syntax_ptr), vec![expr]) } ast::Expr::AwaitExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - self.alloc_expr(Expr::Await { expr }, syntax_ptr) + (self.alloc_expr(Expr::Await { expr }, syntax_ptr), vec![expr]) } ast::Expr::TryExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - self.alloc_expr(Expr::Try { expr }, syntax_ptr) + (self.alloc_expr(Expr::Try { expr }, syntax_ptr), vec![expr]) } ast::Expr::CastExpr(e) => { let expr = self.collect_expr_opt(e.expr()); let type_ref = TypeRef::from_ast_opt(&self.ctx(), e.type_ref()); - self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr) + (self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr), vec![expr]) } ast::Expr::RefExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -455,9 +507,9 @@ impl ExprCollector<'_> { ast::Expr::PrefixExpr(e) => { let expr = self.collect_expr_opt(e.expr()); if let Some(op) = e.op_kind() { - self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr) + (self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr), vec![expr]) } else { - self.alloc_expr(Expr::Missing, syntax_ptr) + (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]) } } ast::Expr::LambdaExpr(e) => { @@ -477,21 +529,24 @@ impl ExprCollector<'_> { .and_then(|r| r.type_ref()) .map(|it| TypeRef::from_ast(&self.ctx(), it)); let body = self.collect_expr_opt(e.body()); - self.alloc_expr(Expr::Lambda { args, arg_types, ret_type, body }, syntax_ptr) + ( + self.alloc_expr(Expr::Lambda { args, arg_types, ret_type, body }, syntax_ptr), + vec![body], + ) } ast::Expr::BinExpr(e) => { let lhs = self.collect_expr_opt(e.lhs()); let rhs = self.collect_expr_opt(e.rhs()); let op = e.op_kind().map(BinaryOp::from); - self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr) + (self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr), vec![lhs, rhs]) } ast::Expr::TupleExpr(e) => { - let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect(); - self.alloc_expr(Expr::Tuple { exprs }, syntax_ptr) + let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect::>(); + (self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr), exprs) } ast::Expr::BoxExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - self.alloc_expr(Expr::Box { expr }, syntax_ptr) + (self.alloc_expr(Expr::Box { expr }, syntax_ptr), vec![expr]) } ast::Expr::ArrayExpr(e) => { @@ -499,34 +554,46 @@ impl ExprCollector<'_> { match kind { ArrayExprKind::ElementList(e) => { - let exprs = e.map(|expr| self.collect_expr(expr)).collect(); - self.alloc_expr(Expr::Array(Array::ElementList(exprs)), syntax_ptr) + let exprs = e.map(|expr| self.collect_expr(expr)).collect::>(); + ( + self.alloc_expr( + Expr::Array(Array::ElementList(exprs.clone())), + syntax_ptr, + ), + exprs, + ) } ArrayExprKind::Repeat { initializer, repeat } => { let initializer = self.collect_expr_opt(initializer); let repeat = self.collect_expr_opt(repeat); - self.alloc_expr( - Expr::Array(Array::Repeat { initializer, repeat }), - syntax_ptr, + ( + self.alloc_expr( + Expr::Array(Array::Repeat { initializer, repeat }), + syntax_ptr, + ), + vec![initializer, repeat], ) } } } - ast::Expr::Literal(e) => self.alloc_expr(Expr::Literal(e.kind().into()), syntax_ptr), + ast::Expr::Literal(e) => { + (self.alloc_expr(Expr::Literal(e.kind().into()), syntax_ptr), vec![]) + } ast::Expr::IndexExpr(e) => { let base = self.collect_expr_opt(e.base()); let index = self.collect_expr_opt(e.index()); - self.alloc_expr(Expr::Index { base, index }, syntax_ptr) + (self.alloc_expr(Expr::Index { base, index }, syntax_ptr), vec![base, index]) } ast::Expr::RangeExpr(e) => { let lhs = e.start().map(|lhs| self.collect_expr(lhs)); let rhs = e.end().map(|rhs| self.collect_expr(rhs)); match e.op_kind() { - Some(range_type) => { - self.alloc_expr(Expr::Range { lhs, rhs, range_type }, syntax_ptr) - } - None => self.alloc_expr(Expr::Missing, syntax_ptr), + Some(range_type) => ( + self.alloc_expr(Expr::Range { lhs, rhs, range_type }, syntax_ptr), + lhs.into_iter().chain(rhs.into_iter()).collect(), + ), + None => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), } } ast::Expr::MacroCall(e) => { @@ -540,7 +607,7 @@ impl ExprCollector<'_> { self.body.item_scope.define_legacy_macro(name, mac); // FIXME: do we still need to allocate this as missing ? - self.alloc_expr(Expr::Missing, syntax_ptr) + (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]) } else { let macro_call = self.expander.to_source(AstPtr::new(&e)); match self.expander.enter_expand(self.db, Some(&self.body.item_scope), e) { @@ -553,15 +620,15 @@ impl ExprCollector<'_> { self.item_trees.insert(self.expander.current_file_id, item_tree); let id = self.collect_expr(expansion); self.expander.exit(self.db, mark); - id + (id, vec![]) } - None => self.alloc_expr(Expr::Missing, syntax_ptr), + None => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), } } } // FIXME implement HIR for these: - ast::Expr::Label(_e) => self.alloc_expr(Expr::Missing, syntax_ptr), + ast::Expr::Label(_e) => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), } } @@ -600,9 +667,14 @@ impl ExprCollector<'_> { } fn collect_block(&mut self, block: ast::BlockExpr) -> ExprId { + let parent_and_children = self.collect_block_inner(block); + self.update_parent_map(parent_and_children) + } + + fn collect_block_inner(&mut self, block: ast::BlockExpr) -> (ExprId, Vec) { let syntax_node_ptr = AstPtr::new(&block.clone().into()); self.collect_block_items(&block); - let statements = block + let (statements, children_exprs): (Vec<_>, Vec<_>) = block .statements() .map(|s| match s { ast::Stmt::LetStmt(stmt) => { @@ -610,14 +682,18 @@ impl ExprCollector<'_> { let type_ref = stmt.ascribed_type().map(|it| TypeRef::from_ast(&self.ctx(), it)); let initializer = stmt.initializer().map(|e| self.collect_expr(e)); - Statement::Let { pat, type_ref, initializer } + (Statement::Let { pat, type_ref, initializer }, initializer) + } + ast::Stmt::ExprStmt(stmt) => { + let expr = self.collect_expr_opt(stmt.expr()); + (Statement::Expr(expr), Some(expr)) } - ast::Stmt::ExprStmt(stmt) => Statement::Expr(self.collect_expr_opt(stmt.expr())), }) - .collect(); + .unzip(); let tail = block.expr().map(|e| self.collect_expr(e)); let label = block.label().and_then(|l| l.lifetime_token()).map(|t| Name::new_lifetime(&t)); - self.alloc_expr(Expr::Block { statements, tail, label }, syntax_node_ptr) + let children_exprs = children_exprs.into_iter().flatten().chain(tail.into_iter()).collect(); + (self.alloc_expr(Expr::Block { statements, tail, label }, syntax_node_ptr), children_exprs) } fn collect_block_items(&mut self, block: &ast::BlockExpr) { diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 5f332aadbb..3942aada56 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 26b3aeb50e..496cb428be 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] From 6c1682396c9894da07af74b43c2443e9bde89be4 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 27 May 2020 22:37:23 -0400 Subject: [PATCH 11/21] Account for deref token in syntax highlighting of unsafe, add test for that case --- crates/ra_ide/src/syntax_highlighting/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs index e9a6fbcdfe..facdc42b7a 100644 --- a/crates/ra_ide/src/syntax_highlighting/tests.rs +++ b/crates/ra_ide/src/syntax_highlighting/tests.rs @@ -389,6 +389,7 @@ fn main() { unsafe_fn(); HasUnsafeFn.unsafe_method(); let y = *x; + let z = -x; } } "# From f678e0d837e472dc2f1421f89f794d33f3ade55c Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 28 May 2020 09:30:19 -0400 Subject: [PATCH 12/21] Add HighlightTag::Operator, use it for unsafe deref. Move unsafe validation to its own file --- crates/ra_hir/src/code_model.rs | 8 ++- 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 +++++++++++++++++++ .../src/snapshots/highlight_injection.html | 2 +- .../src/snapshots/highlight_strings.html | 2 +- .../src/snapshots/highlight_unsafe.html | 4 +- crates/ra_ide/src/snapshots/highlighting.html | 2 +- .../src/snapshots/rainbow_highlighting.html | 2 +- crates/ra_ide/src/syntax_highlighting/html.rs | 2 +- 11 files changed, 81 insertions(+), 58 deletions(-) create mode 100644 crates/ra_hir_ty/src/unsafe_validation.rs diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 13e763e520..9e3b4a2d81 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -25,9 +25,11 @@ use hir_expand::{ use hir_ty::{ autoderef, display::{HirDisplayError, HirFormatter}, - expr::{ExprValidator, UnsafeValidator}, - method_resolution, ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs, - TraitEnvironment, Ty, TyDefId, TypeCtor, + expr::ExprValidator, + method_resolution, + method_resolution, ApplicationTy, Canonical, InEnvironment, Substs, TraitEnvironment, Ty, + TyDefId, TypeCtor, + unsafe_validation::UnsafeValidator, }; use ra_db::{CrateId, CrateName, Edition, FileId}; use ra_prof::profile; diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 3942aada56..99eed949fd 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 f222323249..4141581393 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 9ccf2aa377..9c2c6959dd 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 0000000000..55dbe23fa4 --- /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 }) + } + } + } + } +} diff --git a/crates/ra_ide/src/snapshots/highlight_injection.html b/crates/ra_ide/src/snapshots/highlight_injection.html index 1b0349bae2..3ac6c4ae57 100644 --- a/crates/ra_ide/src/snapshots/highlight_injection.html +++ b/crates/ra_ide/src/snapshots/highlight_injection.html @@ -12,7 +12,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } diff --git a/crates/ra_ide/src/snapshots/highlight_strings.html b/crates/ra_ide/src/snapshots/highlight_strings.html index d184b56910..8556eb8504 100644 --- a/crates/ra_ide/src/snapshots/highlight_strings.html +++ b/crates/ra_ide/src/snapshots/highlight_strings.html @@ -12,7 +12,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } diff --git a/crates/ra_ide/src/snapshots/highlight_unsafe.html b/crates/ra_ide/src/snapshots/highlight_unsafe.html index 6936e949fe..692307280d 100644 --- a/crates/ra_ide/src/snapshots/highlight_unsafe.html +++ b/crates/ra_ide/src/snapshots/highlight_unsafe.html @@ -12,7 +12,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } @@ -47,7 +47,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd unsafe { unsafe_fn(); HasUnsafeFn.unsafe_method(); - let y = *(x); + let y = *x; let z = -x; } } \ No newline at end of file diff --git a/crates/ra_ide/src/snapshots/highlighting.html b/crates/ra_ide/src/snapshots/highlighting.html index 8d0b38f958..47403b367e 100644 --- a/crates/ra_ide/src/snapshots/highlighting.html +++ b/crates/ra_ide/src/snapshots/highlighting.html @@ -12,7 +12,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } diff --git a/crates/ra_ide/src/snapshots/rainbow_highlighting.html b/crates/ra_ide/src/snapshots/rainbow_highlighting.html index 9516c74410..f5fb96f552 100644 --- a/crates/ra_ide/src/snapshots/rainbow_highlighting.html +++ b/crates/ra_ide/src/snapshots/rainbow_highlighting.html @@ -12,7 +12,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } diff --git a/crates/ra_ide/src/syntax_highlighting/html.rs b/crates/ra_ide/src/syntax_highlighting/html.rs index 0c74f73701..ed007c382f 100644 --- a/crates/ra_ide/src/syntax_highlighting/html.rs +++ b/crates/ra_ide/src/syntax_highlighting/html.rs @@ -71,7 +71,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #BC8383; } +.function.unsafe { color: #E28C14; } .operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } From 2608a6fd3a024206d4776cc391e46ef28c018434 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Fri, 29 May 2020 08:55:47 -0400 Subject: [PATCH 13/21] 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_def/src/body.rs | 2 +- crates/ra_hir_def/src/body/lower.rs | 237 ++++++++++++++-------- crates/ra_hir_ty/src/expr.rs | 71 +------ crates/ra_hir_ty/src/unsafe_validation.rs | 75 ++++++- 4 files changed, 232 insertions(+), 153 deletions(-) diff --git a/crates/ra_hir_def/src/body.rs b/crates/ra_hir_def/src/body.rs index 076d1a4fa9..14a1f4773c 100644 --- a/crates/ra_hir_def/src/body.rs +++ b/crates/ra_hir_def/src/body.rs @@ -184,7 +184,7 @@ pub struct Body { /// The `ExprId` of the actual body expression. pub body_expr: ExprId, pub item_scope: ItemScope, - pub parent_map: FxHashMap, + pub parent_map: ArenaMap, } pub type ExprPtr = AstPtr; diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index a1678adeb8..114b38710d 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -7,7 +7,7 @@ use hir_expand::{ name::{name, AsName, Name}, HirFileId, MacroDefId, MacroDefKind, }; -use ra_arena::Arena; +use ra_arena::{map::ArenaMap, Arena}; use ra_syntax::{ ast::{ self, ArgListOwner, ArrayExprKind, LiteralKind, LoopBodyOwner, ModuleItemOwner, NameOwner, @@ -15,7 +15,6 @@ use ra_syntax::{ }, AstNode, AstPtr, }; -use rustc_hash::FxHashMap; use test_utils::mark; use crate::{ @@ -75,7 +74,7 @@ pub(super) fn lower( params: Vec::new(), body_expr: dummy_expr_id(), item_scope: Default::default(), - parent_map: FxHashMap::default(), + parent_map: ArenaMap::default(), }, item_trees: { let mut map = FxHashMap::default(); @@ -97,6 +96,40 @@ struct ExprCollector<'a> { item_trees: FxHashMap>, } +macro_rules! track_parent { + (@build $collector:ident, $parent:expr $(,)?) => { + $parent + }; + (@build $collector:ident, $parent:expr, opt $expr:ident $($rest:tt)*) => { + { + if let Some(expr) = $expr { + $collector.body.parent_map.insert(expr, $parent); + } + track_parent!(@build $collector, $parent $($rest)*) + } + }; + (@build $collector:ident, $parent:expr, vec $expr:ident $($rest:tt)*) => { + { + for expr in $expr { + $collector.body.parent_map.insert(expr, $parent); + } + track_parent!(@build $collector, $parent $($rest)*) + } + }; + (@build $collector:ident, $parent:expr, $expr:ident $($rest:tt)*) => { + { + $collector.body.parent_map.insert($expr, $parent); + track_parent!(@build $collector, $parent $($rest)*) + } + }; + ($collector:ident, $parent:expr, $($rest:tt)*) => { + { + let parent = $parent; + track_parent!(@build $collector, parent, $($rest)*) + } + } +} + impl ExprCollector<'_> { fn collect( mut self, @@ -185,14 +218,42 @@ impl ExprCollector<'_> { } fn collect_expr(&mut self, expr: ast::Expr) -> ExprId { - let parent_and_children = self.collect_expr_inner(expr); - self.update_parent_map(parent_and_children) + let expr_id = self.collect_expr_inner(expr); + + debug_assert!({ + let mut found_count = 0; + let mut incr = || { + found_count += 1; + true + }; + let mut all_children_found = true; + self.body[expr_id].walk_child_exprs(|child| { + all_children_found = all_children_found + && self + .body + .parent_map + .get(child) + .map(|parent| *parent == expr_id) + .unwrap_or(false) + && incr() + }); + + if all_children_found { + let child_count_in_map = + self.body.parent_map.iter().filter(|&(_, parent)| *parent == expr_id).count(); + found_count == child_count_in_map + } else { + false + } + }); + + expr_id } - fn collect_expr_inner(&mut self, expr: ast::Expr) -> (ExprId, Vec) { + fn collect_expr_inner(&mut self, expr: ast::Expr) -> ExprId { let syntax_ptr = AstPtr::new(&expr); if !self.expander.is_cfg_enabled(&expr) { - return (self.missing_expr(), vec![]); + return self.missing_expr(); } match expr { @@ -224,48 +285,40 @@ impl ExprCollector<'_> { guard: None, }, ]; - let children_exprs = if let Some(else_branch) = else_branch { - vec![match_expr, then_branch, else_branch] - } else { - vec![match_expr, then_branch] - }; - return ( + let arm_exprs = arms.iter().map(|arm| arm.expr).collect::>(); + return track_parent!( + self, self.alloc_expr(Expr::Match { expr: match_expr, arms }, syntax_ptr), - children_exprs, + match_expr, vec arm_exprs ); } }, }; - let children_exprs = if let Some(else_branch) = else_branch { - vec![then_branch, else_branch, condition] - } else { - vec![then_branch, condition] - }; - - ( + track_parent!( + self, self.alloc_expr(Expr::If { condition, then_branch, else_branch }, syntax_ptr), - children_exprs, + then_branch, opt else_branch, condition ) } ast::Expr::EffectExpr(e) => match e.effect() { ast::Effect::Try(_) => { let body = self.collect_block_opt(e.block_expr()); - (self.alloc_expr(Expr::TryBlock { body }, syntax_ptr), vec![body]) + track_parent!(self, self.alloc_expr(Expr::TryBlock { body }, syntax_ptr), body) } ast::Effect::Unsafe(_) => { let body = self.collect_block_opt(e.block_expr()); - (self.alloc_expr(Expr::Unsafe { body }, syntax_ptr), vec![body]) + track_parent!(self, self.alloc_expr(Expr::Unsafe { body }, syntax_ptr), body) } // FIXME: we need to record these effects somewhere... ast::Effect::Async(_) | ast::Effect::Label(_) => { - (self.collect_block_opt(e.block_expr()), vec![]) + self.collect_block_opt(e.block_expr()) } }, - ast::Expr::BlockExpr(e) => (self.collect_block(e), vec![]), + ast::Expr::BlockExpr(e) => self.collect_block(e), ast::Expr::LoopExpr(e) => { let body = self.collect_block_opt(e.loop_body()); - (self.alloc_expr( + track_parent!(self, self.alloc_expr(Expr::Loop { body }, syntax_ptr), vec![body]) Expr::Loop { body, label: e @@ -274,7 +327,7 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ), vec![body]) + ), body) } ast::Expr::WhileExpr(e) => { let body = self.collect_block_opt(e.loop_body()); @@ -285,7 +338,6 @@ impl ExprCollector<'_> { None => self.collect_expr_opt(condition.expr()), // if let -- desugar to match Some(pat) => { - // FIXME(pfaria) track the break and arms parents here? mark::hit!(infer_resolve_while_let); let pat = self.collect_pat(pat); let match_expr = self.collect_expr_opt(condition.expr()); @@ -298,7 +350,7 @@ impl ExprCollector<'_> { ]; let match_expr = self.alloc_expr_desugared(Expr::Match { expr: match_expr, arms }); - return (self.alloc_expr( + return track_parent!(self, self.alloc_expr( Expr::Loop { body: match_expr, label: e @@ -307,12 +359,12 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ), vec![match_expr]); + ), match_expr); } }, }; - (self.alloc_expr( + track_parent!(self, self.alloc_expr( Expr::While { condition, body, @@ -322,13 +374,13 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ), vec![body, condition]) + ), body, condition) } ast::Expr::ForExpr(e) => { let iterable = self.collect_expr_opt(e.iterable()); let pat = self.collect_pat_opt(e.pat()); let body = self.collect_block_opt(e.loop_body()); - (self.alloc_expr( + track_parent!(self, self.alloc_expr( Expr::For { iterable, pat, @@ -339,7 +391,7 @@ impl ExprCollector<'_> { .map(|l| Name::new_lifetime(&l)), }, syntax_ptr, - ), vec![iterable, body]) + ), iterable, body) } ast::Expr::CallExpr(e) => { let callee = self.collect_expr_opt(e.expr()); @@ -348,9 +400,7 @@ impl ExprCollector<'_> { } else { Vec::new() }; - let mut children_exprs = args.clone(); - children_exprs.push(callee); - (self.alloc_expr(Expr::Call { callee, args }, syntax_ptr), children_exprs) + track_parent!(self, self.alloc_expr(Expr::Call { callee, args: args.clone() }, syntax_ptr), callee, vec args) } ast::Expr::MethodCallExpr(e) => { let receiver = self.collect_expr_opt(e.expr()); @@ -362,19 +412,24 @@ impl ExprCollector<'_> { let method_name = e.name_ref().map(|nr| nr.as_name()).unwrap_or_else(Name::missing); let generic_args = e.type_arg_list().and_then(|it| GenericArgs::from_ast(&self.ctx(), it)); - let mut children_exprs = args.clone(); - children_exprs.push(receiver); - ( + track_parent!( + self, self.alloc_expr( - Expr::MethodCall { receiver, method_name, args, generic_args }, + Expr::MethodCall { + receiver, + method_name, + args: args.clone(), + generic_args + }, syntax_ptr, ), - children_exprs, + receiver, + vec args ) } ast::Expr::MatchExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - let (arms, mut children_exprs): (Vec<_>, Vec<_>) = + let (arms, children_exprs): (Vec<_>, Vec<_>) = if let Some(match_arm_list) = e.match_arm_list() { match_arm_list .arms() @@ -396,8 +451,7 @@ impl ExprCollector<'_> { } else { (vec![], vec![]) }; - children_exprs.push(expr); - (self.alloc_expr(Expr::Match { expr, arms }, syntax_ptr), children_exprs) + track_parent!(self, self.alloc_expr(Expr::Match { expr, arms: arms.clone() }, syntax_ptr), expr, vec children_exprs) } ast::Expr::PathExpr(e) => { let path = e @@ -405,7 +459,7 @@ impl ExprCollector<'_> { .and_then(|path| self.expander.parse_path(path)) .map(Expr::Path) .unwrap_or(Expr::Missing); - (self.alloc_expr(path, syntax_ptr), vec![]) + self.alloc_expr(path, syntax_ptr) } ast::Expr::ContinueExpr(e) => (self.alloc_expr( Expr::Continue { label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, @@ -413,21 +467,21 @@ impl ExprCollector<'_> { ), vec![]), ast::Expr::BreakExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - (self.alloc_expr( + track_parent!(self, self.alloc_expr( Expr::Break { expr, label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, syntax_ptr, - ), expr.into_iter().collect()) + ), opt expr) } ast::Expr::ParenExpr(e) => { let inner = self.collect_expr_opt(e.expr()); // make the paren expr point to the inner expression as well let src = self.expander.to_source(syntax_ptr); self.source_map.expr_map.insert(src, inner); - (inner, vec![]) + inner } ast::Expr::ReturnExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - (self.alloc_expr(Expr::Return { expr }, syntax_ptr), expr.into_iter().collect()) + track_parent!(self, self.alloc_expr(Expr::Return { expr }, syntax_ptr), opt expr) } ast::Expr::RecordLit(e) => { let path = e.path().and_then(|path| self.expander.parse_path(path)); @@ -463,7 +517,7 @@ impl ExprCollector<'_> { let src = self.expander.to_source(ptr); self.source_map.field_map.insert((res, i), src); } - (res, children) + track_parent!(self, res, vec children) } ast::Expr::FieldExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -471,20 +525,24 @@ impl ExprCollector<'_> { Some(kind) => kind.as_name(), _ => Name::missing(), }; - (self.alloc_expr(Expr::Field { expr, name }, syntax_ptr), vec![expr]) + track_parent!(self, self.alloc_expr(Expr::Field { expr, name }, syntax_ptr), expr) } ast::Expr::AwaitExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - (self.alloc_expr(Expr::Await { expr }, syntax_ptr), vec![expr]) + track_parent!(self, self.alloc_expr(Expr::Await { expr }, syntax_ptr), expr) } ast::Expr::TryExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - (self.alloc_expr(Expr::Try { expr }, syntax_ptr), vec![expr]) + track_parent!(self, self.alloc_expr(Expr::Try { expr }, syntax_ptr), expr) } ast::Expr::CastExpr(e) => { let expr = self.collect_expr_opt(e.expr()); let type_ref = TypeRef::from_ast_opt(&self.ctx(), e.type_ref()); - (self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr), vec![expr]) + track_parent!( + self, + self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr), + expr + ) } ast::Expr::RefExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -501,15 +559,22 @@ impl ExprCollector<'_> { Mutability::from_mutable(e.mut_token().is_some()) }; let rawness = Rawness::from_raw(raw_tok); - - self.alloc_expr(Expr::Ref { expr, rawness, mutability }, syntax_ptr) + track_parent!( + self, + self.alloc_expr(Expr::Ref { expr, rawness, mutability }, syntax_ptr), + expr + ) } ast::Expr::PrefixExpr(e) => { let expr = self.collect_expr_opt(e.expr()); if let Some(op) = e.op_kind() { - (self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr), vec![expr]) + track_parent!( + self, + self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr), + expr + ) } else { - (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]) + self.alloc_expr(Expr::Missing, syntax_ptr) } } ast::Expr::LambdaExpr(e) => { @@ -529,24 +594,30 @@ impl ExprCollector<'_> { .and_then(|r| r.type_ref()) .map(|it| TypeRef::from_ast(&self.ctx(), it)); let body = self.collect_expr_opt(e.body()); - ( + track_parent!( + self, self.alloc_expr(Expr::Lambda { args, arg_types, ret_type, body }, syntax_ptr), - vec![body], + body, ) } ast::Expr::BinExpr(e) => { let lhs = self.collect_expr_opt(e.lhs()); let rhs = self.collect_expr_opt(e.rhs()); let op = e.op_kind().map(BinaryOp::from); - (self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr), vec![lhs, rhs]) + track_parent!( + self, + self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr), + lhs, + rhs + ) } ast::Expr::TupleExpr(e) => { let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect::>(); - (self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr), exprs) + track_parent!(self, self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr), vec exprs) } ast::Expr::BoxExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - (self.alloc_expr(Expr::Box { expr }, syntax_ptr), vec![expr]) + track_parent!(self, self.alloc_expr(Expr::Box { expr }, syntax_ptr), expr) } ast::Expr::ArrayExpr(e) => { @@ -555,45 +626,51 @@ impl ExprCollector<'_> { match kind { ArrayExprKind::ElementList(e) => { let exprs = e.map(|expr| self.collect_expr(expr)).collect::>(); - ( + track_parent!(self, self.alloc_expr( Expr::Array(Array::ElementList(exprs.clone())), syntax_ptr, ), - exprs, + vec exprs, ) } ArrayExprKind::Repeat { initializer, repeat } => { let initializer = self.collect_expr_opt(initializer); let repeat = self.collect_expr_opt(repeat); - ( + track_parent!( + self, self.alloc_expr( Expr::Array(Array::Repeat { initializer, repeat }), syntax_ptr, ), - vec![initializer, repeat], + initializer, + repeat, ) } } } - ast::Expr::Literal(e) => { - (self.alloc_expr(Expr::Literal(e.kind().into()), syntax_ptr), vec![]) - } + ast::Expr::Literal(e) => self.alloc_expr(Expr::Literal(e.kind().into()), syntax_ptr), ast::Expr::IndexExpr(e) => { let base = self.collect_expr_opt(e.base()); let index = self.collect_expr_opt(e.index()); - (self.alloc_expr(Expr::Index { base, index }, syntax_ptr), vec![base, index]) + track_parent!( + self, + self.alloc_expr(Expr::Index { base, index }, syntax_ptr), + base, + index + ) } ast::Expr::RangeExpr(e) => { let lhs = e.start().map(|lhs| self.collect_expr(lhs)); let rhs = e.end().map(|rhs| self.collect_expr(rhs)); match e.op_kind() { - Some(range_type) => ( + Some(range_type) => track_parent!( + self, self.alloc_expr(Expr::Range { lhs, rhs, range_type }, syntax_ptr), - lhs.into_iter().chain(rhs.into_iter()).collect(), + opt lhs, opt rhs ), - None => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), + None => self.alloc_expr(Expr::Missing, syntax_ptr), } } ast::Expr::MacroCall(e) => { @@ -607,7 +684,7 @@ impl ExprCollector<'_> { self.body.item_scope.define_legacy_macro(name, mac); // FIXME: do we still need to allocate this as missing ? - (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]) + self.alloc_expr(Expr::Missing, syntax_ptr) } else { let macro_call = self.expander.to_source(AstPtr::new(&e)); match self.expander.enter_expand(self.db, Some(&self.body.item_scope), e) { @@ -620,15 +697,15 @@ impl ExprCollector<'_> { self.item_trees.insert(self.expander.current_file_id, item_tree); let id = self.collect_expr(expansion); self.expander.exit(self.db, mark); - (id, vec![]) + id } - None => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), + None => self.alloc_expr(Expr::Missing, syntax_ptr), } } } // FIXME implement HIR for these: - ast::Expr::Label(_e) => (self.alloc_expr(Expr::Missing, syntax_ptr), vec![]), + ast::Expr::Label(_e) => self.alloc_expr(Expr::Missing, syntax_ptr), } } diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 99eed949fd..7db928dded 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 55dbe23fa4..430182803e 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 +} From f78df42f813504a376dca555b04ac427582f542c Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Tue, 2 Jun 2020 18:04:23 -0400 Subject: [PATCH 14/21] Fix issues caused during rebase --- crates/ra_hir_def/src/body/lower.rs | 108 ++++++++++++++++------------ 1 file changed, 63 insertions(+), 45 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 114b38710d..4240c6ad87 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -318,16 +318,20 @@ impl ExprCollector<'_> { ast::Expr::BlockExpr(e) => self.collect_block(e), ast::Expr::LoopExpr(e) => { let body = self.collect_block_opt(e.loop_body()); - track_parent!(self, self.alloc_expr(Expr::Loop { body }, syntax_ptr), vec![body]) - Expr::Loop { - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), body) + track_parent!( + self, + self.alloc_expr( + Expr::Loop { + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, + ), + body + ) } ast::Expr::WhileExpr(e) => { let body = self.collect_block_opt(e.loop_body()); @@ -350,48 +354,62 @@ impl ExprCollector<'_> { ]; let match_expr = self.alloc_expr_desugared(Expr::Match { expr: match_expr, arms }); - return track_parent!(self, self.alloc_expr( - Expr::Loop { - body: match_expr, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), match_expr); + return track_parent!( + self, + self.alloc_expr( + Expr::Loop { + body: match_expr, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, + ), + match_expr + ); } }, }; - track_parent!(self, self.alloc_expr( - Expr::While { - condition, - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), body, condition) + track_parent!( + self, + self.alloc_expr( + Expr::While { + condition, + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, + ), + body, + condition + ) } ast::Expr::ForExpr(e) => { let iterable = self.collect_expr_opt(e.iterable()); let pat = self.collect_pat_opt(e.pat()); let body = self.collect_block_opt(e.loop_body()); - track_parent!(self, self.alloc_expr( - Expr::For { - iterable, - pat, - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), iterable, body) + track_parent!( + self, + self.alloc_expr( + Expr::For { + iterable, + pat, + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, + ), + iterable, + body + ) } ast::Expr::CallExpr(e) => { let callee = self.collect_expr_opt(e.expr()); @@ -461,10 +479,10 @@ impl ExprCollector<'_> { .unwrap_or(Expr::Missing); self.alloc_expr(path, syntax_ptr) } - ast::Expr::ContinueExpr(e) => (self.alloc_expr( + ast::Expr::ContinueExpr(e) => self.alloc_expr( Expr::Continue { label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, syntax_ptr, - ), vec![]), + ), ast::Expr::BreakExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); track_parent!(self, self.alloc_expr( From 2fc92fa28cda858c25fea2e378c2eb73e7f65247 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Tue, 2 Jun 2020 18:44:04 -0400 Subject: [PATCH 15/21] Remove track_parent and parent_map, replace with simple walk in missign unsafe validator --- crates/ra_hir_def/src/body.rs | 1 - crates/ra_hir_def/src/body/lower.rs | 369 ++++++---------------- crates/ra_hir_ty/src/unsafe_validation.rs | 42 +-- 3 files changed, 121 insertions(+), 291 deletions(-) diff --git a/crates/ra_hir_def/src/body.rs b/crates/ra_hir_def/src/body.rs index 14a1f4773c..4f2350915d 100644 --- a/crates/ra_hir_def/src/body.rs +++ b/crates/ra_hir_def/src/body.rs @@ -184,7 +184,6 @@ pub struct Body { /// The `ExprId` of the actual body expression. pub body_expr: ExprId, pub item_scope: ItemScope, - pub parent_map: ArenaMap, } pub type ExprPtr = AstPtr; diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 4240c6ad87..fdd2be843c 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -7,7 +7,7 @@ use hir_expand::{ name::{name, AsName, Name}, HirFileId, MacroDefId, MacroDefKind, }; -use ra_arena::{map::ArenaMap, Arena}; +use ra_arena::Arena; use ra_syntax::{ ast::{ self, ArgListOwner, ArrayExprKind, LiteralKind, LoopBodyOwner, ModuleItemOwner, NameOwner, @@ -74,7 +74,6 @@ pub(super) fn lower( params: Vec::new(), body_expr: dummy_expr_id(), item_scope: Default::default(), - parent_map: ArenaMap::default(), }, item_trees: { let mut map = FxHashMap::default(); @@ -96,40 +95,6 @@ struct ExprCollector<'a> { item_trees: FxHashMap>, } -macro_rules! track_parent { - (@build $collector:ident, $parent:expr $(,)?) => { - $parent - }; - (@build $collector:ident, $parent:expr, opt $expr:ident $($rest:tt)*) => { - { - if let Some(expr) = $expr { - $collector.body.parent_map.insert(expr, $parent); - } - track_parent!(@build $collector, $parent $($rest)*) - } - }; - (@build $collector:ident, $parent:expr, vec $expr:ident $($rest:tt)*) => { - { - for expr in $expr { - $collector.body.parent_map.insert(expr, $parent); - } - track_parent!(@build $collector, $parent $($rest)*) - } - }; - (@build $collector:ident, $parent:expr, $expr:ident $($rest:tt)*) => { - { - $collector.body.parent_map.insert($expr, $parent); - track_parent!(@build $collector, $parent $($rest)*) - } - }; - ($collector:ident, $parent:expr, $($rest:tt)*) => { - { - let parent = $parent; - track_parent!(@build $collector, parent, $($rest)*) - } - } -} - impl ExprCollector<'_> { fn collect( mut self, @@ -206,51 +171,7 @@ impl ExprCollector<'_> { id } - fn update_parent_map( - &mut self, - (parent_expr, children_exprs): (ExprId, Vec), - ) -> ExprId { - for child_expr in children_exprs { - self.body.parent_map.insert(child_expr, parent_expr); - } - - parent_expr - } - fn collect_expr(&mut self, expr: ast::Expr) -> ExprId { - let expr_id = self.collect_expr_inner(expr); - - debug_assert!({ - let mut found_count = 0; - let mut incr = || { - found_count += 1; - true - }; - let mut all_children_found = true; - self.body[expr_id].walk_child_exprs(|child| { - all_children_found = all_children_found - && self - .body - .parent_map - .get(child) - .map(|parent| *parent == expr_id) - .unwrap_or(false) - && incr() - }); - - if all_children_found { - let child_count_in_map = - self.body.parent_map.iter().filter(|&(_, parent)| *parent == expr_id).count(); - found_count == child_count_in_map - } else { - false - } - }); - - expr_id - } - - fn collect_expr_inner(&mut self, expr: ast::Expr) -> ExprId { let syntax_ptr = AstPtr::new(&expr); if !self.expander.is_cfg_enabled(&expr) { return self.missing_expr(); @@ -285,30 +206,22 @@ impl ExprCollector<'_> { guard: None, }, ]; - let arm_exprs = arms.iter().map(|arm| arm.expr).collect::>(); - return track_parent!( - self, - self.alloc_expr(Expr::Match { expr: match_expr, arms }, syntax_ptr), - match_expr, vec arm_exprs - ); + return self + .alloc_expr(Expr::Match { expr: match_expr, arms }, syntax_ptr); } }, }; - track_parent!( - self, - self.alloc_expr(Expr::If { condition, then_branch, else_branch }, syntax_ptr), - then_branch, opt else_branch, condition - ) + self.alloc_expr(Expr::If { condition, then_branch, else_branch }, syntax_ptr) } ast::Expr::EffectExpr(e) => match e.effect() { ast::Effect::Try(_) => { let body = self.collect_block_opt(e.block_expr()); - track_parent!(self, self.alloc_expr(Expr::TryBlock { body }, syntax_ptr), body) + self.alloc_expr(Expr::TryBlock { body }, syntax_ptr) } ast::Effect::Unsafe(_) => { let body = self.collect_block_opt(e.block_expr()); - track_parent!(self, self.alloc_expr(Expr::Unsafe { body }, syntax_ptr), body) + self.alloc_expr(Expr::Unsafe { body }, syntax_ptr) } // FIXME: we need to record these effects somewhere... ast::Effect::Async(_) | ast::Effect::Label(_) => { @@ -318,19 +231,15 @@ impl ExprCollector<'_> { ast::Expr::BlockExpr(e) => self.collect_block(e), ast::Expr::LoopExpr(e) => { let body = self.collect_block_opt(e.loop_body()); - track_parent!( - self, - self.alloc_expr( - Expr::Loop { - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), - body + self.alloc_expr( + Expr::Loop { + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, ) } ast::Expr::WhileExpr(e) => { @@ -354,61 +263,47 @@ impl ExprCollector<'_> { ]; let match_expr = self.alloc_expr_desugared(Expr::Match { expr: match_expr, arms }); - return track_parent!( - self, - self.alloc_expr( - Expr::Loop { - body: match_expr, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), - match_expr + return self.alloc_expr( + Expr::Loop { + body: match_expr, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, ); } }, }; - track_parent!( - self, - self.alloc_expr( - Expr::While { - condition, - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), - body, - condition + self.alloc_expr( + Expr::While { + condition, + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, ) } ast::Expr::ForExpr(e) => { let iterable = self.collect_expr_opt(e.iterable()); let pat = self.collect_pat_opt(e.pat()); let body = self.collect_block_opt(e.loop_body()); - track_parent!( - self, - self.alloc_expr( - Expr::For { - iterable, - pat, - body, - label: e - .label() - .and_then(|l| l.lifetime_token()) - .map(|l| Name::new_lifetime(&l)), - }, - syntax_ptr, - ), - iterable, - body + self.alloc_expr( + Expr::For { + iterable, + pat, + body, + label: e + .label() + .and_then(|l| l.lifetime_token()) + .map(|l| Name::new_lifetime(&l)), + }, + syntax_ptr, ) } ast::Expr::CallExpr(e) => { @@ -418,7 +313,7 @@ impl ExprCollector<'_> { } else { Vec::new() }; - track_parent!(self, self.alloc_expr(Expr::Call { callee, args: args.clone() }, syntax_ptr), callee, vec args) + self.alloc_expr(Expr::Call { callee, args: args.clone() }, syntax_ptr) } ast::Expr::MethodCallExpr(e) => { let receiver = self.collect_expr_opt(e.expr()); @@ -430,46 +325,29 @@ impl ExprCollector<'_> { let method_name = e.name_ref().map(|nr| nr.as_name()).unwrap_or_else(Name::missing); let generic_args = e.type_arg_list().and_then(|it| GenericArgs::from_ast(&self.ctx(), it)); - track_parent!( - self, - self.alloc_expr( - Expr::MethodCall { - receiver, - method_name, - args: args.clone(), - generic_args - }, - syntax_ptr, - ), - receiver, - vec args + self.alloc_expr( + Expr::MethodCall { receiver, method_name, args: args.clone(), generic_args }, + syntax_ptr, ) } ast::Expr::MatchExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - let (arms, children_exprs): (Vec<_>, Vec<_>) = - if let Some(match_arm_list) = e.match_arm_list() { - match_arm_list - .arms() - .map(|arm| { - let expr = self.collect_expr_opt(arm.expr()); - ( - MatchArm { - pat: self.collect_pat_opt(arm.pat()), - expr, - guard: arm - .guard() - .and_then(|guard| guard.expr()) - .map(|e| self.collect_expr(e)), - }, - expr, - ) - }) - .unzip() - } else { - (vec![], vec![]) - }; - track_parent!(self, self.alloc_expr(Expr::Match { expr, arms: arms.clone() }, syntax_ptr), expr, vec children_exprs) + let arms = if let Some(match_arm_list) = e.match_arm_list() { + match_arm_list + .arms() + .map(|arm| MatchArm { + pat: self.collect_pat_opt(arm.pat()), + expr: self.collect_expr_opt(arm.expr()), + guard: arm + .guard() + .and_then(|guard| guard.expr()) + .map(|e| self.collect_expr(e)), + }) + .collect() + } else { + vec![] + }; + self.alloc_expr(Expr::Match { expr, arms }, syntax_ptr) } ast::Expr::PathExpr(e) => { let path = e @@ -485,10 +363,10 @@ impl ExprCollector<'_> { ), ast::Expr::BreakExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - track_parent!(self, self.alloc_expr( + self.alloc_expr( Expr::Break { expr, label: e.lifetime_token().map(|l| Name::new_lifetime(&l)) }, syntax_ptr, - ), opt expr) + ) } ast::Expr::ParenExpr(e) => { let inner = self.collect_expr_opt(e.expr()); @@ -499,13 +377,13 @@ impl ExprCollector<'_> { } ast::Expr::ReturnExpr(e) => { let expr = e.expr().map(|e| self.collect_expr(e)); - track_parent!(self, self.alloc_expr(Expr::Return { expr }, syntax_ptr), opt expr) + self.alloc_expr(Expr::Return { expr }, syntax_ptr) } ast::Expr::RecordLit(e) => { let path = e.path().and_then(|path| self.expander.parse_path(path)); let mut field_ptrs = Vec::new(); - let (record_lit, children) = if let Some(nfl) = e.record_field_list() { - let (fields, children): (Vec<_>, Vec<_>) = nfl + let record_lit = if let Some(nfl) = e.record_field_list() { + let fields = nfl .fields() .inspect(|field| field_ptrs.push(AstPtr::new(field))) .filter_map(|field| { @@ -518,16 +396,13 @@ impl ExprCollector<'_> { Some(e) => self.collect_expr(e), None => self.missing_expr(), }; - Some((RecordLitField { name, expr }, expr)) + Some(RecordLitField { name, expr }) }) - .unzip(); + .collect(); let spread = nfl.spread().map(|s| self.collect_expr(s)); - ( - Expr::RecordLit { path, fields, spread: spread }, - children.into_iter().chain(spread.into_iter()).collect(), - ) + Expr::RecordLit { path, fields, spread: spread } } else { - (Expr::RecordLit { path, fields: Vec::new(), spread: None }, vec![]) + Expr::RecordLit { path, fields: Vec::new(), spread: None } }; let res = self.alloc_expr(record_lit, syntax_ptr); @@ -535,7 +410,7 @@ impl ExprCollector<'_> { let src = self.expander.to_source(ptr); self.source_map.field_map.insert((res, i), src); } - track_parent!(self, res, vec children) + res } ast::Expr::FieldExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -543,24 +418,20 @@ impl ExprCollector<'_> { Some(kind) => kind.as_name(), _ => Name::missing(), }; - track_parent!(self, self.alloc_expr(Expr::Field { expr, name }, syntax_ptr), expr) + self.alloc_expr(Expr::Field { expr, name }, syntax_ptr) } ast::Expr::AwaitExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - track_parent!(self, self.alloc_expr(Expr::Await { expr }, syntax_ptr), expr) + self.alloc_expr(Expr::Await { expr }, syntax_ptr) } ast::Expr::TryExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - track_parent!(self, self.alloc_expr(Expr::Try { expr }, syntax_ptr), expr) + self.alloc_expr(Expr::Try { expr }, syntax_ptr) } ast::Expr::CastExpr(e) => { let expr = self.collect_expr_opt(e.expr()); let type_ref = TypeRef::from_ast_opt(&self.ctx(), e.type_ref()); - track_parent!( - self, - self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr), - expr - ) + self.alloc_expr(Expr::Cast { expr, type_ref }, syntax_ptr) } ast::Expr::RefExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -577,20 +448,12 @@ impl ExprCollector<'_> { Mutability::from_mutable(e.mut_token().is_some()) }; let rawness = Rawness::from_raw(raw_tok); - track_parent!( - self, - self.alloc_expr(Expr::Ref { expr, rawness, mutability }, syntax_ptr), - expr - ) + self.alloc_expr(Expr::Ref { expr, rawness, mutability }, syntax_ptr) } ast::Expr::PrefixExpr(e) => { let expr = self.collect_expr_opt(e.expr()); if let Some(op) = e.op_kind() { - track_parent!( - self, - self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr), - expr - ) + self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr) } else { self.alloc_expr(Expr::Missing, syntax_ptr) } @@ -612,30 +475,21 @@ impl ExprCollector<'_> { .and_then(|r| r.type_ref()) .map(|it| TypeRef::from_ast(&self.ctx(), it)); let body = self.collect_expr_opt(e.body()); - track_parent!( - self, - self.alloc_expr(Expr::Lambda { args, arg_types, ret_type, body }, syntax_ptr), - body, - ) + self.alloc_expr(Expr::Lambda { args, arg_types, ret_type, body }, syntax_ptr) } ast::Expr::BinExpr(e) => { let lhs = self.collect_expr_opt(e.lhs()); let rhs = self.collect_expr_opt(e.rhs()); let op = e.op_kind().map(BinaryOp::from); - track_parent!( - self, - self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr), - lhs, - rhs - ) + self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr) } ast::Expr::TupleExpr(e) => { let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect::>(); - track_parent!(self, self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr), vec exprs) + self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr) } ast::Expr::BoxExpr(e) => { let expr = self.collect_expr_opt(e.expr()); - track_parent!(self, self.alloc_expr(Expr::Box { expr }, syntax_ptr), expr) + self.alloc_expr(Expr::Box { expr }, syntax_ptr) } ast::Expr::ArrayExpr(e) => { @@ -644,25 +498,14 @@ impl ExprCollector<'_> { match kind { ArrayExprKind::ElementList(e) => { let exprs = e.map(|expr| self.collect_expr(expr)).collect::>(); - track_parent!(self, - self.alloc_expr( - Expr::Array(Array::ElementList(exprs.clone())), - syntax_ptr, - ), - vec exprs, - ) + self.alloc_expr(Expr::Array(Array::ElementList(exprs.clone())), syntax_ptr) } ArrayExprKind::Repeat { initializer, repeat } => { let initializer = self.collect_expr_opt(initializer); let repeat = self.collect_expr_opt(repeat); - track_parent!( - self, - self.alloc_expr( - Expr::Array(Array::Repeat { initializer, repeat }), - syntax_ptr, - ), - initializer, - repeat, + self.alloc_expr( + Expr::Array(Array::Repeat { initializer, repeat }), + syntax_ptr, ) } } @@ -672,22 +515,15 @@ impl ExprCollector<'_> { ast::Expr::IndexExpr(e) => { let base = self.collect_expr_opt(e.base()); let index = self.collect_expr_opt(e.index()); - track_parent!( - self, - self.alloc_expr(Expr::Index { base, index }, syntax_ptr), - base, - index - ) + self.alloc_expr(Expr::Index { base, index }, syntax_ptr) } ast::Expr::RangeExpr(e) => { let lhs = e.start().map(|lhs| self.collect_expr(lhs)); let rhs = e.end().map(|rhs| self.collect_expr(rhs)); match e.op_kind() { - Some(range_type) => track_parent!( - self, - self.alloc_expr(Expr::Range { lhs, rhs, range_type }, syntax_ptr), - opt lhs, opt rhs - ), + Some(range_type) => { + self.alloc_expr(Expr::Range { lhs, rhs, range_type }, syntax_ptr) + } None => self.alloc_expr(Expr::Missing, syntax_ptr), } } @@ -762,14 +598,9 @@ impl ExprCollector<'_> { } fn collect_block(&mut self, block: ast::BlockExpr) -> ExprId { - let parent_and_children = self.collect_block_inner(block); - self.update_parent_map(parent_and_children) - } - - fn collect_block_inner(&mut self, block: ast::BlockExpr) -> (ExprId, Vec) { let syntax_node_ptr = AstPtr::new(&block.clone().into()); self.collect_block_items(&block); - let (statements, children_exprs): (Vec<_>, Vec<_>) = block + let statements = block .statements() .map(|s| match s { ast::Stmt::LetStmt(stmt) => { @@ -777,18 +608,14 @@ impl ExprCollector<'_> { let type_ref = stmt.ascribed_type().map(|it| TypeRef::from_ast(&self.ctx(), it)); let initializer = stmt.initializer().map(|e| self.collect_expr(e)); - (Statement::Let { pat, type_ref, initializer }, initializer) - } - ast::Stmt::ExprStmt(stmt) => { - let expr = self.collect_expr_opt(stmt.expr()); - (Statement::Expr(expr), Some(expr)) + Statement::Let { pat, type_ref, initializer } } + ast::Stmt::ExprStmt(stmt) => Statement::Expr(self.collect_expr_opt(stmt.expr())), }) - .unzip(); + .collect(); let tail = block.expr().map(|e| self.collect_expr(e)); let label = block.label().and_then(|l| l.lifetime_token()).map(|t| Name::new_lifetime(&t)); - let children_exprs = children_exprs.into_iter().flatten().chain(tail.into_iter()).collect(); - (self.alloc_expr(Expr::Block { statements, tail, label }, syntax_node_ptr), children_exprs) + self.alloc_expr(Expr::Block { statements, tail, label }, syntax_node_ptr) } fn collect_block_items(&mut self, block: &ast::BlockExpr) { diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index 430182803e..f3ce7112ad 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 +} From 2ca52bbb325d4842d9642fc5f9e368b590c4ce41 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Tue, 2 Jun 2020 18:58:42 -0400 Subject: [PATCH 16/21] Revert ide highlighting changes (addressing on another branch) --- crates/ra_hir/src/code_model.rs | 4 -- .../src/snapshots/highlight_injection.html | 2 +- .../src/snapshots/highlight_strings.html | 2 +- .../src/snapshots/highlight_unsafe.html | 53 ------------------- crates/ra_ide/src/snapshots/highlighting.html | 2 +- .../src/snapshots/rainbow_highlighting.html | 2 +- crates/ra_ide/src/syntax_highlighting.rs | 8 +-- crates/ra_ide/src/syntax_highlighting/html.rs | 2 +- crates/ra_ide/src/syntax_highlighting/tags.rs | 8 +-- .../ra_ide/src/syntax_highlighting/tests.rs | 31 ----------- 10 files changed, 10 insertions(+), 104 deletions(-) delete mode 100644 crates/ra_ide/src/snapshots/highlight_unsafe.html diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 9e3b4a2d81..6aa7251b53 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -671,10 +671,6 @@ impl Function { db.function_data(self.id).params.clone() } - pub fn is_unsafe(self, db: &dyn HirDatabase) -> bool { - db.function_data(self.id).is_unsafe - } - pub fn diagnostics(self, db: &dyn HirDatabase, sink: &mut DiagnosticSink) { let _p = profile("Function::diagnostics"); let infer = db.infer(self.id.into()); diff --git a/crates/ra_ide/src/snapshots/highlight_injection.html b/crates/ra_ide/src/snapshots/highlight_injection.html index 3ac6c4ae57..748f076253 100644 --- a/crates/ra_ide/src/snapshots/highlight_injection.html +++ b/crates/ra_ide/src/snapshots/highlight_injection.html @@ -13,7 +13,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .field { color: #94BFF3; } .function { color: #93E0E3; } .function.unsafe { color: #E28C14; } -.operator.unsafe { color: #BC8383; } +.operator.unsafe { color: #E28C14; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/highlight_strings.html b/crates/ra_ide/src/snapshots/highlight_strings.html index 8556eb8504..12a68ce625 100644 --- a/crates/ra_ide/src/snapshots/highlight_strings.html +++ b/crates/ra_ide/src/snapshots/highlight_strings.html @@ -13,7 +13,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .field { color: #94BFF3; } .function { color: #93E0E3; } .function.unsafe { color: #E28C14; } -.operator.unsafe { color: #BC8383; } +.operator.unsafe { color: #E28C14; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/highlight_unsafe.html b/crates/ra_ide/src/snapshots/highlight_unsafe.html deleted file mode 100644 index 692307280d..0000000000 --- a/crates/ra_ide/src/snapshots/highlight_unsafe.html +++ /dev/null @@ -1,53 +0,0 @@ - - -
unsafe fn unsafe_fn() {}
-
-struct HasUnsafeFn;
-
-impl HasUnsafeFn {
-    unsafe fn unsafe_method(&self) {}
-}
-
-fn main() {
-    let x = &5 as *const usize;
-    unsafe {
-        unsafe_fn();
-        HasUnsafeFn.unsafe_method();
-        let y = *x;
-        let z = -x;
-    }
-}
\ No newline at end of file diff --git a/crates/ra_ide/src/snapshots/highlighting.html b/crates/ra_ide/src/snapshots/highlighting.html index 47403b367e..d093a62661 100644 --- a/crates/ra_ide/src/snapshots/highlighting.html +++ b/crates/ra_ide/src/snapshots/highlighting.html @@ -13,7 +13,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .field { color: #94BFF3; } .function { color: #93E0E3; } .function.unsafe { color: #E28C14; } -.operator.unsafe { color: #BC8383; } +.operator.unsafe { color: #E28C14; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/rainbow_highlighting.html b/crates/ra_ide/src/snapshots/rainbow_highlighting.html index f5fb96f552..192d564a6a 100644 --- a/crates/ra_ide/src/snapshots/rainbow_highlighting.html +++ b/crates/ra_ide/src/snapshots/rainbow_highlighting.html @@ -13,7 +13,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .field { color: #94BFF3; } .function { color: #93E0E3; } .function.unsafe { color: #E28C14; } -.operator.unsafe { color: #BC8383; } +.operator.unsafe { color: #E28C14; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/syntax_highlighting.rs b/crates/ra_ide/src/syntax_highlighting.rs index 028b559022..fb4f2ce388 100644 --- a/crates/ra_ide/src/syntax_highlighting.rs +++ b/crates/ra_ide/src/syntax_highlighting.rs @@ -605,13 +605,7 @@ fn highlight_name(db: &RootDatabase, def: Definition) -> Highlight { Definition::Field(_) => HighlightTag::Field, Definition::ModuleDef(def) => match def { hir::ModuleDef::Module(_) => HighlightTag::Module, - hir::ModuleDef::Function(func) => { - let mut h = HighlightTag::Function.into(); - if func.is_unsafe(db) { - h |= HighlightModifier::Unsafe; - } - return h; - } + hir::ModuleDef::Function(_) => HighlightTag::Function, hir::ModuleDef::Adt(hir::Adt::Struct(_)) => HighlightTag::Struct, hir::ModuleDef::Adt(hir::Adt::Enum(_)) => HighlightTag::Enum, hir::ModuleDef::Adt(hir::Adt::Union(_)) => HighlightTag::Union, diff --git a/crates/ra_ide/src/syntax_highlighting/html.rs b/crates/ra_ide/src/syntax_highlighting/html.rs index ed007c382f..3477610e57 100644 --- a/crates/ra_ide/src/syntax_highlighting/html.rs +++ b/crates/ra_ide/src/syntax_highlighting/html.rs @@ -72,7 +72,7 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .field { color: #94BFF3; } .function { color: #93E0E3; } .function.unsafe { color: #E28C14; } -.operator.unsafe { color: #BC8383; } +.operator.unsafe { color: #E28C14; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/syntax_highlighting/tags.rs b/crates/ra_ide/src/syntax_highlighting/tags.rs index e8e251cfc0..b9bea47294 100644 --- a/crates/ra_ide/src/syntax_highlighting/tags.rs +++ b/crates/ra_ide/src/syntax_highlighting/tags.rs @@ -25,7 +25,6 @@ pub enum HighlightTag { EnumVariant, EscapeSequence, Field, - FormatSpecifier, Function, Generic, Keyword, @@ -33,7 +32,6 @@ pub enum HighlightTag { Macro, Module, NumericLiteral, - Operator, SelfKeyword, SelfType, Static, @@ -45,6 +43,8 @@ pub enum HighlightTag { Union, Local, UnresolvedReference, + FormatSpecifier, + Operator, } #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -77,7 +77,6 @@ impl HighlightTag { HighlightTag::EnumVariant => "enum_variant", HighlightTag::EscapeSequence => "escape_sequence", HighlightTag::Field => "field", - HighlightTag::FormatSpecifier => "format_specifier", HighlightTag::Function => "function", HighlightTag::Generic => "generic", HighlightTag::Keyword => "keyword", @@ -85,7 +84,6 @@ impl HighlightTag { HighlightTag::Macro => "macro", HighlightTag::Module => "module", HighlightTag::NumericLiteral => "numeric_literal", - HighlightTag::Operator => "operator", HighlightTag::SelfKeyword => "self_keyword", HighlightTag::SelfType => "self_type", HighlightTag::Static => "static", @@ -97,6 +95,8 @@ impl HighlightTag { HighlightTag::Union => "union", HighlightTag::Local => "variable", HighlightTag::UnresolvedReference => "unresolved_reference", + HighlightTag::FormatSpecifier => "format_specifier", + HighlightTag::Operator => "operator", } } } diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs index facdc42b7a..b7fad97197 100644 --- a/crates/ra_ide/src/syntax_highlighting/tests.rs +++ b/crates/ra_ide/src/syntax_highlighting/tests.rs @@ -370,34 +370,3 @@ fn check_highlighting(ra_fixture: &str, snapshot: &str, rainbow: bool) { fs::write(dst_file, &actual_html).unwrap(); assert_eq_text!(expected_html, actual_html); } - -#[test] -fn test_unsafe_highlighting() { - let (analysis, file_id) = single_file( - r#" -unsafe fn unsafe_fn() {} - -struct HasUnsafeFn; - -impl HasUnsafeFn { - unsafe fn unsafe_method(&self) {} -} - -fn main() { - let x = &5 as *const usize; - unsafe { - unsafe_fn(); - HasUnsafeFn.unsafe_method(); - let y = *x; - let z = -x; - } -} -"# - .trim(), - ); - let dst_file = project_dir().join("crates/ra_ide/src/snapshots/highlight_unsafe.html"); - let actual_html = &analysis.highlight_as_html(file_id, false).unwrap(); - let expected_html = &read_text(&dst_file); - fs::write(dst_file, &actual_html).unwrap(); - assert_eq_text!(expected_html, actual_html); -} From 28bb8ed9cb0aa9f1efad252748ea189716355157 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Tue, 2 Jun 2020 19:09:51 -0400 Subject: [PATCH 17/21] Cleanup changes leftover from previous tracking attempt --- crates/ra_hir_def/src/body/lower.rs | 30 +++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index fdd2be843c..c6bc85e2f1 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -313,20 +313,20 @@ impl ExprCollector<'_> { } else { Vec::new() }; - self.alloc_expr(Expr::Call { callee, args: args.clone() }, syntax_ptr) + self.alloc_expr(Expr::Call { callee, args }, syntax_ptr) } ast::Expr::MethodCallExpr(e) => { let receiver = self.collect_expr_opt(e.expr()); let args = if let Some(arg_list) = e.arg_list() { arg_list.args().map(|e| self.collect_expr(e)).collect() } else { - vec![] + Vec::new() }; let method_name = e.name_ref().map(|nr| nr.as_name()).unwrap_or_else(Name::missing); let generic_args = e.type_arg_list().and_then(|it| GenericArgs::from_ast(&self.ctx(), it)); self.alloc_expr( - Expr::MethodCall { receiver, method_name, args: args.clone(), generic_args }, + Expr::MethodCall { receiver, method_name, args, generic_args }, syntax_ptr, ) } @@ -345,7 +345,7 @@ impl ExprCollector<'_> { }) .collect() } else { - vec![] + Vec::new() }; self.alloc_expr(Expr::Match { expr, arms }, syntax_ptr) } @@ -392,15 +392,17 @@ impl ExprCollector<'_> { } let name = field.field_name()?.as_name(); - let expr = match field.expr() { - Some(e) => self.collect_expr(e), - None => self.missing_expr(), - }; - Some(RecordLitField { name, expr }) + Some(RecordLitField { + name, + expr: match field.expr() { + Some(e) => self.collect_expr(e), + None => self.missing_expr(), + }, + }) }) .collect(); let spread = nfl.spread().map(|s| self.collect_expr(s)); - Expr::RecordLit { path, fields, spread: spread } + Expr::RecordLit { path, fields, spread } } else { Expr::RecordLit { path, fields: Vec::new(), spread: None } }; @@ -484,8 +486,8 @@ impl ExprCollector<'_> { self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr) } ast::Expr::TupleExpr(e) => { - let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect::>(); - self.alloc_expr(Expr::Tuple { exprs: exprs.clone() }, syntax_ptr) + let exprs = e.exprs().map(|expr| self.collect_expr(expr)).collect(); + self.alloc_expr(Expr::Tuple { exprs }, syntax_ptr) } ast::Expr::BoxExpr(e) => { let expr = self.collect_expr_opt(e.expr()); @@ -497,8 +499,8 @@ impl ExprCollector<'_> { match kind { ArrayExprKind::ElementList(e) => { - let exprs = e.map(|expr| self.collect_expr(expr)).collect::>(); - self.alloc_expr(Expr::Array(Array::ElementList(exprs.clone())), syntax_ptr) + let exprs = e.map(|expr| self.collect_expr(expr)).collect(); + self.alloc_expr(Expr::Array(Array::ElementList(exprs)), syntax_ptr) } ArrayExprKind::Repeat { initializer, repeat } => { let initializer = self.collect_expr_opt(initializer); From b1992b469cae689f7de01ea9031962735a409198 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 11:20:02 -0400 Subject: [PATCH 18/21] Remove unneeded code, filename from tests, fix rebasing issues --- Cargo.lock | 4 +- crates/ra_hir/src/code_model.rs | 8 ++- crates/ra_hir_ty/src/tests.rs | 5 -- crates/ra_hir_ty/src/unsafe_validation.rs | 17 ++---- .../src/snapshots/highlight_injection.html | 4 +- .../src/snapshots/highlight_strings.html | 4 +- .../src/snapshots/highlight_unsafe.html | 53 +++++++++++++++++++ crates/ra_ide/src/snapshots/highlighting.html | 4 +- .../src/snapshots/rainbow_highlighting.html | 4 +- crates/ra_ide/src/syntax_highlighting.rs | 8 ++- crates/ra_ide/src/syntax_highlighting/html.rs | 4 +- crates/ra_ide/src/syntax_highlighting/tags.rs | 4 +- 12 files changed, 85 insertions(+), 34 deletions(-) create mode 100644 crates/ra_ide/src/snapshots/highlight_unsafe.html diff --git a/Cargo.lock b/Cargo.lock index a70c22a949..dc758a1f09 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -30,7 +30,7 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ee49baf6cb617b853aa8d93bf420db2383fab46d314482ca2803b40d5fde979b" dependencies = [ - "winapi 0.3.8", + "winapi 0.3.9", ] [[package]] @@ -1791,7 +1791,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ca8a50ef2360fbd1eeb0ecd46795a87a19024eb4b53c5dc916ca1fd95fe62438" dependencies = [ "libc", - "winapi 0.3.8", + "winapi 0.3.9", ] [[package]] diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs index 6aa7251b53..27e94b7fe7 100644 --- a/crates/ra_hir/src/code_model.rs +++ b/crates/ra_hir/src/code_model.rs @@ -27,9 +27,9 @@ use hir_ty::{ display::{HirDisplayError, HirFormatter}, expr::ExprValidator, method_resolution, - method_resolution, ApplicationTy, Canonical, InEnvironment, Substs, TraitEnvironment, Ty, - TyDefId, TypeCtor, unsafe_validation::UnsafeValidator, + ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs, TraitEnvironment, Ty, + TyDefId, TypeCtor, }; use ra_db::{CrateId, CrateName, Edition, FileId}; use ra_prof::profile; @@ -671,6 +671,10 @@ impl Function { db.function_data(self.id).params.clone() } + pub fn is_unsafe(self, db: &dyn HirDatabase) -> bool { + db.function_data(self.id).is_unsafe + } + pub fn diagnostics(self, db: &dyn HirDatabase, sink: &mut DiagnosticSink) { let _p = profile("Function::diagnostics"); let infer = db.infer(self.id.into()); diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 496cb428be..2a85ce85d6 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 f3ce7112ad..e2353404bf 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 { diff --git a/crates/ra_ide/src/snapshots/highlight_injection.html b/crates/ra_ide/src/snapshots/highlight_injection.html index 748f076253..1b0349bae2 100644 --- a/crates/ra_ide/src/snapshots/highlight_injection.html +++ b/crates/ra_ide/src/snapshots/highlight_injection.html @@ -12,8 +12,8 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #E28C14; } -.operator.unsafe { color: #E28C14; } +.function.unsafe { color: #BC8383; } +.operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/highlight_strings.html b/crates/ra_ide/src/snapshots/highlight_strings.html index 12a68ce625..d184b56910 100644 --- a/crates/ra_ide/src/snapshots/highlight_strings.html +++ b/crates/ra_ide/src/snapshots/highlight_strings.html @@ -12,8 +12,8 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #E28C14; } -.operator.unsafe { color: #E28C14; } +.function.unsafe { color: #BC8383; } +.operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/highlight_unsafe.html b/crates/ra_ide/src/snapshots/highlight_unsafe.html new file mode 100644 index 0000000000..6936e949fe --- /dev/null +++ b/crates/ra_ide/src/snapshots/highlight_unsafe.html @@ -0,0 +1,53 @@ + + +
unsafe fn unsafe_fn() {}
+
+struct HasUnsafeFn;
+
+impl HasUnsafeFn {
+    unsafe fn unsafe_method(&self) {}
+}
+
+fn main() {
+    let x = &5 as *const usize;
+    unsafe {
+        unsafe_fn();
+        HasUnsafeFn.unsafe_method();
+        let y = *(x);
+        let z = -x;
+    }
+}
\ No newline at end of file diff --git a/crates/ra_ide/src/snapshots/highlighting.html b/crates/ra_ide/src/snapshots/highlighting.html index d093a62661..8d0b38f958 100644 --- a/crates/ra_ide/src/snapshots/highlighting.html +++ b/crates/ra_ide/src/snapshots/highlighting.html @@ -12,8 +12,8 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #E28C14; } -.operator.unsafe { color: #E28C14; } +.function.unsafe { color: #BC8383; } +.operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/snapshots/rainbow_highlighting.html b/crates/ra_ide/src/snapshots/rainbow_highlighting.html index 192d564a6a..9516c74410 100644 --- a/crates/ra_ide/src/snapshots/rainbow_highlighting.html +++ b/crates/ra_ide/src/snapshots/rainbow_highlighting.html @@ -12,8 +12,8 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #E28C14; } -.operator.unsafe { color: #E28C14; } +.function.unsafe { color: #BC8383; } +.operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/syntax_highlighting.rs b/crates/ra_ide/src/syntax_highlighting.rs index fb4f2ce388..028b559022 100644 --- a/crates/ra_ide/src/syntax_highlighting.rs +++ b/crates/ra_ide/src/syntax_highlighting.rs @@ -605,7 +605,13 @@ fn highlight_name(db: &RootDatabase, def: Definition) -> Highlight { Definition::Field(_) => HighlightTag::Field, Definition::ModuleDef(def) => match def { hir::ModuleDef::Module(_) => HighlightTag::Module, - hir::ModuleDef::Function(_) => HighlightTag::Function, + hir::ModuleDef::Function(func) => { + let mut h = HighlightTag::Function.into(); + if func.is_unsafe(db) { + h |= HighlightModifier::Unsafe; + } + return h; + } hir::ModuleDef::Adt(hir::Adt::Struct(_)) => HighlightTag::Struct, hir::ModuleDef::Adt(hir::Adt::Enum(_)) => HighlightTag::Enum, hir::ModuleDef::Adt(hir::Adt::Union(_)) => HighlightTag::Union, diff --git a/crates/ra_ide/src/syntax_highlighting/html.rs b/crates/ra_ide/src/syntax_highlighting/html.rs index 3477610e57..0c74f73701 100644 --- a/crates/ra_ide/src/syntax_highlighting/html.rs +++ b/crates/ra_ide/src/syntax_highlighting/html.rs @@ -71,8 +71,8 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd .string_literal { color: #CC9393; } .field { color: #94BFF3; } .function { color: #93E0E3; } -.function.unsafe { color: #E28C14; } -.operator.unsafe { color: #E28C14; } +.function.unsafe { color: #BC8383; } +.operator.unsafe { color: #BC8383; } .parameter { color: #94BFF3; } .text { color: #DCDCCC; } .type { color: #7CB8BB; } diff --git a/crates/ra_ide/src/syntax_highlighting/tags.rs b/crates/ra_ide/src/syntax_highlighting/tags.rs index b9bea47294..13d9dd195a 100644 --- a/crates/ra_ide/src/syntax_highlighting/tags.rs +++ b/crates/ra_ide/src/syntax_highlighting/tags.rs @@ -77,6 +77,7 @@ impl HighlightTag { HighlightTag::EnumVariant => "enum_variant", HighlightTag::EscapeSequence => "escape_sequence", HighlightTag::Field => "field", + HighlightTag::FormatSpecifier => "format_specifier", HighlightTag::Function => "function", HighlightTag::Generic => "generic", HighlightTag::Keyword => "keyword", @@ -84,6 +85,7 @@ impl HighlightTag { HighlightTag::Macro => "macro", HighlightTag::Module => "module", HighlightTag::NumericLiteral => "numeric_literal", + HighlightTag::Operator => "operator", HighlightTag::SelfKeyword => "self_keyword", HighlightTag::SelfType => "self_type", HighlightTag::Static => "static", @@ -95,8 +97,6 @@ impl HighlightTag { HighlightTag::Union => "union", HighlightTag::Local => "variable", HighlightTag::UnresolvedReference => "unresolved_reference", - HighlightTag::FormatSpecifier => "format_specifier", - HighlightTag::Operator => "operator", } } } From b7e25ba854a5ca0f1dee7082c113170876358632 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 11:55:54 -0400 Subject: [PATCH 19/21] Improve perf of finding unsafe exprs --- crates/ra_hir_ty/src/unsafe_validation.rs | 94 ++++++++++------------- 1 file changed, 42 insertions(+), 52 deletions(-) diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index e2353404bf..aad13d99cc 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)); - } - } - } - 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)); - } - } - _ => {} - } - } - - for unsafe_expr in &mut unsafe_exprs { - unsafe_expr.inside_unsafe_block = - is_in_unsafe(&body, body.body_expr, unsafe_expr.expr, false); - } + walk_unsafe(&mut unsafe_exprs, db, infer, &body, body.body_expr, false); unsafe_exprs } -fn is_in_unsafe(body: &Body, current: ExprId, needle: ExprId, within_unsafe: bool) -> bool { - if current == needle { - return within_unsafe; - } - +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]; - if let &Expr::Unsafe { body: child } = expr { - return is_in_unsafe(body, child, needle, true); + 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(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 { expr: current, inside_unsafe_block }); + } + } + _ => {} + } + + if let &Expr::Unsafe { body: child } = expr { + 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 } From 68a649d547c844cb44ee619b7f45d1193dad2b02 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 12:00:46 -0400 Subject: [PATCH 20/21] Simplify unsafe expr collection match --- crates/ra_hir_ty/src/unsafe_validation.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs index aad13d99cc..c512c4f8e9 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); }); From 9777d2cb2dcea7b5a3b289708fea21b4bf787f0f Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 12:02:49 -0400 Subject: [PATCH 21/21] Remove html from gitignore so highlight snapshots are not ignored --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index aef0fac339..472fe1a137 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,5 @@ crates/*/target *.log *.iml .vscode/settings.json -*.html generated_assists.adoc generated_features.adoc