From d5f11e530dbf6edbdd0ca32d6cd5fafe634c8c4a Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Sat, 27 Jun 2020 17:11:43 -0400 Subject: Unsafe borrow of packed fields: account for borrow through ref binding, auto ref function calls --- crates/ra_hir/src/code_model.rs | 5 +- crates/ra_hir_def/src/data.rs | 6 +- crates/ra_hir_def/src/item_tree.rs | 8 +- crates/ra_hir_def/src/item_tree/lower.rs | 9 +- crates/ra_hir_ty/src/method_resolution.rs | 2 +- crates/ra_ide/src/completion/complete_dot.rs | 2 +- .../ra_ide/src/completion/complete_trait_impl.rs | 2 +- crates/ra_ide/src/completion/presentation.rs | 2 +- crates/ra_ide/src/syntax_highlighting.rs | 137 ++++++++++++++++++--- crates/ra_ide/src/syntax_highlighting/tests.rs | 31 ++--- 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 { + db.function_data(self.id).self_param } pub fn params(self, db: &dyn HirDatabase) -> Vec { 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, 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, pub is_unsafe: bool, pub params: Box<[TypeRef]>, pub is_varargs: bool, @@ -508,6 +508,12 @@ pub struct Function { pub ast_id: FileAstId, } +#[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, ) { - 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 = (|| { + 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, + db: &RootDatabase, + def: Definition, + name_ref: Option, + 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 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 = (|| { + 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 { 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 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(); } } "# -- cgit v1.2.3