diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 721bb9962624..116b70bf7e32 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -33,6 +33,7 @@ use util::ppaux::ty_to_str; use core::hashmap::HashSet; use core::uint; +use core::util::with; use syntax::ast::m_mutbl; use syntax::ast; use syntax::ast_util; @@ -40,13 +41,18 @@ 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, reported: HashSet, - declared_purity: @mut ast::purity, + declared_purity: @mut PurityState, fn_args: @mut @~[ast::node_id] } @@ -62,6 +68,16 @@ enum purity_cause { pc_cmt(bckerr) } +// if we're not pure, why? +#[deriving(Eq)] +enum impurity_cause { + // some surrounding block was marked as 'unsafe' + pc_unsafe, + + // nothing was unsafe, and nothing was pure + pc_default, +} + pub fn check_loans(bccx: @BorrowckCtxt, +req_maps: ReqMaps, crate: @ast::crate) { @@ -69,7 +85,8 @@ pub fn check_loans(bccx: @BorrowckCtxt, bccx: bccx, req_maps: req_maps, reported: HashSet::new(), - declared_purity: @mut ast::impure_fn, + declared_purity: @mut PurityState { purity: ast::impure_fn, + def: 0 }, fn_args: @mut @~[] }; let vt = visit::mk_vt(@visit::Visitor {visit_expr: check_loans_in_expr, @@ -106,16 +123,18 @@ pub impl assignment_type { pub impl CheckLoanCtxt { fn tcx(&self) -> ty::ctxt { self.bccx.tcx } - fn purity(&mut self, scope_id: ast::node_id) -> Option { - let default_purity = match *self.declared_purity { + fn purity(&mut self, scope_id: ast::node_id) + -> Either + { + let default_purity = match self.declared_purity.purity { // an unsafe declaration overrides all - ast::unsafe_fn => return None, + ast::unsafe_fn => return Right(pc_unsafe), // otherwise, remember what was declared as the // default, but we must scan for requirements // imposed by the borrow check - ast::pure_fn => Some(pc_pure_fn), - ast::extern_fn | ast::impure_fn => None + ast::pure_fn => Left(pc_pure_fn), + ast::extern_fn | ast::impure_fn => Right(pc_default) }; // scan to see if this scope or any enclosing scope requires @@ -125,7 +144,7 @@ pub impl CheckLoanCtxt { loop { match self.req_maps.pure_map.find(&scope_id) { None => (), - Some(e) => return Some(pc_cmt(*e)) + Some(e) => return Left(pc_cmt(*e)) } match self.tcx().region_maps.opt_encl_scope(scope_id) { @@ -171,7 +190,7 @@ pub impl CheckLoanCtxt { // overloaded operators the callee has an id but no expr. // annoying. fn check_pure_callee_or_arg(&mut self, - pc: purity_cause, + pc: Either, opt_expr: Option<@ast::expr>, callee_id: ast::node_id, callee_span: span) { @@ -196,7 +215,7 @@ pub impl CheckLoanCtxt { match opt_expr { Some(expr) => { match expr.node { - ast::expr_path(_) if pc == pc_pure_fn => { + ast::expr_path(_) if pc == Left(pc_pure_fn) => { let def = *self.tcx().def_map.get(&expr.id); let did = ast_util::def_id_of_def(def); let is_fn_arg = @@ -361,10 +380,10 @@ pub impl CheckLoanCtxt { // if this is a pure function, only loan-able state can be // assigned, because it is uniquely tied to this function and // is not visible from the outside - match self.purity(ex.id) { - None => (), - Some(pc_cmt(_)) => { - let purity = self.purity(ex.id).get(); + let purity = self.purity(ex.id); + match purity { + Right(_) => (), + Left(pc_cmt(_)) => { // Subtle: Issue #3162. If we are enforcing purity // because there is a reference to aliasable, mutable data // that we require to be immutable, we can't allow writes @@ -376,10 +395,10 @@ pub impl CheckLoanCtxt { ex.span, at.ing_form(self.bccx.cmt_to_str(cmt))); } - Some(pc_pure_fn) => { + Left(pc_pure_fn) => { if cmt.lp.is_none() { self.report_purity_error( - pc_pure_fn, ex.span, + purity, ex.span, at.ing_form(self.bccx.cmt_to_str(cmt))); } } @@ -462,14 +481,23 @@ pub impl CheckLoanCtxt { } } - fn report_purity_error(&mut self, pc: purity_cause, sp: span, msg: ~str) { + fn report_purity_error(&mut self, pc: Either, + sp: span, msg: ~str) { match pc { - pc_pure_fn => { + Right(pc_default) => { fail!(~"pc_default should be filtered sooner") } + Right(pc_unsafe) => { + // this error was prevented by being marked as unsafe, so flag the + // definition as having contributed to the validity of the program + let def = self.declared_purity.def; + debug!("flagging %? as a used unsafe source", def); + self.tcx().used_unsafe.insert(def); + } + Left(pc_pure_fn) => { self.tcx().sess.span_err( sp, fmt!("%s prohibited in pure context", msg)); } - pc_cmt(ref e) => { + Left(pc_cmt(ref e)) => { if self.reported.insert((*e).cmt.id) { self.tcx().sess.span_err( (*e).cmt.span, @@ -556,16 +584,32 @@ pub impl CheckLoanCtxt { callee_id: ast::node_id, callee_span: span, args: &[@ast::expr]) { - match self.purity(expr.id) { - None => {} - Some(ref pc) => { - self.check_pure_callee_or_arg( - (*pc), callee, callee_id, callee_span); - for args.each |arg| { - self.check_pure_callee_or_arg( - (*pc), Some(*arg), arg.id, arg.span); + let pc = self.purity(expr.id); + match pc { + // no purity, no need to check for anything + Right(pc_default) => return, + + // some form of purity, definitely need to check + Left(_) => (), + + // Unsafe trumped. To see if the unsafe is necessary, see what the + // purity would have been without a trump, and if it's some form + // of purity then we need to go ahead with the check + Right(pc_unsafe) => { + match do with(&mut self.declared_purity.purity, + ast::impure_fn) { self.purity(expr.id) } { + Right(pc_unsafe) => fail!(~"unsafe can't trump twice"), + Right(pc_default) => return, + Left(_) => () + } } - } + + } + self.check_pure_callee_or_arg( + pc, callee, callee_id, callee_span); + for args.each |arg| { + self.check_pure_callee_or_arg( + pc, Some(*arg), arg.id, arg.span); } } } @@ -580,27 +624,32 @@ fn check_loans_in_fn(fk: &visit::fn_kind, let is_stack_closure = self.is_stack_closure(id); let fty = ty::node_id_to_type(self.tcx(), id); - let declared_purity; + let declared_purity, src; match *fk { visit::fk_item_fn(*) | visit::fk_method(*) | visit::fk_dtor(*) => { declared_purity = ty::ty_fn_purity(fty); + src = id; } visit::fk_anon(*) | visit::fk_fn_block(*) => { let fty_sigil = ty::ty_closure_sigil(fty); check_moves_from_captured_variables(self, id, fty_sigil); - declared_purity = ty::determine_inherited_purity( - *self.declared_purity, - ty::ty_fn_purity(fty), + let pair = ty::determine_inherited_purity( + (self.declared_purity.purity, self.declared_purity.def), + (ty::ty_fn_purity(fty), id), fty_sigil); + declared_purity = pair.first(); + src = pair.second(); } } 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 = declared_purity; + self.declared_purity = @mut PurityState { + purity: declared_purity, def: src + }; match *fk { visit::fk_anon(*) | @@ -754,7 +803,10 @@ fn check_loans_in_block(blk: &ast::blk, ast::default_blk => { } ast::unsafe_blk => { - *self.declared_purity = ast::unsafe_fn; + *self.declared_purity = PurityState { + purity: ast::unsafe_fn, + def: blk.node.id, + }; } } diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 6f8992bf1ca4..876ed76f9874 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -75,6 +75,7 @@ pub enum lint { default_methods, deprecated_mutable_fields, deprecated_drop, + unused_unsafe, foreign_mode, managed_heap_memory, @@ -256,6 +257,13 @@ pub fn get_lint_dict() -> LintDict { default: deny }), + (~"unused_unsafe", + LintSpec { + lint: unused_unsafe, + desc: "unnecessary use of an \"unsafe\" block or function", + default: warn + }), + (~"unused_variable", LintSpec { lint: unused_variable, @@ -490,6 +498,7 @@ fn check_item(i: @ast::item, cx: ty::ctxt) { check_item_default_methods(cx, i); check_item_deprecated_mutable_fields(cx, i); check_item_deprecated_drop(cx, i); + check_item_unused_unsafe(cx, i); } // Take a visitor, and modify it so that it will not proceed past subitems. @@ -923,19 +932,55 @@ fn check_item_non_camel_case_types(cx: ty::ctxt, it: @ast::item) { } } +fn check_item_unused_unsafe(cx: ty::ctxt, it: @ast::item) { + let visit_expr: @fn(@ast::expr) = |e| { + match e.node { + ast::expr_block(ref blk) if blk.node.rules == ast::unsafe_blk => { + if !cx.used_unsafe.contains(&blk.node.id) { + cx.sess.span_lint(unused_unsafe, blk.node.id, it.id, + blk.span, + ~"unnecessary \"unsafe\" block"); + } + } + _ => () + } + }; + + let visit = item_stopping_visitor( + visit::mk_simple_visitor(@visit::SimpleVisitor { + visit_expr: visit_expr, + .. *visit::default_simple_visitor() + })); + visit::visit_item(it, (), visit); +} + fn check_fn(tcx: ty::ctxt, fk: &visit::fn_kind, decl: &ast::fn_decl, _body: &ast::blk, span: span, id: ast::node_id) { debug!("lint check_fn fk=%? id=%?", fk, id); - // don't complain about blocks, since they tend to get their modes - // specified from the outside + // Check for an 'unsafe fn' which doesn't need to be unsafe match *fk { - visit::fk_fn_block(*) => { return; } - _ => {} + visit::fk_item_fn(_, _, ast::unsafe_fn, _) => { + if !tcx.used_unsafe.contains(&id) { + tcx.sess.span_lint(unused_unsafe, id, id, span, + ~"unnecessary \"unsafe\" function"); + } + } + _ => () + } + + // Check for deprecated modes + match *fk { + // don't complain about blocks, since they tend to get their modes + // specified from the outside + visit::fk_fn_block(*) => {} + + _ => { + let fn_ty = ty::node_id_to_type(tcx, id); + check_fn_deprecated_modes(tcx, fn_ty, decl, span, id); + } } - let fn_ty = ty::node_id_to_type(tcx, id); - check_fn_deprecated_modes(tcx, fn_ty, decl, span, id); } fn check_fn_deprecated_modes(tcx: ty::ctxt, fn_ty: ty::t, decl: &ast::fn_decl, diff --git a/src/librustc/middle/resolve.rs b/src/librustc/middle/resolve.rs index fa902b7bf56a..c9381757d77d 100644 --- a/src/librustc/middle/resolve.rs +++ b/src/librustc/middle/resolve.rs @@ -5212,17 +5212,11 @@ pub impl Resolver { import_resolution.span != dummy_sp() && import_resolution.privacy != Public { import_resolution.state.warned = true; - match self.unused_import_lint_level(module_) { - warn => { - self.session.span_warn(copy import_resolution.span, - ~"unused import"); - } - deny | forbid => { - self.session.span_err(copy import_resolution.span, - ~"unused import"); - } - allow => () - } + let span = import_resolution.span; + self.session.span_lint_level( + self.unused_import_lint_level(module_), + span, + ~"unused import"); } } } diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index a26c1c1e7663..f62e366ebdca 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -300,7 +300,11 @@ struct ctxt_ { destructors: @mut HashSet, // Maps a trait onto a mapping from self-ty to impl - trait_impls: @mut HashMap> + trait_impls: @mut HashMap>, + + // Set of used unsafe nodes (functions or blocks). Unsafe nodes not + // present in this set can be warned about. + used_unsafe: @mut HashSet, } enum tbox_flag { @@ -885,7 +889,8 @@ pub fn mk_ctxt(s: session::Session, supertraits: @mut HashMap::new(), destructor_for_type: @mut HashMap::new(), destructors: @mut HashSet::new(), - trait_impls: @mut HashMap::new() + trait_impls: @mut HashMap::new(), + used_unsafe: @mut HashSet::new(), } } @@ -4309,16 +4314,16 @@ pub fn eval_repeat_count(tcx: ctxt, count_expr: @ast::expr) -> uint { } // Determine what purity to check a nested function under -pub fn determine_inherited_purity(parent_purity: ast::purity, - child_purity: ast::purity, - child_sigil: ast::Sigil) - -> ast::purity { +pub fn determine_inherited_purity(parent: (ast::purity, ast::node_id), + child: (ast::purity, ast::node_id), + child_sigil: ast::Sigil) + -> (ast::purity, ast::node_id) { // If the closure is a stack closure and hasn't had some non-standard // purity inferred for it, then check it under its parent's purity. // Otherwise, use its own match child_sigil { - ast::BorrowedSigil if child_purity == ast::impure_fn => parent_purity, - _ => child_purity + ast::BorrowedSigil if child.first() == ast::impure_fn => parent, + _ => child } } diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index 2d1c940103a7..5ec4c233bc0d 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/src/librustc/middle/typeck/check/mod.rs @@ -179,6 +179,11 @@ pub enum FnKind { Vanilla } +struct PurityState { + purity: ast::purity, + from: ast::node_id, +} + pub struct FnCtxt { // var_bindings, locals and next_var_id are shared // with any nested functions that capture the environment @@ -187,7 +192,7 @@ pub struct FnCtxt { ret_ty: ty::t, // Used by loop bodies that return from the outer function indirect_ret_ty: Option, - purity: ast::purity, + ps: PurityState, // Sometimes we generate region pointers where the precise region // to use is not known. For example, an expression like `&x.f` @@ -238,7 +243,7 @@ pub fn blank_fn_ctxt(ccx: @mut CrateCtxt, @mut FnCtxt { ret_ty: rty, indirect_ret_ty: None, - purity: ast::pure_fn, + ps: PurityState { purity: ast::pure_fn, from: 0 }, region_lb: region_bnd, in_scope_regions: @Nil, fn_kind: Vanilla, @@ -265,7 +270,7 @@ pub fn check_bare_fn(ccx: @mut CrateCtxt, ty::ty_bare_fn(ref fn_ty) => { let fcx = check_fn(ccx, self_info, fn_ty.purity, - &fn_ty.sig, decl, body, Vanilla, + &fn_ty.sig, decl, id, body, Vanilla, @Nil, blank_inherited(ccx));; vtable::resolve_in_block(fcx, body); @@ -282,6 +287,7 @@ pub fn check_fn(ccx: @mut CrateCtxt, purity: ast::purity, fn_sig: &ty::FnSig, decl: &ast::fn_decl, + id: ast::node_id, body: &ast::blk, fn_kind: FnKind, inherited_isr: isr_alist, @@ -342,7 +348,7 @@ pub fn check_fn(ccx: @mut CrateCtxt, @mut FnCtxt { ret_ty: ret_ty, indirect_ret_ty: indirect_ret_ty, - purity: purity, + ps: PurityState { purity: purity, from: id }, region_lb: body.node.id, in_scope_regions: isr, fn_kind: fn_kind, @@ -867,8 +873,12 @@ pub impl FnCtxt { } fn require_unsafe(&self, sp: span, op: ~str) { - match self.purity { - ast::unsafe_fn => {/*ok*/} + 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); + } _ => { self.ccx.tcx.sess.span_err( sp, @@ -1679,12 +1689,13 @@ pub fn check_expr_with_unifier(fcx: @mut FnCtxt, fcx.write_ty(expr.id, fty); - let inherited_purity = - ty::determine_inherited_purity(copy fcx.purity, purity, + let (inherited_purity, id) = + ty::determine_inherited_purity((fcx.ps.purity, fcx.ps.from), + (purity, expr.id), sigil); check_fn(fcx.ccx, None, inherited_purity, &fty_sig, - decl, body, fn_kind, fcx.in_scope_regions, fcx.inh); + decl, id, body, fn_kind, fcx.in_scope_regions, fcx.inh); } @@ -2923,8 +2934,11 @@ pub fn check_block_with_expected(fcx0: @mut FnCtxt, blk: &ast::blk, expected: Option) { let fcx = match blk.node.rules { - ast::unsafe_blk => @mut FnCtxt {purity: ast::unsafe_fn,.. copy *fcx0}, - ast::default_blk => fcx0 + ast::unsafe_blk => @mut FnCtxt { + ps: PurityState { purity: ast::unsafe_fn, from: blk.node.id }, + .. copy *fcx0 + }, + ast::default_blk => fcx0 }; do fcx.with_region_lb(blk.node.id) { let mut warned = false; diff --git a/src/test/compile-fail/unused-unsafe.rs b/src/test/compile-fail/unused-unsafe.rs new file mode 100644 index 000000000000..368a0fbe9bec --- /dev/null +++ b/src/test/compile-fail/unused-unsafe.rs @@ -0,0 +1,44 @@ +// Copyright 2013 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Exercise the unused_unsafe attribute in some positive and negative cases + +#[deny(unused_unsafe)]; + +use core::libc; + +fn callback(_f: &fn() -> T) -> T { fail!() } + +fn bad1() { unsafe {} } //~ ERROR: unnecessary "unsafe" block +fn bad2() { unsafe { bad1() } } //~ ERROR: unnecessary "unsafe" block +unsafe fn bad3() {} //~ ERROR: unnecessary "unsafe" function +unsafe fn bad4() { unsafe {} } //~ ERROR: unnecessary "unsafe" function + //~^ ERROR: unnecessary "unsafe" block +fn bad5() { unsafe { do callback {} } } //~ ERROR: unnecessary "unsafe" block + +unsafe fn good0() { libc::exit(1) } +fn good1() { unsafe { libc::exit(1) } } +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) } + + do callback { + what(); + } + } +} + +#[allow(unused_unsafe)] unsafe fn allowed0() {} +#[allow(unused_unsafe)] fn allowed1() { unsafe {} } + +fn main() { }