Unsafe borrow of packed fields: account for borrow through ref binding, auto ref function calls

10 files changed, 156 insertions, 48 deletions

diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs
index 0007d7fa8..a880fa671 100644
--- a/crates/ra_hir/src/code_model.rs
+++ b/crates/ra_hir/src/code_model.rs
@@ -11,6 +11,7 @@ use hir_def::{
     docs::Documentation,
     expr::{BindingAnnotation, Pat, PatId},
     import_map,
+    item_tree::SelfParam,
     per_ns::PerNs,
     resolver::{HasResolver, Resolver},
     src::HasSource as _,
@@ -670,8 +671,8 @@ impl Function {
         db.function_data(self.id).name.clone()
     }
 
-    pub fn has_self_param(self, db: &dyn HirDatabase) -> bool {
-        db.function_data(self.id).has_self_param
+    pub fn self_param(self, db: &dyn HirDatabase) -> Option<SelfParam> {
+        db.function_data(self.id).self_param
     }
 
     pub fn params(self, db: &dyn HirDatabase) -> Vec<TypeRef> {
diff --git a/crates/ra_hir_def/src/data.rs b/crates/ra_hir_def/src/data.rs
index 88a8ef9bf..2a26b0183 100644
--- a/crates/ra_hir_def/src/data.rs
+++ b/crates/ra_hir_def/src/data.rs
@@ -10,7 +10,7 @@ use crate::{
     attr::Attrs,
     body::Expander,
     db::DefDatabase,
-    item_tree::{AssocItem, ItemTreeId, ModItem},
+    item_tree::{AssocItem, ItemTreeId, ModItem, SelfParam},
     type_ref::{TypeBound, TypeRef},
     visibility::RawVisibility,
     AssocContainerId, AssocItemId, ConstId, ConstLoc, FunctionId, FunctionLoc, HasModule, ImplId,
@@ -25,7 +25,7 @@ pub struct FunctionData {
     pub attrs: Attrs,
     /// True if the first param is `self`. This is relevant to decide whether this
     /// can be called as a method.
-    pub has_self_param: bool,
+    pub self_param: Option<SelfParam>,
     pub is_unsafe: bool,
     pub is_varargs: bool,
     pub visibility: RawVisibility,
@@ -42,7 +42,7 @@ impl FunctionData {
             params: func.params.to_vec(),
             ret_type: func.ret_type.clone(),
             attrs: item_tree.attrs(ModItem::from(loc.id.value).into()).clone(),
-            has_self_param: func.has_self_param,
+            self_param: func.self_param,
             is_unsafe: func.is_unsafe,
             is_varargs: func.is_varargs,
             visibility: item_tree[func.visibility].clone(),
diff --git a/crates/ra_hir_def/src/item_tree.rs b/crates/ra_hir_def/src/item_tree.rs
index a67e75dac..1eaea66e4 100644
--- a/crates/ra_hir_def/src/item_tree.rs
+++ b/crates/ra_hir_def/src/item_tree.rs
@@ -500,7 +500,7 @@ pub struct Function {
     pub name: Name,
     pub visibility: RawVisibilityId,
     pub generic_params: GenericParamsId,
-    pub has_self_param: bool,
+    pub self_param: Option<SelfParam>,
     pub is_unsafe: bool,
     pub params: Box<[TypeRef]>,
     pub is_varargs: bool,
@@ -508,6 +508,12 @@ pub struct Function {
     pub ast_id: FileAstId<ast::Fn>,
 }
 
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+pub struct SelfParam {
+    pub is_ref: bool,
+    pub is_mut: bool,
+}
+
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct Struct {
     pub name: Name,
diff --git a/crates/ra_hir_def/src/item_tree/lower.rs b/crates/ra_hir_def/src/item_tree/lower.rs
index 450ef8798..89ad91d37 100644
--- a/crates/ra_hir_def/src/item_tree/lower.rs
+++ b/crates/ra_hir_def/src/item_tree/lower.rs
@@ -283,7 +283,7 @@ impl Ctx {
         let name = func.name()?.as_name();
 
         let mut params = Vec::new();
-        let mut has_self_param = false;
+        let mut func_self_param = None;
         if let Some(param_list) = func.param_list() {
             if let Some(self_param) = param_list.self_param() {
                 let self_type = match self_param.ty() {
@@ -302,7 +302,10 @@ impl Ctx {
                     }
                 };
                 params.push(self_type);
-                has_self_param = true;
+                func_self_param = Some(SelfParam {
+                    is_ref: self_param.amp_token().is_some(),
+                    is_mut: self_param.mut_token().is_some(),
+                });
             }
             for param in param_list.params() {
                 let type_ref = TypeRef::from_ast_opt(&self.body_ctx, param.ty());
@@ -335,7 +338,7 @@ impl Ctx {
             name,
             visibility,
             generic_params: GenericParamsId::EMPTY,
-            has_self_param,
+            self_param: func_self_param,
             is_unsafe: func.unsafe_token().is_some(),
             params: params.into_boxed_slice(),
             is_varargs,
diff --git a/crates/ra_hir_ty/src/method_resolution.rs b/crates/ra_hir_ty/src/method_resolution.rs
index fb4b30a13..79c5adf0f 100644
--- a/crates/ra_hir_ty/src/method_resolution.rs
+++ b/crates/ra_hir_ty/src/method_resolution.rs
@@ -640,7 +640,7 @@ fn is_valid_candidate(
                 }
             }
             if let Some(receiver_ty) = receiver_ty {
-                if !data.has_self_param {
+                if data.self_param.is_none() {
                     return false;
                 }
                 let transformed_receiver_ty = match transform_receiver_ty(db, m, self_ty) {
diff --git a/crates/ra_ide/src/completion/complete_dot.rs b/crates/ra_ide/src/completion/complete_dot.rs
index 532665285..5488db43f 100644
--- a/crates/ra_ide/src/completion/complete_dot.rs
+++ b/crates/ra_ide/src/completion/complete_dot.rs
@@ -48,7 +48,7 @@ fn complete_methods(acc: &mut Completions, ctx: &CompletionContext, receiver: &T
         let mut seen_methods = FxHashSet::default();
         let traits_in_scope = ctx.scope.traits_in_scope();
         receiver.iterate_method_candidates(ctx.db, krate, &traits_in_scope, None, |_ty, func| {
-            if func.has_self_param(ctx.db)
+            if func.self_param(ctx.db).is_some()
                 && ctx.scope.module().map_or(true, |m| func.is_visible_from(ctx.db, m))
                 && seen_methods.insert(func.name(ctx.db))
             {
diff --git a/crates/ra_ide/src/completion/complete_trait_impl.rs b/crates/ra_ide/src/completion/complete_trait_impl.rs
index d9a0ef167..e3ba7ebc4 100644
--- a/crates/ra_ide/src/completion/complete_trait_impl.rs
+++ b/crates/ra_ide/src/completion/complete_trait_impl.rs
@@ -136,7 +136,7 @@ fn add_function_impl(
         .lookup_by(fn_name)
         .set_documentation(func.docs(ctx.db));
 
-    let completion_kind = if func.has_self_param(ctx.db) {
+    let completion_kind = if func.self_param(ctx.db).is_some() {
         CompletionItemKind::Method
     } else {
         CompletionItemKind::Function
diff --git a/crates/ra_ide/src/completion/presentation.rs b/crates/ra_ide/src/completion/presentation.rs
index 9a94ff476..fc3d1a4bd 100644
--- a/crates/ra_ide/src/completion/presentation.rs
+++ b/crates/ra_ide/src/completion/presentation.rs
@@ -191,7 +191,7 @@ impl Completions {
         func: hir::Function,
         local_name: Option<String>,
     ) {
-        let has_self_param = func.has_self_param(ctx.db);
+        let has_self_param = func.self_param(ctx.db).is_some();
 
         let name = local_name.unwrap_or_else(|| func.name(ctx.db).to_string());
         let ast_node = func.source(ctx.db).value;
diff --git a/crates/ra_ide/src/syntax_highlighting.rs b/crates/ra_ide/src/syntax_highlighting.rs
index a4a7aa228..454fef39c 100644
--- a/crates/ra_ide/src/syntax_highlighting.rs
+++ b/crates/ra_ide/src/syntax_highlighting.rs
@@ -497,9 +497,9 @@ fn highlight_element(
             match name_kind {
                 Some(NameClass::ExternCrate(_)) => HighlightTag::Module.into(),
                 Some(NameClass::Definition(def)) => {
-                    highlight_name(db, def, false) | HighlightModifier::Definition
+                    highlight_name(sema, db, def, None, false) | HighlightModifier::Definition
                 }
-                Some(NameClass::ConstReference(def)) => highlight_name(db, def, false),
+                Some(NameClass::ConstReference(def)) => highlight_name(sema, db, def, None, false),
                 Some(NameClass::FieldShorthand { field, .. }) => {
                     let mut h = HighlightTag::Field.into();
                     if let Definition::Field(field) = field {
@@ -532,7 +532,7 @@ fn highlight_element(
                                 binding_hash = Some(calc_binding_hash(&name, *shadow_count))
                             }
                         };
-                        highlight_name(db, def, possibly_unsafe)
+                        highlight_name(sema, db, def, Some(name_ref), possibly_unsafe)
                     }
                     NameRefClass::FieldShorthand { .. } => HighlightTag::Field.into(),
                 },
@@ -565,8 +565,8 @@ fn highlight_element(
                 _ => h,
             }
         }
-        REF_EXPR => {
-            let ref_expr = element.into_node().and_then(ast::RefExpr::cast)?;
+        T![&] => {
+            let ref_expr = element.parent().and_then(ast::RefExpr::cast)?;
             let expr = ref_expr.expr()?;
             let field_expr = match expr {
                 ast::Expr::FieldExpr(fe) => fe,
@@ -668,6 +668,52 @@ fn highlight_element(
                         HighlightTag::SelfKeyword.into()
                     }
                 }
+                T![ref] => {
+                    let modifier: Option<HighlightModifier> = (|| {
+                        let bind_pat = element.parent().and_then(ast::BindPat::cast)?;
+                        let parent = bind_pat.syntax().parent()?;
+
+                        let ty = if let Some(pat_list) =
+                            ast::RecordFieldPatList::cast(parent.clone())
+                        {
+                            let record_pat =
+                                pat_list.syntax().parent().and_then(ast::RecordPat::cast)?;
+                            sema.type_of_pat(&ast::Pat::RecordPat(record_pat))
+                        } else if let Some(let_stmt) = ast::LetStmt::cast(parent.clone()) {
+                            let field_expr =
+                                if let ast::Expr::FieldExpr(field_expr) = let_stmt.initializer()? {
+                                    field_expr
+                                } else {
+                                    return None;
+                                };
+
+                            sema.type_of_expr(&field_expr.expr()?)
+                        } else if let Some(record_field_pat) = ast::RecordFieldPat::cast(parent) {
+                            let record_pat = record_field_pat
+                                .syntax()
+                                .parent()
+                                .and_then(ast::RecordFieldPatList::cast)?
+                                .syntax()
+                                .parent()
+                                .and_then(ast::RecordPat::cast)?;
+                            sema.type_of_pat(&ast::Pat::RecordPat(record_pat))
+                        } else {
+                            None
+                        }?;
+
+                        if !ty.is_packed(db) {
+                            return None;
+                        }
+
+                        Some(HighlightModifier::Unsafe)
+                    })();
+
+                    if let Some(modifier) = modifier {
+                        h | modifier
+                    } else {
+                        h
+                    }
+                }
                 _ => h,
             }
         }
@@ -697,7 +743,13 @@ fn is_child_of_impl(element: &SyntaxElement) -> bool {
     }
 }
 
-fn highlight_name(db: &RootDatabase, def: Definition, possibly_unsafe: bool) -> Highlight {
+fn highlight_name(
+    sema: &Semantics<RootDatabase>,
+    db: &RootDatabase,
+    def: Definition,
+    name_ref: Option<ast::NameRef>,
+    possibly_unsafe: bool,
+) -> Highlight {
     match def {
         Definition::Macro(_) => HighlightTag::Macro,
         Definition::Field(field) => {
@@ -716,6 +768,29 @@ fn highlight_name(db: &RootDatabase, def: Definition, possibly_unsafe: bool) ->
                 let mut h = HighlightTag::Function.into();
                 if func.is_unsafe(db) {
                     h |= HighlightModifier::Unsafe;
+                } else {
+                    (|| {
+                        let method_call_expr =
+                            name_ref?.syntax().parent().and_then(ast::MethodCallExpr::cast)?;
+                        let expr = method_call_expr.expr()?;
+                        let field_expr = if let ast::Expr::FieldExpr(field_expr) = expr {
+                            Some(field_expr)
+                        } else {
+                            None
+                        }?;
+                        let ty = sema.type_of_expr(&field_expr.expr()?)?;
+                        if !ty.is_packed(db) {
+                            return None;
+                        }
+
+                        let func = sema.resolve_method_call(&method_call_expr)?;
+                        if func.self_param(db)?.is_ref {
+                            Some(HighlightModifier::Unsafe)
+                        } else {
+                            None
+                        }
+                    })()
+                    .map(|modifier| h |= modifier);
                 }
                 return h;
             }
@@ -787,8 +862,33 @@ fn highlight_name_ref_by_syntax(name: ast::NameRef, sema: &Semantics<RootDatabas
         _ => return default.into(),
     };
 
-    let tag = match parent.kind() {
-        METHOD_CALL_EXPR => HighlightTag::Function,
+    match parent.kind() {
+        METHOD_CALL_EXPR => {
+            let mut h = Highlight::new(HighlightTag::Function);
+            let modifier: Option<HighlightModifier> = (|| {
+                let method_call_expr = ast::MethodCallExpr::cast(parent)?;
+                let expr = method_call_expr.expr()?;
+                let field_expr = if let ast::Expr::FieldExpr(field_expr) = expr {
+                    field_expr
+                } else {
+                    return None;
+                };
+
+                let expr = field_expr.expr()?;
+                let ty = sema.type_of_expr(&expr)?;
+                if ty.is_packed(sema.db) {
+                    Some(HighlightModifier::Unsafe)
+                } else {
+                    None
+                }
+            })();
+
+            if let Some(modifier) = modifier {
+                h |= modifier;
+            }
+
+            h
+        }
         FIELD_EXPR => {
             let h = HighlightTag::Field;
             let is_union = ast::FieldExpr::cast(parent)
@@ -801,7 +901,7 @@ fn highlight_name_ref_by_syntax(name: ast::NameRef, sema: &Semantics<RootDatabas
                     })
                 })
                 .unwrap_or(false);
-            return if is_union { h | HighlightModifier::Unsafe } else { h.into() };
+            if is_union { h | HighlightModifier::Unsafe } else { h.into() }
         }
         PATH_SEGMENT => {
             let path = match parent.parent().and_then(ast::Path::cast) {
@@ -826,18 +926,15 @@ fn highlight_name_ref_by_syntax(name: ast::NameRef, sema: &Semantics<RootDatabas
             };
 
             match parent.kind() {
-                CALL_EXPR => HighlightTag::Function,
-                _ => {
-                    if name.text().chars().next().unwrap_or_default().is_uppercase() {
-                        HighlightTag::Struct
-                    } else {
-                        HighlightTag::Constant
-                    }
+                CALL_EXPR => HighlightTag::Function.into(),
+                _ => if name.text().chars().next().unwrap_or_default().is_uppercase() {
+                    HighlightTag::Struct.into()
+                } else {
+                    HighlightTag::Constant
                 }
+                .into(),
             }
         }
-        _ => default,
-    };
-
-    tag.into()
+        _ => default.into(),
+    }
 }
diff --git a/crates/ra_ide/src/syntax_highlighting/tests.rs b/crates/ra_ide/src/syntax_highlighting/tests.rs
index a7f5ad862..a8087635a 100644
--- a/crates/ra_ide/src/syntax_highlighting/tests.rs
+++ b/crates/ra_ide/src/syntax_highlighting/tests.rs
@@ -301,16 +301,7 @@ trait DoTheAutoref {
     fn calls_autoref(&self);
 }
 
-struct NeedsAlign {
-    a: u16
-}
-
-#[repr(packed)]
-struct HasAligned {
-    a: NeedsAlign
-}
-
-impl DoTheAutoref for NeedsAlign {
+impl DoTheAutoref for u16 {
     fn calls_autoref(&self) {}
 }
 
@@ -318,6 +309,7 @@ fn main() {
     let x = &5 as *const _ as *const usize;
     let u = Union { b: 0 };
     unsafe {
+        // unsafe fn and method calls
         unsafe_fn();
         let b = u.b;
         match u {
@@ -325,13 +317,22 @@ fn main() {
             Union { a } => (),
         }
         HasUnsafeFn.unsafe_method();
-        let _y = *(x);
-        let z = -x;
+
+        // unsafe deref
+        let y = *x;
+
+        // unsafe access to a static mut
         let a = global_mut.a;
+
+        // unsafe ref of packed fields
         let packed = Packed { a: 0 };
-        let _a = &packed.a;
-        let h = HasAligned{ a: NeedsAlign { a: 1 } };
-        h.a.calls_autoref();
+        let a = &packed.a;
+        let ref a = packed.a;
+        let Packed { ref a } = packed;
+        let Packed { a: ref _a } = packed;
+
+        // unsafe auto ref of packed field
+        packed.a.calls_autoref();
     }
 }
 "#