From c089a17854669925c008a5944d0490f1692dde7e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Apr 2013 11:11:50 -0400 Subject: [PATCH 1/3] 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 6f3075717ed86..8fed8e78094c2 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 dd147d9e46887..536de78c752f9 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 7d9044f5d8921..9552badb57f67 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() {} From 4c08a8d6c31f55bfeed0ef0c2bf8b91f90415cfe Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Apr 2013 19:33:33 -0400 Subject: [PATCH 2/3] Removing more unnecessary unsafe blocks throughout --- src/libcore/gc.rs | 102 ++++++++++++++-------------- src/libcore/pipes.rs | 4 +- src/libcore/rt/sched/local_sched.rs | 6 +- src/libcore/rt/uvll.rs | 4 +- src/libcore/unstable.rs | 20 +++--- src/libcore/unstable/weak_task.rs | 4 +- src/librustc/middle/trans/base.rs | 6 +- src/libstd/arc.rs | 34 ++++------ 8 files changed, 83 insertions(+), 97 deletions(-) diff --git a/src/libcore/gc.rs b/src/libcore/gc.rs index 71d9ab439f34f..a6bae3c76631c 100644 --- a/src/libcore/gc.rs +++ b/src/libcore/gc.rs @@ -231,66 +231,64 @@ unsafe fn walk_gc_roots(mem: Memory, sentinel: **Word, visitor: Visitor) { // the stack. let mut reached_sentinel = ptr::is_null(sentinel); for stackwalk::walk_stack |frame| { - unsafe { - let pc = last_ret; - let Segment {segment: next_segment, boundary: boundary} = - find_segment_for_frame(frame.fp, segment); - segment = next_segment; - // Each stack segment is bounded by a morestack frame. The - // morestack frame includes two return addresses, one for - // morestack itself, at the normal offset from the frame - // pointer, and then a second return address for the - // function prologue (which called morestack after - // determining that it had hit the end of the stack). - // Since morestack itself takes two parameters, the offset - // for this second return address is 3 greater than the - // return address for morestack. - let ret_offset = if boundary { 4 } else { 1 }; - last_ret = *ptr::offset(frame.fp, ret_offset) as *Word; - - if ptr::is_null(pc) { - loop; - } + let pc = last_ret; + let Segment {segment: next_segment, boundary: boundary} = + find_segment_for_frame(frame.fp, segment); + segment = next_segment; + // Each stack segment is bounded by a morestack frame. The + // morestack frame includes two return addresses, one for + // morestack itself, at the normal offset from the frame + // pointer, and then a second return address for the + // function prologue (which called morestack after + // determining that it had hit the end of the stack). + // Since morestack itself takes two parameters, the offset + // for this second return address is 3 greater than the + // return address for morestack. + let ret_offset = if boundary { 4 } else { 1 }; + last_ret = *ptr::offset(frame.fp, ret_offset) as *Word; + + if ptr::is_null(pc) { + loop; + } - let mut delay_reached_sentinel = reached_sentinel; - let sp = is_safe_point(pc); - match sp { - Some(sp_info) => { - for walk_safe_point(frame.fp, sp_info) |root, tydesc| { - // Skip roots until we see the sentinel. - if !reached_sentinel { - if root == sentinel { - delay_reached_sentinel = true; - } - loop; + let mut delay_reached_sentinel = reached_sentinel; + let sp = is_safe_point(pc); + match sp { + Some(sp_info) => { + for walk_safe_point(frame.fp, sp_info) |root, tydesc| { + // Skip roots until we see the sentinel. + if !reached_sentinel { + if root == sentinel { + delay_reached_sentinel = true; } + loop; + } - // Skip null pointers, which can occur when a - // unique pointer has already been freed. - if ptr::is_null(*root) { - loop; - } + // Skip null pointers, which can occur when a + // unique pointer has already been freed. + if ptr::is_null(*root) { + loop; + } - if ptr::is_null(tydesc) { - // Root is a generic box. - let refcount = **root; - if mem | task_local_heap != 0 && refcount != -1 { - if !visitor(root, tydesc) { return; } - } else if mem | exchange_heap != 0 && refcount == -1 { - if !visitor(root, tydesc) { return; } - } - } else { - // Root is a non-immediate. - if mem | stack != 0 { - if !visitor(root, tydesc) { return; } - } + if ptr::is_null(tydesc) { + // Root is a generic box. + let refcount = **root; + if mem | task_local_heap != 0 && refcount != -1 { + if !visitor(root, tydesc) { return; } + } else if mem | exchange_heap != 0 && refcount == -1 { + if !visitor(root, tydesc) { return; } + } + } else { + // Root is a non-immediate. + if mem | stack != 0 { + if !visitor(root, tydesc) { return; } } } - } - None => () } - reached_sentinel = delay_reached_sentinel; + } + None => () } + reached_sentinel = delay_reached_sentinel; } } diff --git a/src/libcore/pipes.rs b/src/libcore/pipes.rs index 36cfdbf5617aa..2ec3afca61269 100644 --- a/src/libcore/pipes.rs +++ b/src/libcore/pipes.rs @@ -156,9 +156,7 @@ pub impl PacketHeader { unsafe fn unblock(&self) { let old_task = swap_task(&mut self.blocked_task, ptr::null()); if !old_task.is_null() { - unsafe { - rustrt::rust_task_deref(old_task) - } + rustrt::rust_task_deref(old_task) } match swap_state_acq(&mut self.state, Empty) { Empty | Blocked => (), diff --git a/src/libcore/rt/sched/local_sched.rs b/src/libcore/rt/sched/local_sched.rs index 77fbadf0bb7bb..2d1e06163beb8 100644 --- a/src/libcore/rt/sched/local_sched.rs +++ b/src/libcore/rt/sched/local_sched.rs @@ -80,10 +80,8 @@ pub unsafe fn unsafe_borrow() -> &mut Scheduler { } pub unsafe fn unsafe_borrow_io() -> &mut IoFactoryObject { - unsafe { - let sched = unsafe_borrow(); - return sched.event_loop.io().unwrap(); - } + let sched = unsafe_borrow(); + return sched.event_loop.io().unwrap(); } fn tls_key() -> tls::Key { diff --git a/src/libcore/rt/uvll.rs b/src/libcore/rt/uvll.rs index 640a69743ba6a..b7eff217ff8c6 100644 --- a/src/libcore/rt/uvll.rs +++ b/src/libcore/rt/uvll.rs @@ -98,7 +98,7 @@ pub enum uv_req_type { pub unsafe fn malloc_handle(handle: uv_handle_type) -> *c_void { assert!(handle != UV_UNKNOWN_HANDLE && handle != UV_HANDLE_TYPE_MAX); - let size = unsafe { rust_uv_handle_size(handle as uint) }; + let size = rust_uv_handle_size(handle as uint); let p = malloc(size); assert!(p.is_not_null()); return p; @@ -110,7 +110,7 @@ pub unsafe fn free_handle(v: *c_void) { pub unsafe fn malloc_req(req: uv_req_type) -> *c_void { assert!(req != UV_UNKNOWN_REQ && req != UV_REQ_TYPE_MAX); - let size = unsafe { rust_uv_req_size(req as uint) }; + let size = rust_uv_req_size(req as uint); let p = malloc(size); assert!(p.is_not_null()); return p; diff --git a/src/libcore/unstable.rs b/src/libcore/unstable.rs index a6bb93c20cd0b..4a69de26f6b13 100644 --- a/src/libcore/unstable.rs +++ b/src/libcore/unstable.rs @@ -262,18 +262,16 @@ pub impl Exclusive { // the exclusive. Supporting that is a work in progress. #[inline(always)] unsafe fn with(&self, f: &fn(x: &mut T) -> U) -> U { - unsafe { - let rec = get_shared_mutable_state(&self.x); - do (*rec).lock.lock { - if (*rec).failed { - fail!( - ~"Poisoned exclusive - another task failed inside!"); - } - (*rec).failed = true; - let result = f(&mut (*rec).data); - (*rec).failed = false; - result + let rec = get_shared_mutable_state(&self.x); + do (*rec).lock.lock { + if (*rec).failed { + fail!( + ~"Poisoned exclusive - another task failed inside!"); } + (*rec).failed = true; + let result = f(&mut (*rec).data); + (*rec).failed = false; + result } } diff --git a/src/libcore/unstable/weak_task.rs b/src/libcore/unstable/weak_task.rs index 4e2174fd5d24c..7a30bb92111b1 100644 --- a/src/libcore/unstable/weak_task.rs +++ b/src/libcore/unstable/weak_task.rs @@ -43,11 +43,11 @@ pub unsafe fn weaken_task(f: &fn(Port)) { let task = get_task_id(); // Expect the weak task service to be alive assert!(service.try_send(RegisterWeakTask(task, shutdown_chan))); - unsafe { rust_dec_kernel_live_count(); } + rust_dec_kernel_live_count(); do (|| { f(shutdown_port.take()) }).finally || { - unsafe { rust_inc_kernel_live_count(); } + rust_inc_kernel_live_count(); // Service my have already exited service.send(UnregisterWeakTask(task)); } diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index b86e9a512932a..68405f4fc5fe2 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -2628,13 +2628,11 @@ pub fn get_item_val(ccx: @CrateContext, id: ast::node_id) -> ValueRef { let class_ty = ty::lookup_item_type(tcx, parent_id).ty; // This code shouldn't be reached if the class is generic assert!(!ty::type_has_params(class_ty)); - let lldty = unsafe { - T_fn(~[ + let lldty = T_fn(~[ T_ptr(T_i8()), T_ptr(type_of(ccx, class_ty)) ], - T_nil()) - }; + T_nil()); let s = get_dtor_symbol(ccx, /*bad*/copy *pt, dt.node.id, None); /* Make the declaration for the dtor */ diff --git a/src/libstd/arc.rs b/src/libstd/arc.rs index 8abe0262314b5..33aa6171de4b7 100644 --- a/src/libstd/arc.rs +++ b/src/libstd/arc.rs @@ -177,15 +177,13 @@ pub impl MutexARC { */ #[inline(always)] unsafe fn access(&self, blk: &fn(x: &mut T) -> U) -> U { - unsafe { - let state = get_shared_mutable_state(&self.x); - // Borrowck would complain about this if the function were - // not already unsafe. See borrow_rwlock, far below. - do (&(*state).lock).lock { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data) - } + let state = get_shared_mutable_state(&self.x); + // Borrowck would complain about this if the function were + // not already unsafe. See borrow_rwlock, far below. + do (&(*state).lock).lock { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data) } } @@ -195,16 +193,14 @@ pub impl MutexARC { &self, blk: &fn(x: &'x mut T, c: &'c Condvar) -> U) -> U { - unsafe { - let state = get_shared_mutable_state(&self.x); - do (&(*state).lock).lock_cond |cond| { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data, - &Condvar {is_mutex: true, - failed: &mut (*state).failed, - cond: cond }) - } + let state = get_shared_mutable_state(&self.x); + do (&(*state).lock).lock_cond |cond| { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data, + &Condvar {is_mutex: true, + failed: &mut (*state).failed, + cond: cond }) } } } From 0c2ab662b71377efd2da7dced4f56e75cb68f540 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Apr 2013 19:38:49 -0400 Subject: [PATCH 3/3] Fixing some various warnings about unused imports --- src/libcore/rt/uvio.rs | 3 ++- src/libcore/str/ascii.rs | 1 + src/librustc/back/link.rs | 2 +- src/librustc/middle/trans/base.rs | 2 +- src/librustc/middle/ty.rs | 2 +- 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libcore/rt/uvio.rs b/src/libcore/rt/uvio.rs index 7bf1792baf49b..abdd8d6619a8a 100644 --- a/src/libcore/rt/uvio.rs +++ b/src/libcore/rt/uvio.rs @@ -11,7 +11,7 @@ use option::*; use result::*; -use super::io::net::ip::{IpAddr, Ipv4}; // n.b. Ipv4 is used only in tests +use super::io::net::ip::IpAddr; use super::uv::*; use super::rtio::*; use ops::Drop; @@ -19,6 +19,7 @@ use cell::{Cell, empty_cell}; use cast::transmute; use super::sched::{Scheduler, local_sched}; +#[cfg(test)] use super::io::net::ip::Ipv4; #[cfg(test)] use super::sched::Task; #[cfg(test)] use unstable::run_in_bare_thread; #[cfg(test)] use uint; diff --git a/src/libcore/str/ascii.rs b/src/libcore/str/ascii.rs index 339274ab47e4f..f6c0176eafc63 100644 --- a/src/libcore/str/ascii.rs +++ b/src/libcore/str/ascii.rs @@ -196,6 +196,7 @@ impl ToStrConsume for ~[Ascii] { } } +#[cfg(test)] mod tests { use super::*; diff --git a/src/librustc/back/link.rs b/src/librustc/back/link.rs index 99ea0bc0c865e..8befa4c82031a 100644 --- a/src/librustc/back/link.rs +++ b/src/librustc/back/link.rs @@ -26,7 +26,7 @@ use core::char; use core::hash::Streaming; use core::hash; use core::io::WriterUtil; -use core::libc::{c_int, c_uint, c_char}; +use core::libc::{c_int, c_uint}; use core::os::consts::{macos, freebsd, linux, android, win32}; use core::os; use core::ptr; diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index 68405f4fc5fe2..da87568b3d0c0 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -70,7 +70,7 @@ use core::hash; use core::hashmap::{HashMap, HashSet}; use core::int; use core::io; -use core::libc::{c_uint, c_ulonglong}; +use core::libc::c_uint; use core::uint; use std::time; use syntax::ast::ident; diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index b487fedf01dbb..47c9c3bdb6be4 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -514,7 +514,7 @@ pub struct substs { } mod primitives { - use super::{sty, t_box_}; + use super::t_box_; use syntax::ast;