4743: Add tracking of packed repr, use it to highlight unsafe refs r=matklad a=Nashenas88

Taking a reference to a misaligned field on a packed struct is an
unsafe operation. Highlight that behavior. Currently, the misaligned
part isn't tracked, so this highlight is a bit too aggressive.

Fixes #4600

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>
Co-authored-by: Paul Daniel Faria <nashenas88@users.noreply.github.com>
Co-authored-by: Paul Daniel Faria <paulf@pop-os.localdomain>
This commit is contained in:
bors[bot] 2020-08-12 13:20:18 +00:00 committed by GitHub
commit 11de7ac2fb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 284 additions and 32 deletions

View file

@ -4,6 +4,7 @@ use std::{iter, sync::Arc};
use arrayvec::ArrayVec;
use either::Either;
use hir_def::{
adt::ReprKind,
adt::StructKind,
adt::VariantData,
builtin_type::BuiltinType,
@ -431,6 +432,10 @@ impl Struct {
Type::from_def(db, self.id.lookup(db.upcast()).container.module(db.upcast()).krate, self.id)
}
pub fn repr(self, db: &dyn HirDatabase) -> Option<ReprKind> {
db.struct_data(self.id).repr.clone()
}
fn variant_data(self, db: &dyn HirDatabase) -> Arc<VariantData> {
db.struct_data(self.id).variant_data.clone()
}
@ -1253,6 +1258,19 @@ impl Type {
)
}
pub fn is_packed(&self, db: &dyn HirDatabase) -> bool {
let adt_id = match self.ty.value {
Ty::Apply(ApplicationTy { ctor: TypeCtor::Adt(adt_id), .. }) => adt_id,
_ => return false,
};
let adt = adt_id.into();
match adt {
Adt::Struct(s) => matches!(s.repr(db), Some(ReprKind::Packed)),
_ => false,
}
}
pub fn is_raw_ptr(&self) -> bool {
matches!(&self.ty.value, Ty::Apply(ApplicationTy { ctor: TypeCtor::RawPtr(..), .. }))
}

View file

@ -49,7 +49,7 @@ pub use hir_def::{
docs::Documentation,
nameres::ModuleSource,
path::{ModPath, Path, PathKind},
type_ref::Mutability,
type_ref::{Mutability, TypeRef},
};
pub use hir_expand::{
hygiene::Hygiene, name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc,

View file

@ -25,7 +25,8 @@ use crate::{
semantics::source_to_def::{ChildContainer, SourceToDefCache, SourceToDefCtx},
source_analyzer::{resolve_hir_path, resolve_hir_path_qualifier, SourceAnalyzer},
AssocItem, Callable, Crate, Field, Function, HirFileId, ImplDef, InFile, Local, MacroDef,
Module, ModuleDef, Name, Origin, Path, ScopeDef, Trait, Type, TypeAlias, TypeParam, VariantDef,
Module, ModuleDef, Name, Origin, Path, ScopeDef, Trait, Type, TypeAlias, TypeParam, TypeRef,
VariantDef,
};
use resolver::TypeNs;
@ -279,6 +280,18 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> {
pub fn assert_contains_node(&self, node: &SyntaxNode) {
self.imp.assert_contains_node(node)
}
pub fn is_unsafe_method_call(&self, method_call_expr: ast::MethodCallExpr) -> bool {
self.imp.is_unsafe_method_call(method_call_expr)
}
pub fn is_unsafe_ref_expr(&self, ref_expr: &ast::RefExpr) -> bool {
self.imp.is_unsafe_ref_expr(ref_expr)
}
pub fn is_unsafe_ident_pat(&self, ident_pat: &ast::IdentPat) -> bool {
self.imp.is_unsafe_ident_pat(ident_pat)
}
}
impl<'db> SemanticsImpl<'db> {
@ -574,6 +587,90 @@ impl<'db> SemanticsImpl<'db> {
});
InFile::new(file_id, node)
}
pub fn is_unsafe_method_call(&self, method_call_expr: ast::MethodCallExpr) -> bool {
method_call_expr
.expr()
.and_then(|expr| {
let field_expr = if let ast::Expr::FieldExpr(field_expr) = expr {
field_expr
} else {
return None;
};
let ty = self.type_of_expr(&field_expr.expr()?)?;
if !ty.is_packed(self.db) {
return None;
}
let func = self.resolve_method_call(&method_call_expr).map(Function::from)?;
let is_unsafe = func.has_self_param(self.db)
&& matches!(func.params(self.db).first(), Some(TypeRef::Reference(..)));
Some(is_unsafe)
})
.unwrap_or(false)
}
pub fn is_unsafe_ref_expr(&self, ref_expr: &ast::RefExpr) -> bool {
ref_expr
.expr()
.and_then(|expr| {
let field_expr = match expr {
ast::Expr::FieldExpr(field_expr) => field_expr,
_ => return None,
};
let expr = field_expr.expr()?;
self.type_of_expr(&expr)
})
// Binding a reference to a packed type is possibly unsafe.
.map(|ty| ty.is_packed(self.db))
.unwrap_or(false)
// FIXME This needs layout computation to be correct. It will highlight
// more than it should with the current implementation.
}
pub fn is_unsafe_ident_pat(&self, ident_pat: &ast::IdentPat) -> bool {
if !ident_pat.ref_token().is_some() {
return false;
}
ident_pat
.syntax()
.parent()
.and_then(|parent| {
// `IdentPat` can live under `RecordPat` directly under `RecordPatField` or
// `RecordPatFieldList`. `RecordPatField` also lives under `RecordPatFieldList`,
// so this tries to lookup the `IdentPat` anywhere along that structure to the
// `RecordPat` so we can get the containing type.
let record_pat = ast::RecordPatField::cast(parent.clone())
.and_then(|record_pat| record_pat.syntax().parent())
.or_else(|| Some(parent.clone()))
.and_then(|parent| {
ast::RecordPatFieldList::cast(parent)?
.syntax()
.parent()
.and_then(ast::RecordPat::cast)
});
// If this doesn't match a `RecordPat`, fallback to a `LetStmt` to see if
// this is initialized from a `FieldExpr`.
if let Some(record_pat) = record_pat {
self.type_of_pat(&ast::Pat::RecordPat(record_pat))
} else if let Some(let_stmt) = ast::LetStmt::cast(parent) {
let field_expr = match let_stmt.initializer()? {
ast::Expr::FieldExpr(field_expr) => field_expr,
_ => return None,
};
self.type_of_expr(&field_expr.expr()?)
} else {
None
}
})
// Binding a reference to a packed type is possibly unsafe.
.map(|ty| ty.is_packed(self.db))
.unwrap_or(false)
}
}
pub trait ToDef: AstNode + Clone {

View file

@ -9,11 +9,12 @@ use hir_expand::{
};
use ra_arena::{map::ArenaMap, Arena};
use ra_syntax::ast::{self, NameOwner, VisibilityOwner};
use tt::{Delimiter, DelimiterKind, Leaf, Subtree, TokenTree};
use crate::{
body::{CfgExpander, LowerCtx},
db::DefDatabase,
item_tree::{Field, Fields, ItemTree},
item_tree::{AttrOwner, Field, Fields, ItemTree, ModItem},
src::HasChildSource,
src::HasSource,
trace::Trace,
@ -29,6 +30,7 @@ use ra_cfg::CfgOptions;
pub struct StructData {
pub name: Name,
pub variant_data: Arc<VariantData>,
pub repr: Option<ReprKind>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
@ -58,26 +60,58 @@ pub struct FieldData {
pub visibility: RawVisibility,
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ReprKind {
Packed,
Other,
}
fn repr_from_value(item_tree: &ItemTree, of: AttrOwner) -> Option<ReprKind> {
item_tree.attrs(of).by_key("repr").tt_values().find_map(parse_repr_tt)
}
fn parse_repr_tt(tt: &Subtree) -> Option<ReprKind> {
match tt.delimiter {
Some(Delimiter { kind: DelimiterKind::Parenthesis, .. }) => {}
_ => return None,
}
let mut it = tt.token_trees.iter();
match it.next()? {
TokenTree::Leaf(Leaf::Ident(ident)) if ident.text == "packed" => Some(ReprKind::Packed),
_ => Some(ReprKind::Other),
}
}
impl StructData {
pub(crate) fn struct_data_query(db: &dyn DefDatabase, id: StructId) -> Arc<StructData> {
let loc = id.lookup(db);
let item_tree = db.item_tree(loc.id.file_id);
let repr = repr_from_value(&item_tree, ModItem::from(loc.id.value).into());
let cfg_options = db.crate_graph()[loc.container.module(db).krate].cfg_options.clone();
let strukt = &item_tree[loc.id.value];
let variant_data = lower_fields(&item_tree, &cfg_options, &strukt.fields);
Arc::new(StructData { name: strukt.name.clone(), variant_data: Arc::new(variant_data) })
Arc::new(StructData {
name: strukt.name.clone(),
variant_data: Arc::new(variant_data),
repr,
})
}
pub(crate) fn union_data_query(db: &dyn DefDatabase, id: UnionId) -> Arc<StructData> {
let loc = id.lookup(db);
let item_tree = db.item_tree(loc.id.file_id);
let repr = repr_from_value(&item_tree, ModItem::from(loc.id.value).into());
let cfg_options = db.crate_graph()[loc.container.module(db).krate].cfg_options.clone();
let union = &item_tree[loc.id.value];
let variant_data = lower_fields(&item_tree, &cfg_options, &union.fields);
Arc::new(StructData { name: union.name.clone(), variant_data: Arc::new(variant_data) })
Arc::new(StructData {
name: union.name.clone(),
variant_data: Arc::new(variant_data),
repr,
})
}
}

View file

@ -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(),
},
@ -566,9 +566,20 @@ fn highlight_element(
}
}
p if p.is_punct() => match p {
T![::] | T![->] | T![=>] | T![&] | T![..] | T![=] | T![@] => {
HighlightTag::Operator.into()
T![&] => {
let h = HighlightTag::Operator.into();
let is_unsafe = element
.parent()
.and_then(ast::RefExpr::cast)
.map(|ref_expr| sema.is_unsafe_ref_expr(&ref_expr))
.unwrap_or(false);
if is_unsafe {
h | HighlightModifier::Unsafe
} else {
h
}
}
T![::] | T![->] | T![=>] | T![..] | T![=] | T![@] => HighlightTag::Operator.into(),
T![!] if element.parent().and_then(ast::MacroCall::cast).is_some() => {
HighlightTag::Macro.into()
}
@ -649,6 +660,18 @@ fn highlight_element(
HighlightTag::SelfKeyword.into()
}
}
T![ref] => element
.parent()
.and_then(ast::IdentPat::cast)
.and_then(|ident_pat| {
if sema.is_unsafe_ident_pat(&ident_pat) {
Some(HighlightModifier::Unsafe)
} else {
None
}
})
.map(|modifier| h | modifier)
.unwrap_or(h),
_ => h,
}
}
@ -678,7 +701,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) => {
@ -697,6 +726,15 @@ 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 is_unsafe = name_ref
.and_then(|name_ref| name_ref.syntax().parent())
.and_then(ast::MethodCallExpr::cast)
.map(|method_call_expr| sema.is_unsafe_method_call(method_call_expr))
.unwrap_or(false);
if is_unsafe {
h |= HighlightModifier::Unsafe;
}
}
return h;
}
@ -768,8 +806,18 @@ 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 is_unsafe = ast::MethodCallExpr::cast(parent)
.map(|method_call_expr| sema.is_unsafe_method_call(method_call_expr))
.unwrap_or(false);
if is_unsafe {
h |= HighlightModifier::Unsafe;
}
h
}
FIELD_EXPR => {
let h = HighlightTag::Field;
let is_union = ast::FieldExpr::cast(parent)
@ -782,7 +830,11 @@ 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) {
@ -807,18 +859,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(),
}
}

View file

@ -292,10 +292,24 @@ struct TypeForStaticMut {
static mut global_mut: TypeForStaticMut = TypeForStaticMut { a: 0 };
#[repr(packed)]
struct Packed {
a: u16,
}
trait DoTheAutoref {
fn calls_autoref(&self);
}
impl DoTheAutoref for u16 {
fn calls_autoref(&self) {}
}
fn main() {
let x = &5 as *const usize;
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 {
@ -303,9 +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 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();
}
}
"#

View file

@ -54,10 +54,24 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd
<span class="keyword">static</span> <span class="keyword">mut</span> <span class="static declaration mutable unsafe">global_mut</span><span class="punctuation">:</span> <span class="struct">TypeForStaticMut</span> <span class="operator">=</span> <span class="struct">TypeForStaticMut</span> <span class="punctuation">{</span> <span class="field">a</span><span class="punctuation">:</span> <span class="numeric_literal">0</span> <span class="punctuation">}</span><span class="punctuation">;</span>
<span class="attribute">#</span><span class="attribute">[</span><span class="function attribute">repr</span><span class="punctuation">(</span><span class="attribute">packed</span><span class="punctuation">)</span><span class="attribute">]</span>
<span class="keyword">struct</span> <span class="struct declaration">Packed</span> <span class="punctuation">{</span>
<span class="field declaration">a</span><span class="punctuation">:</span> <span class="builtin_type">u16</span><span class="punctuation">,</span>
<span class="punctuation">}</span>
<span class="keyword">trait</span> <span class="trait declaration">DoTheAutoref</span> <span class="punctuation">{</span>
<span class="keyword">fn</span> <span class="function declaration">calls_autoref</span><span class="punctuation">(</span><span class="operator">&</span><span class="self_keyword">self</span><span class="punctuation">)</span><span class="punctuation">;</span>
<span class="punctuation">}</span>
<span class="keyword">impl</span> <span class="trait">DoTheAutoref</span> <span class="keyword">for</span> <span class="builtin_type">u16</span> <span class="punctuation">{</span>
<span class="keyword">fn</span> <span class="function declaration">calls_autoref</span><span class="punctuation">(</span><span class="operator">&</span><span class="self_keyword">self</span><span class="punctuation">)</span> <span class="punctuation">{</span><span class="punctuation">}</span>
<span class="punctuation">}</span>
<span class="keyword">fn</span> <span class="function declaration">main</span><span class="punctuation">(</span><span class="punctuation">)</span> <span class="punctuation">{</span>
<span class="keyword">let</span> <span class="variable declaration">x</span> <span class="operator">=</span> <span class="operator">&</span><span class="numeric_literal">5</span> <span class="keyword">as</span> <span class="keyword">*</span><span class="keyword">const</span> <span class="builtin_type">usize</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">x</span> <span class="operator">=</span> <span class="operator">&</span><span class="numeric_literal">5</span> <span class="keyword">as</span> <span class="keyword">*</span><span class="keyword">const</span> <span class="punctuation">_</span> <span class="keyword">as</span> <span class="keyword">*</span><span class="keyword">const</span> <span class="builtin_type">usize</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">u</span> <span class="operator">=</span> <span class="union">Union</span> <span class="punctuation">{</span> <span class="field">b</span><span class="punctuation">:</span> <span class="numeric_literal">0</span> <span class="punctuation">}</span><span class="punctuation">;</span>
<span class="keyword unsafe">unsafe</span> <span class="punctuation">{</span>
<span class="comment">// unsafe fn and method calls</span>
<span class="function unsafe">unsafe_fn</span><span class="punctuation">(</span><span class="punctuation">)</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">b</span> <span class="operator">=</span> <span class="variable">u</span><span class="punctuation">.</span><span class="field unsafe">b</span><span class="punctuation">;</span>
<span class="keyword control">match</span> <span class="variable">u</span> <span class="punctuation">{</span>
@ -65,8 +79,21 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd
<span class="union">Union</span> <span class="punctuation">{</span> <span class="field unsafe">a</span> <span class="punctuation">}</span> <span class="operator">=&gt;</span> <span class="punctuation">(</span><span class="punctuation">)</span><span class="punctuation">,</span>
<span class="punctuation">}</span>
<span class="struct">HasUnsafeFn</span><span class="punctuation">.</span><span class="function unsafe">unsafe_method</span><span class="punctuation">(</span><span class="punctuation">)</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">y</span> <span class="operator">=</span> <span class="operator unsafe">*</span><span class="punctuation">(</span><span class="variable">x</span><span class="punctuation">)</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">z</span> <span class="operator">=</span> <span class="numeric_literal">-</span><span class="variable">x</span><span class="punctuation">;</span>
<span class="comment">// unsafe deref</span>
<span class="keyword">let</span> <span class="variable declaration">y</span> <span class="operator">=</span> <span class="operator unsafe">*</span><span class="variable">x</span><span class="punctuation">;</span>
<span class="comment">// unsafe access to a static mut</span>
<span class="keyword">let</span> <span class="variable declaration">a</span> <span class="operator">=</span> <span class="static mutable unsafe">global_mut</span><span class="punctuation">.</span><span class="field">a</span><span class="punctuation">;</span>
<span class="comment">// unsafe ref of packed fields</span>
<span class="keyword">let</span> <span class="variable declaration">packed</span> <span class="operator">=</span> <span class="struct">Packed</span> <span class="punctuation">{</span> <span class="field">a</span><span class="punctuation">:</span> <span class="numeric_literal">0</span> <span class="punctuation">}</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="variable declaration">a</span> <span class="operator">=</span> <span class="operator unsafe">&</span><span class="variable">packed</span><span class="punctuation">.</span><span class="field">a</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="keyword unsafe">ref</span> <span class="variable declaration">a</span> <span class="operator">=</span> <span class="variable">packed</span><span class="punctuation">.</span><span class="field">a</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="struct">Packed</span> <span class="punctuation">{</span> <span class="keyword unsafe">ref</span> <span class="field">a</span> <span class="punctuation">}</span> <span class="operator">=</span> <span class="variable">packed</span><span class="punctuation">;</span>
<span class="keyword">let</span> <span class="struct">Packed</span> <span class="punctuation">{</span> <span class="field">a</span><span class="punctuation">:</span> <span class="keyword unsafe">ref</span> <span class="variable declaration">_a</span> <span class="punctuation">}</span> <span class="operator">=</span> <span class="variable">packed</span><span class="punctuation">;</span>
<span class="comment">// unsafe auto ref of packed field</span>
<span class="variable">packed</span><span class="punctuation">.</span><span class="field">a</span><span class="punctuation">.</span><span class="function unsafe">calls_autoref</span><span class="punctuation">(</span><span class="punctuation">)</span><span class="punctuation">;</span>
<span class="punctuation">}</span>
<span class="punctuation">}</span></code></pre>