Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support RFC 2396 #19038

Merged
merged 1 commit into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/hir-def/src/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ impl FunctionData {
flags.remove(FnFlags::HAS_UNSAFE_KW);
}

if attrs.by_key(&sym::target_feature).exists() {
flags.insert(FnFlags::HAS_TARGET_FEATURE);
}

Arc::new(FunctionData {
name: func.name.clone(),
params: func
Expand Down Expand Up @@ -155,6 +159,10 @@ impl FunctionData {
pub fn is_varargs(&self) -> bool {
self.flags.contains(FnFlags::IS_VARARGS)
}

pub fn has_target_feature(&self) -> bool {
self.flags.contains(FnFlags::HAS_TARGET_FEATURE)
}
}

fn parse_rustc_legacy_const_generics(tt: &crate::tt::TopSubtree) -> Box<[u32]> {
Expand Down
7 changes: 6 additions & 1 deletion crates/hir-def/src/item_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ pub struct Param {

bitflags::bitflags! {
#[derive(Debug, Clone, Copy, Eq, PartialEq, Default)]
pub(crate) struct FnFlags: u8 {
pub(crate) struct FnFlags: u16 {
const HAS_SELF_PARAM = 1 << 0;
const HAS_BODY = 1 << 1;
const HAS_DEFAULT_KW = 1 << 2;
Expand All @@ -946,6 +946,11 @@ bitflags::bitflags! {
const HAS_UNSAFE_KW = 1 << 5;
const IS_VARARGS = 1 << 6;
const HAS_SAFE_KW = 1 << 7;
/// The `#[target_feature]` attribute is necessary to check safety (with RFC 2396),
/// but keeping it for all functions will consume a lot of memory when there are
/// only very few functions with it. So we only encode its existence here, and lookup
/// it if needed.
const HAS_TARGET_FEATURE = 1 << 8;
}
}

Expand Down
13 changes: 10 additions & 3 deletions crates/hir-ty/src/diagnostics/unsafe_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use hir_def::{
};

use crate::{
db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TyExt, TyKind,
db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TargetFeatures, TyExt,
TyKind,
};

/// Returns `(unsafe_exprs, fn_is_unsafe)`.
Expand Down Expand Up @@ -96,6 +97,7 @@ struct UnsafeVisitor<'a> {
inside_assignment: bool,
inside_union_destructure: bool,
unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
def_target_features: TargetFeatures,
}

impl<'a> UnsafeVisitor<'a> {
Expand All @@ -107,6 +109,10 @@ impl<'a> UnsafeVisitor<'a> {
unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
) -> Self {
let resolver = def.resolver(db.upcast());
let def_target_features = match def {
DefWithBodyId::FunctionId(func) => TargetFeatures::from_attrs(&db.attrs(func.into())),
_ => TargetFeatures::default(),
};
Self {
db,
infer,
Expand All @@ -117,6 +123,7 @@ impl<'a> UnsafeVisitor<'a> {
inside_assignment: false,
inside_union_destructure: false,
unsafe_expr_cb,
def_target_features,
}
}

Expand Down Expand Up @@ -181,7 +188,7 @@ impl<'a> UnsafeVisitor<'a> {
match expr {
&Expr::Call { callee, .. } => {
if let Some(func) = self.infer[callee].as_fn_def(self.db) {
if is_fn_unsafe_to_call(self.db, func) {
if is_fn_unsafe_to_call(self.db, func, &self.def_target_features) {
self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);
}
}
Expand Down Expand Up @@ -212,7 +219,7 @@ impl<'a> UnsafeVisitor<'a> {
if self
.infer
.method_resolution(current)
.map(|(func, _)| is_fn_unsafe_to_call(self.db, func))
.map(|(func, _)| is_fn_unsafe_to_call(self.db, func, &self.def_target_features))
.unwrap_or(false)
{
self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-ty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub use mapping::{
};
pub use method_resolution::check_orphan_rules;
pub use traits::TraitEnvironment;
pub use utils::{all_super_traits, direct_super_traits, is_fn_unsafe_to_call};
pub use utils::{all_super_traits, direct_super_traits, is_fn_unsafe_to_call, TargetFeatures};
pub use variance::Variance;

pub use chalk_ir::{
Expand Down
44 changes: 42 additions & 2 deletions crates/hir-ty/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,18 @@ use chalk_ir::{
DebruijnIndex,
};
use hir_def::{
attr::Attrs,
db::DefDatabase,
generics::{WherePredicate, WherePredicateTypeTarget},
lang_item::LangItem,
resolver::{HasResolver, TypeNs},
tt,
type_ref::{TraitBoundModifier, TypeRef},
EnumId, EnumVariantId, FunctionId, Lookup, OpaqueInternableThing, TraitId, TypeAliasId,
TypeOrConstParamId,
};
use hir_expand::name::Name;
use intern::sym;
use intern::{sym, Symbol};
use rustc_abi::TargetDataLayout;
use rustc_hash::FxHashSet;
use smallvec::{smallvec, SmallVec};
Expand Down Expand Up @@ -264,12 +266,50 @@ impl<'a> ClosureSubst<'a> {
}
}

pub fn is_fn_unsafe_to_call(db: &dyn HirDatabase, func: FunctionId) -> bool {
#[derive(Debug, Default)]
pub struct TargetFeatures {
enabled: FxHashSet<Symbol>,
}

impl TargetFeatures {
pub fn from_attrs(attrs: &Attrs) -> Self {
let enabled = attrs
.by_key(&sym::target_feature)
.tt_values()
.filter_map(|tt| {
match tt.token_trees().flat_tokens() {
[
tt::TokenTree::Leaf(tt::Leaf::Ident(enable_ident)),
tt::TokenTree::Leaf(tt::Leaf::Punct(tt::Punct { char: '=', .. })),
tt::TokenTree::Leaf(tt::Leaf::Literal(tt::Literal { kind: tt::LitKind::Str, symbol: features, .. })),
] if enable_ident.sym == sym::enable => Some(features),
_ => None,
}
})
.flat_map(|features| features.as_str().split(',').map(Symbol::intern))
.collect();
Self { enabled }
}
}

pub fn is_fn_unsafe_to_call(
db: &dyn HirDatabase,
func: FunctionId,
caller_target_features: &TargetFeatures,
) -> bool {
let data = db.function_data(func);
if data.is_unsafe() {
return true;
}

if data.has_target_feature() {
// RFC 2396 <https://rust-lang.github.io/rfcs/2396-target-feature-1.1.html>.
let callee_target_features = TargetFeatures::from_attrs(&db.attrs(func.into()));
if !caller_target_features.enabled.is_superset(&callee_target_features.enabled) {
return true;
}
}

let loc = func.lookup(db.upcast());
match loc.container {
hir_def::ItemContainerId::ExternBlockId(block) => {
Expand Down
4 changes: 3 additions & 1 deletion crates/hir/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ impl HirDisplay for Function {
if data.is_async() {
f.write_str("async ")?;
}
if self.is_unsafe_to_call(db) {
// FIXME: This will show `unsafe` for functions that are `#[target_feature]` but not unsafe
// (they are conditionally unsafe to call). We probably should show something else.
if self.is_unsafe_to_call(db, None) {
f.write_str("unsafe ")?;
}
if let Some(abi) = &data.abi {
Expand Down
7 changes: 5 additions & 2 deletions crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2361,8 +2361,11 @@ impl Function {
db.attrs(self.id.into()).is_unstable()
}

pub fn is_unsafe_to_call(self, db: &dyn HirDatabase) -> bool {
hir_ty::is_fn_unsafe_to_call(db, self.id)
pub fn is_unsafe_to_call(self, db: &dyn HirDatabase, caller: Option<Function>) -> bool {
let target_features = caller
.map(|caller| hir_ty::TargetFeatures::from_attrs(&db.attrs(caller.id.into())))
.unwrap_or_default();
hir_ty::is_fn_unsafe_to_call(db, self.id, &target_features)
}

/// Whether this function declaration has a definition.
Expand Down
7 changes: 7 additions & 0 deletions crates/hir/src/semantics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2040,6 +2040,13 @@ impl SemanticsScope<'_> {
Crate { id: self.resolver.krate() }
}

pub fn containing_function(&self) -> Option<Function> {
self.resolver.body_owner().and_then(|owner| match owner {
DefWithBodyId::FunctionId(id) => Some(id.into()),
_ => None,
})
}

pub(crate) fn resolver(&self) -> &Resolver {
&self.resolver
}
Expand Down
12 changes: 9 additions & 3 deletions crates/hir/src/term_search/tactics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ pub(super) fn free_function<'a, DB: HirDatabase>(
let ret_ty = it.ret_type_with_args(db, generics.iter().cloned());
// Filter out private and unsafe functions
if !it.is_visible_from(db, module)
|| it.is_unsafe_to_call(db)
|| it.is_unsafe_to_call(db, None)
|| it.is_unstable(db)
|| ctx.config.enable_borrowcheck && ret_ty.contains_reference(db)
|| ret_ty.is_raw_ptr()
Expand Down Expand Up @@ -470,7 +470,10 @@ pub(super) fn impl_method<'a, DB: HirDatabase>(
}

// Filter out private and unsafe functions
if !it.is_visible_from(db, module) || it.is_unsafe_to_call(db) || it.is_unstable(db) {
if !it.is_visible_from(db, module)
|| it.is_unsafe_to_call(db, None)
|| it.is_unstable(db)
{
return None;
}

Expand Down Expand Up @@ -658,7 +661,10 @@ pub(super) fn impl_static_method<'a, DB: HirDatabase>(
}

// Filter out private and unsafe functions
if !it.is_visible_from(db, module) || it.is_unsafe_to_call(db) || it.is_unstable(db) {
if !it.is_visible_from(db, module)
|| it.is_unsafe_to_call(db, None)
|| it.is_unstable(db)
{
return None;
}

Expand Down
4 changes: 4 additions & 0 deletions crates/ide-completion/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ pub(crate) struct CompletionContext<'a> {
pub(crate) krate: hir::Crate,
/// The module of the `scope`.
pub(crate) module: hir::Module,
/// The function where we're completing, if inside a function.
pub(crate) containing_function: Option<hir::Function>,
/// Whether nightly toolchain is used. Cached since this is looked up a lot.
pub(crate) is_nightly: bool,
/// The edition of the current crate
Expand Down Expand Up @@ -760,6 +762,7 @@ impl<'a> CompletionContext<'a> {

let krate = scope.krate();
let module = scope.module();
let containing_function = scope.containing_function();
let edition = krate.edition(db);

let toolchain = db.toolchain_channel(krate.into());
Expand Down Expand Up @@ -874,6 +877,7 @@ impl<'a> CompletionContext<'a> {
token,
krate,
module,
containing_function,
is_nightly,
edition,
expected_name,
Expand Down
18 changes: 9 additions & 9 deletions crates/ide-completion/src/render/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ fn render(
let detail = if ctx.completion.config.full_function_signatures {
detail_full(db, func, ctx.completion.edition)
} else {
detail(db, func, ctx.completion.edition)
detail(ctx.completion, func, ctx.completion.edition)
};
item.set_documentation(ctx.docs(func))
.set_deprecated(ctx.is_deprecated(func) || ctx.is_deprecated_assoc_item(func))
Expand Down Expand Up @@ -307,26 +307,26 @@ fn ref_of_param(ctx: &CompletionContext<'_>, arg: &str, ty: &hir::Type) -> &'sta
""
}

fn detail(db: &dyn HirDatabase, func: hir::Function, edition: Edition) -> String {
let mut ret_ty = func.ret_type(db);
fn detail(ctx: &CompletionContext<'_>, func: hir::Function, edition: Edition) -> String {
let mut ret_ty = func.ret_type(ctx.db);
let mut detail = String::new();

if func.is_const(db) {
if func.is_const(ctx.db) {
format_to!(detail, "const ");
}
if func.is_async(db) {
if func.is_async(ctx.db) {
format_to!(detail, "async ");
if let Some(async_ret) = func.async_ret_type(db) {
if let Some(async_ret) = func.async_ret_type(ctx.db) {
ret_ty = async_ret;
}
}
if func.is_unsafe_to_call(db) {
if func.is_unsafe_to_call(ctx.db, ctx.containing_function) {
format_to!(detail, "unsafe ");
}

format_to!(detail, "fn({})", params_display(db, func, edition));
format_to!(detail, "fn({})", params_display(ctx.db, func, edition));
if !ret_ty.is_unit() {
format_to!(detail, " -> {}", ret_ty.display(db, edition));
format_to!(detail, " -> {}", ret_ty.display(ctx.db, edition));
}
detail
}
Expand Down
20 changes: 20 additions & 0 deletions crates/ide-diagnostics/src/handlers/missing_unsafe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -812,4 +812,24 @@ fn main() {
"#,
)
}

#[test]
fn target_feature() {
check_diagnostics(
r#"
#[target_feature(enable = "avx")]
fn foo() {}

#[target_feature(enable = "avx,avx2")]
fn bar() {
foo();
}

fn baz() {
foo();
// ^^^^^ 💡 error: call to unsafe function is unsafe and requires an unsafe function or block
}
"#,
);
}
}
8 changes: 6 additions & 2 deletions crates/ide/src/syntax_highlighting/highlight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,11 @@ pub(super) fn highlight_def(
}
}

if func.is_unsafe_to_call(db) {
// FIXME: Passing `None` here means not-unsafe functions with `#[target_feature]` will be
// highlighted as unsafe, even when the current target features set is a superset (RFC 2396).
// We probably should consider checking the current function, but I found no easy way to do
// that (also I'm worried about perf). There's also an instance below.
if func.is_unsafe_to_call(db, None) {
h |= HlMod::Unsafe;
}
if func.is_async(db) {
Expand Down Expand Up @@ -589,7 +593,7 @@ fn highlight_method_call(

let mut h = SymbolKind::Method.into();

if func.is_unsafe_to_call(sema.db) || sema.is_unsafe_method_call(method_call) {
if func.is_unsafe_to_call(sema.db, None) || sema.is_unsafe_method_call(method_call) {
h |= HlMod::Unsafe;
}
if func.is_async(sema.db) {
Expand Down
2 changes: 2 additions & 0 deletions crates/intern/src/symbol/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,8 @@ define_symbols! {
system,
sysv64,
Target,
target_feature,
enable,
termination,
test_case,
test,
Expand Down
Loading