Merge #4587

4587: Add "missing unsafe" diagnostics r=Nashenas88 a=Nashenas88

Addresses #190 

Co-authored-by: Paul Daniel Faria <[email protected]>

7 files changed, 307 insertions, 2 deletions

diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs
index ebd9cb08f..a59efb347 100644
--- a/crates/ra_hir_ty/src/diagnostics.rs
+++ b/crates/ra_hir_ty/src/diagnostics.rs
@@ -169,3 +169,31 @@ impl AstDiagnostic for BreakOutsideOfLoop {
         ast::Expr::cast(node).unwrap()
     }
 }
+
+#[derive(Debug)]
+pub struct MissingUnsafe {
+    pub file: HirFileId,
+    pub expr: AstPtr<ast::Expr>,
+}
+
+impl Diagnostic for MissingUnsafe {
+    fn message(&self) -> String {
+        format!("This operation is unsafe and requires an unsafe function or block")
+    }
+    fn source(&self) -> InFile<SyntaxNodePtr> {
+        InFile { file_id: self.file, value: self.expr.clone().into() }
+    }
+    fn as_any(&self) -> &(dyn Any + Send + 'static) {
+        self
+    }
+}
+
+impl AstDiagnostic for MissingUnsafe {
+    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::Expr::cast(node).unwrap()
+    }
+}
diff --git a/crates/ra_hir_ty/src/infer/expr.rs b/crates/ra_hir_ty/src/infer/expr.rs
index a9565a58d..61af5f064 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::Unsafe { 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/lib.rs b/crates/ra_hir_ty/src/lib.rs
index f22232324..414158139 100644
--- a/crates/ra_hir_ty/src/lib.rs
+++ b/crates/ra_hir_ty/src/lib.rs
@@ -37,6 +37,7 @@ pub(crate) mod utils;
 pub mod db;
 pub mod diagnostics;
 pub mod expr;
+pub mod unsafe_validation;
 
 #[cfg(test)]
 mod tests;
diff --git a/crates/ra_hir_ty/src/test_db.rs b/crates/ra_hir_ty/src/test_db.rs
index ad04e3e0f..9c2c6959d 100644
--- a/crates/ra_hir_ty/src/test_db.rs
+++ b/crates/ra_hir_ty/src/test_db.rs
@@ -11,7 +11,10 @@ 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,
+    unsafe_validation::UnsafeValidator,
+};
 
 #[salsa::database(
     ra_db::SourceDatabaseExtStorage,
@@ -119,7 +122,9 @@ impl TestDB {
                 let infer = self.infer(f.into());
                 let mut sink = DiagnosticSink::new(&mut cb);
                 infer.add_diagnostics(self, f, &mut sink);
-                let mut validator = ExprValidator::new(f, infer, &mut sink);
+                let mut validator = ExprValidator::new(f, infer.clone(), &mut sink);
+                validator.validate_body(self);
+                let mut validator = UnsafeValidator::new(f, infer, &mut sink);
                 validator.validate_body(self);
             }
         }
diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs
index 85ff26a36..2a85ce85d 100644
--- a/crates/ra_hir_ty/src/tests.rs
+++ b/crates/ra_hir_ty/src/tests.rs
@@ -539,6 +539,155 @@ fn missing_record_pat_field_no_diagnostic_if_not_exhaustive() {
 }
 
 #[test]
+fn missing_unsafe_diagnostic_with_raw_ptr() {
+    let diagnostics = TestDB::with_files(
+        r"
+//- /lib.rs
+fn missing_unsafe() {
+    let x = &5 as *const usize;
+    let y = *x;
+}
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @r#""*x": This operation is unsafe and requires an unsafe function or block"#);
+}
+
+#[test]
+fn missing_unsafe_diagnostic_with_unsafe_call() {
+    let diagnostics = TestDB::with_files(
+        r"
+//- /lib.rs
+unsafe fn unsafe_fn() {
+    let x = &5 as *const usize;
+    let y = *x;
+}
+
+fn missing_unsafe() {
+    unsafe_fn();
+}
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @r#""unsafe_fn()": This operation is unsafe and requires an unsafe function or block"#);
+}
+
+#[test]
+fn missing_unsafe_diagnostic_with_unsafe_method_call() {
+    let diagnostics = TestDB::with_files(
+        r"
+struct HasUnsafe;
+
+impl HasUnsafe {
+    unsafe fn unsafe_fn(&self) {
+        let x = &5 as *const usize;
+        let y = *x;
+    }
+}
+
+fn missing_unsafe() {
+    HasUnsafe.unsafe_fn();
+}
+
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @r#""HasUnsafe.unsafe_fn()": This operation is unsafe and requires an unsafe function or block"#);
+}
+
+#[test]
+fn no_missing_unsafe_diagnostic_with_raw_ptr_in_unsafe_block() {
+    let diagnostics = TestDB::with_files(
+        r"
+fn nothing_to_see_move_along() {
+    let x = &5 as *const usize;
+    unsafe {
+        let y = *x;
+    }
+}
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @"");
+}
+
+#[test]
+fn missing_unsafe_diagnostic_with_raw_ptr_outside_unsafe_block() {
+    let diagnostics = TestDB::with_files(
+        r"
+fn nothing_to_see_move_along() {
+    let x = &5 as *const usize;
+    unsafe {
+        let y = *x;
+    }
+    let z = *x;
+}
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @r#""*x": This operation is unsafe and requires an unsafe function or block"#);
+}
+
+#[test]
+fn no_missing_unsafe_diagnostic_with_unsafe_call_in_unsafe_block() {
+    let diagnostics = TestDB::with_files(
+        r"
+unsafe fn unsafe_fn() {
+    let x = &5 as *const 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"
+struct HasUnsafe;
+
+impl HasUnsafe {
+    unsafe fn unsafe_fn() {
+        let x = &5 as *const usize;
+        let y = *x;
+    }
+}
+
+fn nothing_to_see_move_along() {
+    unsafe {
+        HasUnsafe.unsafe_fn();
+    }
+}
+
+",
+    )
+    .diagnostics()
+    .0;
+
+    assert_snapshot!(diagnostics, @"");
+}
+
+#[test]
 fn break_outside_of_loop() {
     let diagnostics = TestDB::with_files(
         r"
diff --git a/crates/ra_hir_ty/src/tests/simple.rs b/crates/ra_hir_ty/src/tests/simple.rs
index 7d8197f8b..cd919466f 100644
--- a/crates/ra_hir_ty/src/tests/simple.rs
+++ b/crates/ra_hir_ty/src/tests/simple.rs
@@ -1880,6 +1880,7 @@ fn main() {
         @r###"
     10..130 '{     ...2 }; }': ()
     20..21 'x': i32
+    24..37 'unsafe { 92 }': i32
     31..37 '{ 92 }': i32
     33..35 '92': i32
     47..48 'y': {unknown}
diff --git a/crates/ra_hir_ty/src/unsafe_validation.rs b/crates/ra_hir_ty/src/unsafe_validation.rs
new file mode 100644
index 000000000..c512c4f8e
--- /dev/null
+++ b/crates/ra_hir_ty/src/unsafe_validation.rs
@@ -0,0 +1,120 @@
+//! Provides validations for unsafe code. Currently checks if unsafe functions are missing
+//! unsafe blocks.
+
+use std::sync::Arc;
+
+use hir_def::{
+    body::Body,
+    expr::{Expr, ExprId, UnaryOp},
+    DefWithBodyId, FunctionId,
+};
+use hir_expand::diagnostics::DiagnosticSink;
+
+use crate::{
+    db::HirDatabase, diagnostics::MissingUnsafe, lower::CallableDef, ApplicationTy,
+    InferenceResult, Ty, TypeCtor,
+};
+
+pub struct UnsafeValidator<'a, 'b: 'a> {
+    func: FunctionId,
+    infer: Arc<InferenceResult>,
+    sink: &'a mut DiagnosticSink<'b>,
+}
+
+impl<'a, 'b> UnsafeValidator<'a, 'b> {
+    pub fn new(
+        func: FunctionId,
+        infer: Arc<InferenceResult>,
+        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 })
+                }
+            }
+        }
+    }
+}
+
+pub struct UnsafeExpr {
+    pub expr: ExprId,
+    pub inside_unsafe_block: bool,
+}
+
+pub fn unsafe_expressions(
+    db: &dyn HirDatabase,
+    infer: &InferenceResult,
+    def: DefWithBodyId,
+) -> Vec<UnsafeExpr> {
+    let mut unsafe_exprs = vec![];
+    let body = db.body(def);
+    walk_unsafe(&mut unsafe_exprs, db, infer, &body, body.body_expr, false);
+
+    unsafe_exprs
+}
+
+fn walk_unsafe(
+    unsafe_exprs: &mut Vec<UnsafeExpr>,
+    db: &dyn HirDatabase,
+    infer: &InferenceResult,
+    body: &Body,
+    current: ExprId,
+    inside_unsafe_block: bool,
+) {
+    let expr = &body.exprs[current];
+    match expr {
+        Expr::Call { callee, .. } => {
+            let ty = &infer[*callee];
+            if let &Ty::Apply(ApplicationTy {
+                ctor: TypeCtor::FnDef(CallableDef::FunctionId(func)),
+                ..
+            }) = ty
+            {
+                if db.function_data(func).is_unsafe {
+                    unsafe_exprs.push(UnsafeExpr { expr: current, inside_unsafe_block });
+                }
+            }
+        }
+        Expr::MethodCall { .. } => {
+            if infer
+                .method_resolution(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 });
+            }
+        }
+        Expr::Unsafe { body: child } => {
+            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);
+    });
+}