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.

Sign up for GitHub

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

Jump to bottom

Port improved unused_unsafe check to -Zthir_unsafeck #99379

Closed
wants to merge 1 commit into from
Closed

Port improved unused_unsafe check to -Zthir_unsafeck #99379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
215 changes: 145 additions & 70 deletions compiler/rustc_mir_build/src/check_unsafety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
use crate::build::ExprCategory;
use hir::intravisit;
use rustc_data_structures::fx::FxHashMap;
use rustc_middle::thir::visit::{self, Visitor};

use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_middle::mir::BorrowKind;
use rustc_middle::thir::*;
use rustc_middle::mir::{BorrowKind, UnusedUnsafe, UsedUnsafeBlockData};
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt};
use rustc_middle::{lint, thir::*};
use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::symbol::Symbol;
use rustc_span::Span;

use std::borrow::Cow;
use std::collections::hash_map;
use std::ops::Bound;

struct UnsafetyVisitor<'a, 'tcx> {
Expand All @@ -24,7 +27,6 @@ struct UnsafetyVisitor<'a, 'tcx> {
/// The current "safety context". This notably tracks whether we are in an
/// `unsafe` block, and whether it has been used.
safety_context: SafetyContext,
body_unsafety: BodyUnsafety,
/// The `#[target_feature]` attributes of the body. Used for checking
/// calls to functions with `#[target_feature]` (RFC 2396).
body_target_features: &'tcx [Symbol],
Expand All @@ -34,51 +36,41 @@ struct UnsafetyVisitor<'a, 'tcx> {
in_union_destructure: bool,
param_env: ParamEnv<'tcx>,
inside_adt: bool,
used_unsafe_blocks: &'a mut FxHashMap<hir::HirId, UsedUnsafeBlockData>,
}

impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
fn in_safety_context(&mut self, safety_context: SafetyContext, f: impl FnOnce(&mut Self)) {
if let (
SafetyContext::UnsafeBlock { span: enclosing_span, .. },
SafetyContext::UnsafeBlock { span: block_span, hir_id, .. },
) = (self.safety_context, safety_context)
{
self.warn_unused_unsafe(
hir_id,
block_span,
Some((self.tcx.sess.source_map().guess_head_span(enclosing_span), "block")),
);
f(self);
} else {
let prev_context = self.safety_context;
self.safety_context = safety_context;
let prev_context = self.safety_context;
self.safety_context = safety_context;

f(self);
f(self);

if let SafetyContext::UnsafeBlock { used: false, span, hir_id } = self.safety_context {
self.warn_unused_unsafe(
hir_id,
span,
if self.unsafe_op_in_unsafe_fn_allowed() {
self.body_unsafety.unsafe_fn_sig_span().map(|span| (span, "fn"))
} else {
None
},
);
}
self.safety_context = prev_context;
}
self.safety_context = prev_context;
}

fn requires_unsafe(&mut self, span: Span, kind: UnsafeOpKind) {
use UsedUnsafeBlockData::{AllAllowedInUnsafeFn, SomeDisallowedInUnsafeFn};

let unsafe_op_in_unsafe_fn_allowed = self.unsafe_op_in_unsafe_fn_allowed();
match self.safety_context {
SafetyContext::BuiltinUnsafeBlock => {}
SafetyContext::UnsafeBlock { ref mut used, .. } => {
if !self.body_unsafety.is_unsafe() || !unsafe_op_in_unsafe_fn_allowed {
// Mark this block as useful
*used = true;
}
SafetyContext::UnsafeBlock(hir_id) => {
let new_usage = if unsafe_op_in_unsafe_fn_allowed {
AllAllowedInUnsafeFn(self.hir_context)
} else {
SomeDisallowedInUnsafeFn
};
match self.used_unsafe_blocks.entry(hir_id) {
hash_map::Entry::Occupied(mut entry) => {
if new_usage == SomeDisallowedInUnsafeFn {
*entry.get_mut() = SomeDisallowedInUnsafeFn;
}
}
hash_map::Entry::Vacant(entry) => {
entry.insert(new_usage);
}
};
}
SafetyContext::UnsafeFn if unsafe_op_in_unsafe_fn_allowed => {}
SafetyContext::UnsafeFn => {
Expand Down Expand Up @@ -117,24 +109,6 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
}
}

fn warn_unused_unsafe(
&self,
hir_id: hir::HirId,
block_span: Span,
enclosing_unsafe: Option<(Span, &'static str)>,
) {
let block_span = self.tcx.sess.source_map().guess_head_span(block_span);
self.tcx.struct_span_lint_hir(UNUSED_UNSAFE, hir_id, block_span, |lint| {
let msg = "unnecessary `unsafe` block";
let mut db = lint.build(msg);
db.span_label(block_span, msg);
if let Some((span, kind)) = enclosing_unsafe {
db.span_label(span, format!("because it's nested under this `unsafe` {}", kind));
}
db.emit();
});
}

/// Whether the `unsafe_op_in_unsafe_fn` lint is `allow`ed at the current HIR node.
fn unsafe_op_in_unsafe_fn_allowed(&self) -> bool {
self.tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, self.hir_context).0 == Level::Allow
Expand Down Expand Up @@ -200,10 +174,9 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
});
}
BlockSafety::ExplicitUnsafe(hir_id) => {
self.in_safety_context(
SafetyContext::UnsafeBlock { span: block.span, hir_id, used: false },
|this| visit::walk_block(this, block),
);
self.in_safety_context(SafetyContext::UnsafeBlock(hir_id), |this| {
visit::walk_block(this, block)
});
}
BlockSafety::Safe => {
visit::walk_block(self, block);
Expand Down Expand Up @@ -421,8 +394,12 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
});
let closure_thir = &closure_thir.borrow();
let hir_context = self.tcx.hir().local_def_id_to_hir_id(closure_id);
let mut closure_visitor =
UnsafetyVisitor { thir: closure_thir, hir_context, ..*self };
let mut closure_visitor = UnsafetyVisitor {
thir: closure_thir,
hir_context,
used_unsafe_blocks: self.used_unsafe_blocks,
..*self
};
closure_visitor.visit_expr(&closure_thir[expr]);
// Unsafe blocks can be used in closures, make sure to take it into account
self.safety_context = closure_visitor.safety_context;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safety_context has a stack-like behaviour. Should this be an assert_eq?

Expand Down Expand Up @@ -495,7 +472,7 @@ enum SafetyContext {
Safe,
BuiltinUnsafeBlock,
UnsafeFn,
UnsafeBlock { span: Span, hir_id: hir::HirId, used: bool },
UnsafeBlock(hir::HirId),
}

#[derive(Clone, Copy)]
Expand All @@ -512,14 +489,6 @@ impl BodyUnsafety {
fn is_unsafe(&self) -> bool {
matches!(self, BodyUnsafety::Unsafe(_))
}

/// If the body is unsafe, returns the `Span` of its signature.
fn unsafe_fn_sig_span(self) -> Option<Span> {
match self {
BodyUnsafety::Unsafe(span) => Some(span),
BodyUnsafety::Safe => None,
}
}
}

#[derive(Clone, Copy, PartialEq)]
Expand Down Expand Up @@ -616,6 +585,106 @@ impl UnsafeOpKind {
}
}

/// Context information for [`UnusedUnsafeVisitor`] traversal,
/// saves (innermost) relevant context
#[derive(Copy, Clone, Debug)]
enum Context {
Safe,
/// in an `unsafe fn`
UnsafeFn(hir::HirId),
/// in a *used* `unsafe` block
/// (i.e. a block without unused-unsafe warning)
UnsafeBlock(hir::HirId),
}

struct UnusedUnsafeVisitor<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
used_unsafe_blocks: &'a FxHashMap<hir::HirId, UsedUnsafeBlockData>,
context: Context,
}

impl<'tcx> intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'_, 'tcx> {
fn visit_block(&mut self, block: &'tcx hir::Block<'tcx>) {
use UsedUnsafeBlockData::{AllAllowedInUnsafeFn, SomeDisallowedInUnsafeFn};

if let hir::BlockCheckMode::UnsafeBlock(hir::UnsafeSource::UserProvided) = block.rules {
let used = match self.tcx.lint_level_at_node(UNUSED_UNSAFE, block.hir_id) {
(Level::Allow, _) => Some(SomeDisallowedInUnsafeFn),
_ => self.used_unsafe_blocks.get(&block.hir_id).copied(),
};
let unused_unsafe = match (self.context, used) {
(_, None) => UnusedUnsafe::Unused,
(Context::Safe, Some(_))
| (Context::UnsafeFn(_), Some(SomeDisallowedInUnsafeFn)) => {
let previous_context = self.context;
self.context = Context::UnsafeBlock(block.hir_id);
intravisit::walk_block(self, block);
self.context = previous_context;
return;
}
(Context::UnsafeFn(hir_id), Some(AllAllowedInUnsafeFn(lint_root))) => {
UnusedUnsafe::InUnsafeFn(hir_id, lint_root)
}
(Context::UnsafeBlock(hir_id), Some(_)) => UnusedUnsafe::InUnsafeBlock(hir_id),
};
report_unused_unsafe(self.tcx, block.hir_id, unused_unsafe);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we report this at the end of UnsafetyVisitor::in_safety_context?

}
intravisit::walk_block(self, block);
}

fn visit_fn(
&mut self,
fk: intravisit::FnKind<'tcx>,
_fd: &'tcx hir::FnDecl<'tcx>,
b: hir::BodyId,
_s: rustc_span::Span,
_id: hir::HirId,
) {
if matches!(fk, intravisit::FnKind::Closure) {
self.visit_body(self.tcx.hir().body(b))
}
}
}

fn report_unused_unsafe(tcx: TyCtxt<'_>, id: hir::HirId, kind: UnusedUnsafe) {
let span = tcx.sess.source_map().guess_head_span(tcx.hir().span(id));
tcx.struct_span_lint_hir(UNUSED_UNSAFE, id, span, |lint| {
let msg = "unnecessary `unsafe` block";
let mut db = lint.build(msg);
db.span_label(span, msg);
match kind {
UnusedUnsafe::Unused => {}
UnusedUnsafe::InUnsafeBlock(id) => {
db.span_label(
tcx.sess.source_map().guess_head_span(tcx.hir().span(id)),
format!("because it's nested under this `unsafe` block"),
);
}
UnusedUnsafe::InUnsafeFn(id, usage_lint_root) => {
db.span_label(
tcx.sess.source_map().guess_head_span(tcx.hir().span(id)),
format!("because it's nested under this `unsafe` fn"),
)
.note(
"this `unsafe` block does contain unsafe operations, \
but those are already allowed in an `unsafe fn`",
);
let (level, source) =
tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, usage_lint_root);
assert_eq!(level, Level::Allow);
lint::explain_lint_level_source(
UNSAFE_OP_IN_UNSAFE_FN,
Level::Allow,
source,
&mut db,
);
}
}

db.emit();
});
}

pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalDefId>) {
// THIR unsafeck is gated under `-Z thir-unsafeck`
if !tcx.sess.opts.unstable_opts.thir_unsafeck {
Expand Down Expand Up @@ -655,14 +724,20 @@ pub fn check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalD
thir,
safety_context,
hir_context: hir_id,
body_unsafety,
body_target_features,
assignment_info: None,
in_union_destructure: false,
param_env: tcx.param_env(def.did),
inside_adt: false,
used_unsafe_blocks: &mut Default::default(),
};
visitor.visit_expr(&thir[expr]);
let mut visitor2 = UnusedUnsafeVisitor {
tcx,
used_unsafe_blocks: visitor.used_unsafe_blocks,
context: if body_unsafety.is_unsafe() { Context::UnsafeFn(hir_id) } else { Context::Safe },
};
intravisit::Visitor::visit_body(&mut visitor2, tcx.hir().body(tcx.hir().body_owned_by(hir_id)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this traversal happen on HIR and not THIR?

}

pub(crate) fn thir_check_unsafety<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) {
Expand Down
61 changes: 0 additions & 61 deletions src/test/ui/span/lint-unused-unsafe-thir.rs
View file
Open in desktop

This file was deleted.

Loading