Skip to content

Commit

Permalink
Rollup merge of rust-lang#71862 - LeSeulArtichaut:unsafe-block-in-uns…
Browse files Browse the repository at this point in the history
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
  • Loading branch information
RalfJung authored May 29, 2020
2 parents b08168e + 0e3b31c commit 4e2351e
Show file tree
Hide file tree
Showing 15 changed files with 527 additions and 29 deletions.
3 changes: 3 additions & 0 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,9 @@ declare_features! (
/// Allows the use of `#[ffi_const]` on foreign functions.
(active, ffi_const, "1.45.0", Some(58328), None),

/// No longer treat an unsafe function as an unsafe block.
(active, unsafe_block_in_unsafe_fn, "1.45.0", Some(71668), None),

// -------------------------------------------------------------------------
// feature-group-end: actual feature gates
// -------------------------------------------------------------------------
Expand Down
21 changes: 19 additions & 2 deletions src/librustc_lint/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ use rustc_middle::lint::LintDiagnosticBuilder;
use rustc_middle::lint::{struct_lint_level, LintLevelMap, LintLevelSets, LintSet, LintSource};
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::TyCtxt;
use rustc_session::lint::{builtin, Level, Lint};
use rustc_session::lint::{builtin, Level, Lint, LintId};
use rustc_session::parse::feature_err;
use rustc_session::Session;
use rustc_span::source_map::MultiSpan;
use rustc_span::symbol::{sym, Symbol};
use rustc_span::{source_map::MultiSpan, Span, DUMMY_SP};

use std::cmp;

Expand Down Expand Up @@ -80,11 +80,13 @@ impl<'s> LintLevelsBuilder<'s> {
let level = cmp::min(level, self.sets.lint_cap);

let lint_flag_val = Symbol::intern(lint_name);

let ids = match store.find_lints(&lint_name) {
Ok(ids) => ids,
Err(_) => continue, // errors handled in check_lint_name_cmdline above
};
for id in ids {
self.check_gated_lint(id, DUMMY_SP);
let src = LintSource::CommandLine(lint_flag_val);
specs.insert(id, (level, src));
}
Expand Down Expand Up @@ -213,6 +215,7 @@ impl<'s> LintLevelsBuilder<'s> {
CheckLintNameResult::Ok(ids) => {
let src = LintSource::Node(name, li.span(), reason);
for id in ids {
self.check_gated_lint(*id, attr.span);
specs.insert(*id, (level, src));
}
}
Expand Down Expand Up @@ -383,6 +386,20 @@ impl<'s> LintLevelsBuilder<'s> {
BuilderPush { prev, changed: prev != self.cur }
}

fn check_gated_lint(&self, id: LintId, span: Span) {
if id == LintId::of(builtin::UNSAFE_OP_IN_UNSAFE_FN)
&& !self.sess.features_untracked().unsafe_block_in_unsafe_fn
{
feature_err(
&self.sess.parse_sess,
sym::unsafe_block_in_unsafe_fn,
span,
"the `unsafe_op_in_unsafe_fn` lint is unstable",
)
.emit();
}
}

/// Called after `push` when the scope of a set of attributes are exited.
pub fn pop(&mut self, push: BuilderPush) {
self.cur = push.prev;
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ impl<'tcx> Body<'tcx> {
}
}

#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]
#[derive(Copy, Clone, PartialEq, Eq, Debug, RustcEncodable, RustcDecodable, HashStable)]
pub enum Safety {
Safe,
/// Unsafe because of a PushUnsafeBlock
Expand Down
14 changes: 13 additions & 1 deletion src/librustc_middle/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,27 @@ use super::{Field, SourceInfo};

#[derive(Copy, Clone, PartialEq, RustcEncodable, RustcDecodable, HashStable)]
pub enum UnsafetyViolationKind {
/// Only permitted in regular `fn`s, prohibitted in `const fn`s.
General,
/// Permitted both in `const fn`s and regular `fn`s.
GeneralAndConstFn,
BorrowPacked(hir::HirId),
/// Borrow of packed field.
/// Has to be handled as a lint for backwards compatibility.
BorrowPacked,
/// Unsafe operation in an `unsafe fn` but outside an `unsafe` block.
/// Has to be handled as a lint for backwards compatibility.
/// Should stay gated under `#![feature(unsafe_block_in_unsafe_fn)]`.
UnsafeFn,
/// Borrow of packed field in an `unsafe fn` but outside an `unsafe` block.
/// Has to be handled as a lint for backwards compatibility.
/// Should stay gated under `#![feature(unsafe_block_in_unsafe_fn)]`.
UnsafeFnBorrowPacked,
}

#[derive(Copy, Clone, PartialEq, RustcEncodable, RustcDecodable, HashStable)]
pub struct UnsafetyViolation {
pub source_info: SourceInfo,
pub lint_root: hir::HirId,
pub description: Symbol,
pub details: Symbol,
pub kind: UnsafetyViolationKind,
Expand Down
140 changes: 117 additions & 23 deletions src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::hir_id::HirId;
use rustc_hir::intravisit;
use rustc_hir::Node;
use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::cast::CastTy;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::{SAFE_PACKED_BORROWS, UNUSED_UNSAFE};
use rustc_session::lint::builtin::{SAFE_PACKED_BORROWS, UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;
use rustc_span::symbol::{sym, Symbol};

use std::ops::Bound;
Expand Down Expand Up @@ -202,25 +204,30 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {

if context.is_borrow() {
if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
let source_info = self.source_info;
let lint_root = self.body.source_scopes[source_info.scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;
self.require_unsafe(
"borrow of packed field",
"fields of packed structs might be misaligned: dereferencing a \
misaligned pointer or even just creating a misaligned reference \
is undefined behavior",
UnsafetyViolationKind::BorrowPacked(lint_root),
UnsafetyViolationKind::BorrowPacked,
);
}
}

for (i, elem) in place.projection.iter().enumerate() {
let proj_base = &place.projection[..i];
let old_source_info = self.source_info;
if context.is_borrow() {
if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
self.require_unsafe(
"borrow of packed field",
"fields of packed structs might be misaligned: dereferencing a \
misaligned pointer or even just creating a misaligned reference \
is undefined behavior",
UnsafetyViolationKind::BorrowPacked,
);
}
}
let source_info = self.source_info;
if let [] = proj_base {
let decl = &self.body.local_decls[place.local];
if decl.internal {
Expand Down Expand Up @@ -301,7 +308,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
}
_ => {}
}
self.source_info = old_source_info;
self.source_info = source_info;
}
}
}
Expand All @@ -314,9 +321,15 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
kind: UnsafetyViolationKind,
) {
let source_info = self.source_info;
let lint_root = self.body.source_scopes[self.source_info.scope]
.local_data
.as_ref()
.assert_crate_local()
.lint_root;
self.register_violations(
&[UnsafetyViolation {
source_info,
lint_root,
description: Symbol::intern(description),
details: Symbol::intern(details),
kind,
Expand All @@ -343,22 +356,42 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
match violation.kind {
UnsafetyViolationKind::GeneralAndConstFn
| UnsafetyViolationKind::General => {}
UnsafetyViolationKind::BorrowPacked(_) => {
UnsafetyViolationKind::BorrowPacked => {
if self.min_const_fn {
// const fns don't need to be backwards compatible and can
// emit these violations as a hard error instead of a backwards
// compat lint
violation.kind = UnsafetyViolationKind::General;
}
}
UnsafetyViolationKind::UnsafeFn
| UnsafetyViolationKind::UnsafeFnBorrowPacked => {
bug!("`UnsafetyViolationKind::UnsafeFn` in an `Safe` context")
}
}
if !self.violations.contains(&violation) {
self.violations.push(violation)
}
}
false
}
// With the RFC 2585, no longer allow `unsafe` operations in `unsafe fn`s
Safety::FnUnsafe if self.tcx.features().unsafe_block_in_unsafe_fn => {
for violation in violations {
let mut violation = *violation;

if violation.kind == UnsafetyViolationKind::BorrowPacked {
violation.kind = UnsafetyViolationKind::UnsafeFnBorrowPacked;
} else {
violation.kind = UnsafetyViolationKind::UnsafeFn;
}
if !self.violations.contains(&violation) {
self.violations.push(violation)
}
}
false
}
// `unsafe` function bodies allow unsafe without additional unsafe blocks
// `unsafe` function bodies allow unsafe without additional unsafe blocks (before RFC 2585)
Safety::BuiltinUnsafe | Safety::FnUnsafe => true,
Safety::ExplicitUnsafe(hir_id) => {
// mark unsafe block as used if there are any unsafe operations inside
Expand All @@ -373,7 +406,7 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
UnsafetyViolationKind::GeneralAndConstFn => {}
// these things are forbidden in const fns
UnsafetyViolationKind::General
| UnsafetyViolationKind::BorrowPacked(_) => {
| UnsafetyViolationKind::BorrowPacked => {
let mut violation = *violation;
// const fns don't need to be backwards compatible and can
// emit these violations as a hard error instead of a backwards
Expand All @@ -383,6 +416,10 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
self.violations.push(violation)
}
}
UnsafetyViolationKind::UnsafeFn
| UnsafetyViolationKind::UnsafeFnBorrowPacked => bug!(
"`UnsafetyViolationKind::UnsafeFn` in an `ExplicitUnsafe` context"
),
}
}
}
Expand Down Expand Up @@ -575,9 +612,12 @@ fn is_enclosed(
kind: hir::ItemKind::Fn(ref sig, _, _), ..
})) = tcx.hir().find(parent_id)
{
match sig.header.unsafety {
hir::Unsafety::Unsafe => Some(("fn".to_string(), parent_id)),
hir::Unsafety::Normal => None,
if sig.header.unsafety == hir::Unsafety::Unsafe
&& !tcx.features().unsafe_block_in_unsafe_fn
{
Some(("fn".to_string(), parent_id))
} else {
None
}
} else {
is_enclosed(tcx, used_unsafe, parent_id)
Expand Down Expand Up @@ -630,40 +670,90 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: DefId) {
let UnsafetyCheckResult { violations, unsafe_blocks } =
tcx.unsafety_check_result(def_id.expect_local());

for &UnsafetyViolation { source_info, description, details, kind } in violations.iter() {
for &UnsafetyViolation { source_info, lint_root, description, details, kind } in
violations.iter()
{
// Report an error.
let unsafe_fn_msg =
if unsafe_op_in_unsafe_fn_allowed(tcx, lint_root) { " function or" } else { "" };

match kind {
UnsafetyViolationKind::GeneralAndConstFn | UnsafetyViolationKind::General => {
// once
struct_span_err!(
tcx.sess,
source_info.span,
E0133,
"{} is unsafe and requires unsafe function or block",
description
"{} is unsafe and requires unsafe{} block",
description,
unsafe_fn_msg,
)
.span_label(source_info.span, &*description.as_str())
.note(&details.as_str())
.emit();
}
UnsafetyViolationKind::BorrowPacked(lint_hir_id) => {
UnsafetyViolationKind::BorrowPacked => {
if let Some(impl_def_id) = builtin_derive_def_id(tcx, def_id) {
tcx.ensure().unsafe_derive_on_repr_packed(impl_def_id);
} else {
tcx.struct_span_lint_hir(
SAFE_PACKED_BORROWS,
lint_hir_id,
lint_root,
source_info.span,
|lint| {
lint.build(&format!(
"{} is unsafe and requires unsafe function or block (error E0133)",
description
"{} is unsafe and requires unsafe{} block (error E0133)",
description, unsafe_fn_msg,
))
.note(&details.as_str())
.emit()
},
)
}
}
UnsafetyViolationKind::UnsafeFn => tcx.struct_span_lint_hir(
UNSAFE_OP_IN_UNSAFE_FN,
lint_root,
source_info.span,
|lint| {
lint.build(&format!(
"{} is unsafe and requires unsafe block (error E0133)",
description,
))
.span_label(source_info.span, &*description.as_str())
.note(&details.as_str())
.emit();
},
),
UnsafetyViolationKind::UnsafeFnBorrowPacked => {
// When `unsafe_op_in_unsafe_fn` is disallowed, the behavior of safe and unsafe functions
// should be the same in terms of warnings and errors. Therefore, with `#[warn(safe_packed_borrows)]`,
// a safe packed borrow should emit a warning *but not an error* in an unsafe function,
// just like in a safe function, even if `unsafe_op_in_unsafe_fn` is `deny`.
//
// Also, `#[warn(unsafe_op_in_unsafe_fn)]` can't cause any new errors. Therefore, with
// `#[deny(safe_packed_borrows)]` and `#[warn(unsafe_op_in_unsafe_fn)]`, a packed borrow
// should only issue a warning for the sake of backwards compatibility.
//
// The solution those 2 expectations is to always take the minimum of both lints.
// This prevent any new errors (unless both lints are explicitely set to `deny`).
let lint = if tcx.lint_level_at_node(SAFE_PACKED_BORROWS, lint_root).0
<= tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, lint_root).0
{
SAFE_PACKED_BORROWS
} else {
UNSAFE_OP_IN_UNSAFE_FN
};
tcx.struct_span_lint_hir(&lint, lint_root, source_info.span, |lint| {
lint.build(&format!(
"{} is unsafe and requires unsafe block (error E0133)",
description,
))
.span_label(source_info.span, &*description.as_str())
.note(&details.as_str())
.emit();
})
}
}
}

Expand All @@ -683,3 +773,7 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: DefId) {
report_unused_unsafe(tcx, &unsafe_used, block_id);
}
}

fn unsafe_op_in_unsafe_fn_allowed(tcx: TyCtxt<'_>, id: HirId) -> bool {
tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, id).0 == Level::Allow
}
Loading

0 comments on commit 4e2351e

Please sign in to comment.