From c089a17854669925c008a5944d0490f1692dde7e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Apr 2013 11:11:50 -0400 Subject: [PATCH] Improve the unused unsafe block warning to include unsafe blocks in unsafe functions --- src/librustc/middle/borrowck/check_loans.rs | 25 ++-------- src/librustc/middle/typeck/check/mod.rs | 55 +++++++++++++++------ src/test/compile-fail/unused-unsafe.rs | 34 ++++++++++--- 3 files changed, 71 insertions(+), 43 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 6f3075717ed..8fed8e78094 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -20,6 +20,7 @@ use core::prelude::*; use middle::moves; +use middle::typeck::check::PurityState; use middle::borrowck::{Loan, bckerr, BorrowckCtxt, inherent_mutability}; use middle::borrowck::{ReqMaps, root_map_key, save_and_restore_managed}; use middle::borrowck::{MoveError, MoveOk, MoveFromIllegalCmt}; @@ -41,11 +42,6 @@ use syntax::codemap::span; use syntax::print::pprust; use syntax::visit; -struct PurityState { - def: ast::node_id, - purity: ast::purity -} - struct CheckLoanCtxt { bccx: @BorrowckCtxt, req_maps: ReqMaps, @@ -85,8 +81,7 @@ pub fn check_loans(bccx: @BorrowckCtxt, bccx: bccx, req_maps: req_maps, reported: HashSet::new(), - declared_purity: @mut PurityState { purity: ast::impure_fn, - def: 0 }, + declared_purity: @mut PurityState::function(ast::impure_fn, 0), fn_args: @mut @~[] }; let vt = visit::mk_vt(@visit::Visitor {visit_expr: check_loans_in_expr, @@ -658,9 +653,7 @@ fn check_loans_in_fn(fk: &visit::fn_kind, debug!("purity on entry=%?", copy self.declared_purity); do save_and_restore_managed(self.declared_purity) { do save_and_restore_managed(self.fn_args) { - self.declared_purity = @mut PurityState { - purity: declared_purity, def: src - }; + self.declared_purity = @mut PurityState::function(declared_purity, src); match *fk { visit::fk_anon(*) | @@ -810,17 +803,7 @@ fn check_loans_in_block(blk: &ast::blk, do save_and_restore_managed(self.declared_purity) { self.check_for_conflicting_loans(blk.node.id); - match blk.node.rules { - ast::default_blk => { - } - ast::unsafe_blk => { - *self.declared_purity = PurityState { - purity: ast::unsafe_fn, - def: blk.node.id, - }; - } - } - + *self.declared_purity = self.declared_purity.recurse(blk); visit::visit_block(blk, self, vt); } } diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index dd147d9e468..536de78c752 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/src/librustc/middle/typeck/check/mod.rs @@ -115,6 +115,7 @@ use core::result::{Result, Ok, Err}; use core::result; use core::str; use core::vec; +use core::util::replace; use std::list::Nil; use syntax::abi::AbiSet; use syntax::ast::{provided, required}; @@ -179,9 +180,36 @@ pub enum FnKind { Vanilla } -struct PurityState { +pub struct PurityState { + def: ast::node_id, purity: ast::purity, - from: ast::node_id, + priv from_fn: bool +} + +pub impl PurityState { + pub fn function(purity: ast::purity, def: ast::node_id) -> PurityState { + PurityState { def: def, purity: purity, from_fn: true } + } + + pub fn recurse(&mut self, blk: &ast::blk) -> PurityState { + match self.purity { + // If this unsafe, then if the outer function was already marked as + // unsafe we shouldn't attribute the unsafe'ness to the block. This + // way the block can be warned about instead of ignoring this + // extraneous block (functions are never warned about). + ast::unsafe_fn if self.from_fn => *self, + + purity => { + let (purity, def) = match blk.node.rules { + ast::unsafe_blk => (ast::unsafe_fn, blk.node.id), + ast::default_blk => (purity, self.def), + }; + PurityState{ def: def, + purity: purity, + from_fn: false } + } + } + } } pub struct FnCtxt { @@ -243,7 +271,7 @@ pub fn blank_fn_ctxt(ccx: @mut CrateCtxt, @mut FnCtxt { ret_ty: rty, indirect_ret_ty: None, - ps: PurityState { purity: ast::pure_fn, from: 0 }, + ps: PurityState::function(ast::pure_fn, 0), region_lb: region_bnd, in_scope_regions: @Nil, fn_kind: Vanilla, @@ -348,7 +376,7 @@ pub fn check_fn(ccx: @mut CrateCtxt, @mut FnCtxt { ret_ty: ret_ty, indirect_ret_ty: indirect_ret_ty, - ps: PurityState { purity: purity, from: id }, + ps: PurityState::function(purity, id), region_lb: body.node.id, in_scope_regions: isr, fn_kind: fn_kind, @@ -876,8 +904,8 @@ pub impl FnCtxt { match self.ps.purity { ast::unsafe_fn => { // ok, but flag that we used the source of unsafeness - debug!("flagging %? as a used unsafe source", self.ps.from); - self.tcx().used_unsafe.insert(self.ps.from); + debug!("flagging %? as a used unsafe source", self.ps); + self.tcx().used_unsafe.insert(self.ps.def); } _ => { self.ccx.tcx.sess.span_err( @@ -1689,7 +1717,7 @@ pub fn check_expr_with_unifier(fcx: @mut FnCtxt, fcx.write_ty(expr.id, fty); let (inherited_purity, id) = - ty::determine_inherited_purity((fcx.ps.purity, fcx.ps.from), + ty::determine_inherited_purity((fcx.ps.purity, fcx.ps.def), (purity, expr.id), sigil); @@ -2929,16 +2957,11 @@ pub fn check_block(fcx0: @mut FnCtxt, blk: &ast::blk) { check_block_with_expected(fcx0, blk, None) } -pub fn check_block_with_expected(fcx0: @mut FnCtxt, +pub fn check_block_with_expected(fcx: @mut FnCtxt, blk: &ast::blk, expected: Option) { - let fcx = match blk.node.rules { - ast::unsafe_blk => @mut FnCtxt { - ps: PurityState { purity: ast::unsafe_fn, from: blk.node.id }, - .. copy *fcx0 - }, - ast::default_blk => fcx0 - }; + let prev = replace(&mut fcx.ps, fcx.ps.recurse(blk)); + do fcx.with_region_lb(blk.node.id) { let mut warned = false; let mut last_was_bot = false; @@ -2990,6 +3013,8 @@ pub fn check_block_with_expected(fcx0: @mut FnCtxt, } }; } + + fcx.ps = prev; } pub fn check_const(ccx: @mut CrateCtxt, diff --git a/src/test/compile-fail/unused-unsafe.rs b/src/test/compile-fail/unused-unsafe.rs index 7d9044f5d89..9552badb57f 100644 --- a/src/test/compile-fail/unused-unsafe.rs +++ b/src/test/compile-fail/unused-unsafe.rs @@ -12,30 +12,50 @@ #[deny(unused_unsafe)]; -use core::libc; +extern mod foo { + fn bar(); +} fn callback(_f: &fn() -> T) -> T { fail!() } +unsafe fn unsf() {} fn bad1() { unsafe {} } //~ ERROR: unnecessary `unsafe` block fn bad2() { unsafe { bad1() } } //~ ERROR: unnecessary `unsafe` block -unsafe fn bad4() { unsafe {} } //~ ERROR: unnecessary `unsafe` block -fn bad5() { unsafe { do callback {} } } //~ ERROR: unnecessary `unsafe` block +unsafe fn bad3() { unsafe {} } //~ ERROR: unnecessary `unsafe` block +fn bad4() { unsafe { do callback {} } } //~ ERROR: unnecessary `unsafe` block +unsafe fn bad5() { unsafe { unsf() } } //~ ERROR: unnecessary `unsafe` block +fn bad6() { + unsafe { //~ ERROR: unnecessary `unsafe` block + unsafe { // don't put the warning here + unsf() + } + } +} +unsafe fn bad7() { + unsafe { //~ ERROR: unnecessary `unsafe` block + unsafe { //~ ERROR: unnecessary `unsafe` block + unsf() + } + } +} -unsafe fn good0() { libc::exit(1) } -fn good1() { unsafe { libc::exit(1) } } +unsafe fn good0() { unsf() } +fn good1() { unsafe { unsf() } } fn good2() { /* bug uncovered when implementing warning about unused unsafe blocks. Be sure that when purity is inherited that the source of the unsafe-ness is tracked correctly */ unsafe { - unsafe fn what() -> ~[~str] { libc::exit(2) } + unsafe fn what() -> ~[~str] { fail!() } do callback { what(); } } } +unsafe fn good3() { foo::bar() } +fn good4() { unsafe { foo::bar() } } #[allow(unused_unsafe)] fn allowed() { unsafe {} } -fn main() { } +fn main() {}