Merge #4587

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

Addresses #190 

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>

.gitignore

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

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]]

crates/ra_hir/src/code_model.rs

@@ -26,8 +26,10 @@ use hir_ty::{
     autoderef,
     display::{HirDisplayError, HirFormatter},
     expr::ExprValidator,
-    method_resolution, ApplicationTy, Canonical, GenericPredicate, InEnvironment, Substs,
+    method_resolution,
-    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;
@@ -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_def/src/body/lower.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/expr.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_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()
+    }
+}

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}, _>

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;

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);
             }
         }

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

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);
+    });
+}

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)]