Merge #4587

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

Addresses #190 

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

Author: bors[bot]

.gitignore

@@ -7,6 +7,5 @@ crates/*/target
 *.log
 *.iml
 .vscode/settings.json
-*.html
 generated_assists.adoc
 generated_features.adoc

Cargo.lock

@@ -26,8 +26,10 @@ use hir_ty::{
     autoderef,
     display::{HirDisplayError, HirFormatter},
     expr::ExprValidator,
-    method_resolution, ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs,
-    TraitEnvironment, Ty, TyDefId, TypeCtor,
+    method_resolution,
+    unsafe_validation::UnsafeValidator,
+    ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs, TraitEnvironment, Ty,
+    TyDefId, TypeCtor,
 };
 use ra_db::{CrateId, CrateName, Edition, FileId};
 use ra_prof::profile;
@@ -677,7 +679,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.id, infer, sink);
         validator.validate_body(db);
     }
 }

crates/ra_hir/src/code_model.rs

@@ -176,6 +176,7 @@ impl ExprCollector<'_> {
         if !self.expander.is_cfg_enabled(&expr) {
             return self.missing_expr();
         }
+
         match expr {
             ast::Expr::IfExpr(e) => {
                 let then_branch = self.collect_block_opt(e.then_branch());
@@ -218,8 +219,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::Unsafe { 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())
                 }
             },
@@ -445,7 +450,6 @@ 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)
             }
             ast::Expr::PrefixExpr(e) => {

crates/ra_hir_def/src/body/lower.rs

@@ -150,6 +150,9 @@ pub enum Expr {
     Tuple {
         exprs: Vec<ExprId>,
     },
+    Unsafe {
+        body: ExprId,
+    },
     Array(Array),
     Literal(Literal),
 }
@@ -247,7 +250,7 @@ impl Expr {
                     f(*expr);
                 }
             }
-            Expr::TryBlock { body } => f(*body),
+            Expr::TryBlock { body } | Expr::Unsafe { body } => f(*body),
             Expr::Loop { body, .. } => f(*body),
             Expr::While { condition, body, .. } => {
                 f(*condition);

crates/ra_hir_def/src/expr.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()
+    }
+}

crates/ra_hir_ty/src/diagnostics.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}, _>

crates/ra_hir_ty/src/infer/expr.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;

crates/ra_hir_ty/src/lib.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);
             }
         }

crates/ra_hir_ty/src/test_db.rs

@@ -538,6 +538,155 @@ 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 *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(

crates/ra_hir_ty/src/tests.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}