From 4e6d0731fd1fcb8e83c873c61d2a852f0e714b34 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Sun, 15 Feb 2015 10:34:30 +1100 Subject: [PATCH 1/2] lint: allow specific functions to be tagged `#[must_use]`. A function marked with `#[must_use]` behaves as if it returns a `#[must_use]` type, not matter what the actual return type is, e.g. #[must_use} fn foo() -> i32 { 1 } foo(); // error: unused result which must be used The #[must_use] lint is currently restricted to types; types that should *usually* not be ignored can be tagged with it to warn the user. This serves most purposes well, but it doesn't always work, since a function/method may be just acting as a go-between for a must-use type and a more general/flexible one, without doing anything significant (i.e. not really counting as a "use" of the must-use type). In this case, the function can be marked `#[must_use]` to ensure that its own result can be used, and avoid accidental ignoring of must-use types. --- src/librustc/lint/builtin.rs | 38 +++++++++++++++++++++++--- src/librustc/middle/ty.rs | 36 +++++++++++++++++++----- src/test/compile-fail/unused-result.rs | 29 ++++++++++++++++++++ 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index a415ff3ed7165..e0ca022d615aa 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -743,7 +743,7 @@ impl LintPass for PathStatements { declare_lint! { pub UNUSED_MUST_USE, Warn, - "unused result of a type flagged as #[must_use]" + "unused result of a type or function flagged as #[must_use]" } declare_lint! { @@ -772,9 +772,9 @@ impl LintPass for UnusedResults { let t = ty::expr_ty(cx.tcx, expr); let mut warned = false; + let mut is_unit = false; match t.sty { - ty::ty_tup(ref tys) if tys.is_empty() => return, - ty::ty_bool => return, + ty::ty_tup(ref tys) if tys.is_empty() => is_unit = true, ty::ty_struct(did, _) | ty::ty_enum(did, _) => { if ast_util::is_local(did) { @@ -788,7 +788,37 @@ impl LintPass for UnusedResults { } _ => {} } - if !warned { + match expr.node { + ast::ExprCall(ref func, _) => { + if let Some(func) = cx.tcx.def_map.borrow().get(&func.id) { + if let Some(attrs) = ty::get_attrs_opt(cx.tcx, func.def_id()) { + warned |= check_must_use(cx, &attrs, s.span) + } + } + } + ast::ExprMethodCall(..) => { + let method_call = ty::MethodCall::expr(expr.id); + let map = cx.tcx.method_map.borrow(); + let method = map.get(&method_call).unwrap(); + let did = match method.origin { + ty::MethodStatic(did) => Some(did), + ty::MethodStaticClosure(did) => Some(did), + // FIXME: it should be possible to get more info in this + // case, since e.g. the impl is sometimes known. + ty::MethodTypeParam(_) => None, + ty::MethodTraitObject(_) => None + }; + + if let Some(did) = did { + if let Some(attrs) = ty::get_attrs_opt(cx.tcx, did) { + warned |= check_must_use(cx, &attrs, s.span) + } + } + } + _ => {} + } + + if !warned && !is_unit { cx.span_lint(UNUSED_RESULTS, s.span, "unused result"); } diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 6026359ddace0..c82d0d8c75b97 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -5575,15 +5575,37 @@ pub fn predicates<'tcx>( vec } -/// Get the attributes of a definition. -pub fn get_attrs<'tcx>(tcx: &'tcx ctxt, did: DefId) - -> CowVec<'tcx, ast::Attribute> { - if is_local(did) { - let item = tcx.map.expect_item(did.node); - Cow::Borrowed(&item.attrs[]) +/// Get the attributes of a definition, returning `None` if `did` +/// refers to nothing or something that cannot have attributes. +pub fn get_attrs_opt<'tcx>(tcx: &'tcx ctxt, did: DefId) + -> Option> { + let attrs: CowVec<'tcx, ast::Attribute> = if is_local(did) { + match tcx.map.find(did.node) { + Some(ast_map::NodeItem(item)) => Cow::Borrowed(&item.attrs), + Some(ast_map::NodeForeignItem(item)) => Cow::Borrowed(&item.attrs), + Some(ast_map::NodeTraitItem(item)) => match *item { + ast::RequiredMethod(ref ty_meth) => Cow::Borrowed(&ty_meth.attrs), + ast::ProvidedMethod(ref meth) => Cow::Borrowed(&meth.attrs), + ast::TypeTraitItem(ref ty) => Cow::Borrowed(&ty.attrs), + }, + Some(ast_map::NodeImplItem(item)) => match *item { + ast::MethodImplItem(ref meth) => Cow::Borrowed(&meth.attrs), + ast::TypeImplItem(ref ty) => Cow::Borrowed(&ty.attrs), + }, + Some(ast_map::NodeVariant(variant)) => Cow::Borrowed(&variant.node.attrs), + _ => return None + } } else { Cow::Owned(csearch::get_item_attrs(&tcx.sess.cstore, did)) - } + }; + Some(attrs) +} + +/// Get the attributes of a definition. +pub fn get_attrs<'tcx>(tcx: &'tcx ctxt, did: DefId) -> CowVec<'tcx, ast::Attribute> { + get_attrs_opt(tcx, did).unwrap_or_else(|| { + tcx.sess.bug("get_attrs: DefId without attributes") + }) } /// Determine whether an item is annotated with an attribute diff --git a/src/test/compile-fail/unused-result.rs b/src/test/compile-fail/unused-result.rs index 6ed3b081c97fd..ee7cae240ad21 100644 --- a/src/test/compile-fail/unused-result.rs +++ b/src/test/compile-fail/unused-result.rs @@ -23,11 +23,28 @@ fn bar() -> isize { return foo::(); } fn baz() -> MustUse { return foo::(); } fn qux() -> MustUseMsg { return foo::(); } +#[must_use] +fn func() -> bool { true } +#[must_use = "some message"] +fn func_msg() -> i32 { 1 } + +impl MustUse { + #[must_use] + fn method(&self) -> f64 { 0.0 } + #[must_use = "some message"] + fn method_msg(&self) -> &str { "foo" } +} + #[allow(unused_results)] fn test() { foo::(); foo::(); //~ ERROR: unused result which must be used foo::(); //~ ERROR: unused result which must be used: some message + func(); //~ ERROR: unused result which must be used + func_msg(); //~ ERROR: unused result which must be used: some message + + MustUse::Test.method(); //~ ERROR: unused result which must be used + MustUse::Test.method_msg(); //~ ERROR: unused result which must be used: some message } #[allow(unused_results, unused_must_use)] @@ -35,14 +52,26 @@ fn test2() { foo::(); foo::(); foo::(); + func(); + func_msg(); + MustUse::Test.method(); + MustUse::Test.method_msg(); } fn main() { foo::(); //~ ERROR: unused result foo::(); //~ ERROR: unused result which must be used foo::(); //~ ERROR: unused result which must be used: some message + func(); //~ ERROR: unused result which must be used + func_msg(); //~ ERROR: unused result which must be used: some message + MustUse::Test.method(); //~ ERROR: unused result which must be used + MustUse::Test.method_msg(); //~ ERROR: unused result which must be used: some message let _ = foo::(); let _ = foo::(); let _ = foo::(); + let _ = func(); + let _ = func_msg(); + let _ = MustUse::Test.method(); + let _ = MustUse::Test.method_msg(); } From b9015bcd4228888c52704884c8d4f0b299b0b680 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Sun, 15 Feb 2015 10:45:25 +1100 Subject: [PATCH 2/2] Mark Result::{ok, err} #[must_use]. These functions are just adapters for `Result`, and something like `returns_result().ok()` is likely to be accidentally not handling the possibility of error as intended: `ok` does not *require* that the variant is `Ok`, and so errors are just silently ignord. Something like `returns_result().ok().unwrap()` is more likely to be what was desired. Similarly for `err`. Users who do wish to just ignore the result can use `let _ = returns_result();` or `drop(returns_result());` instead of calling one of these (effectively) no-op methods. --- src/libcore/result.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libcore/result.rs b/src/libcore/result.rs index d610962f8620c..0481403f8ed1d 100644 --- a/src/libcore/result.rs +++ b/src/libcore/result.rs @@ -319,6 +319,8 @@ impl Result { /// ``` #[inline] #[stable(feature = "rust1", since = "1.0.0")] + #[must_use = "`ok` only discards the contents of the `Err` variant, \ + it does not emit an error if the `Result` is not `Ok`"] pub fn ok(self) -> Option { match self { Ok(x) => Some(x), @@ -342,6 +344,8 @@ impl Result { /// ``` #[inline] #[stable(feature = "rust1", since = "1.0.0")] + #[must_use = "`err` only discards the contents of the `Ok` variant, \ + it does not emit an error if the `Result` is not `Err`"] pub fn err(self) -> Option { match self { Ok(_) => None,