From 9896beb5b5363eae1f8bfee35c12b3d78185e0e6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 23 Jan 2014 09:43:29 -0800 Subject: [PATCH 1/2] Implement an unused_result lint I attempted to implement the lint in two steps. My first attempt was a default-warn lint about *all* unused results. While this attempt did indeed find many possible bugs, I felt that the false-positive rate was too high to be turned on by default for all of Rust. My second attempt was to make unused-result a default-allow lint, but allow certain types to opt-in to the notion of "you must use this". For example, the Result type is now flagged with #[must_use]. This lint about "must use" types is warn by default (it's different from unused-result). The unused_must_use lint had a 100% hit rate in the compiler, but there's not that many places that return Result right now. I believe that this lint is a crucial step towards moving away from conditions for I/O (because all I/O will return Result by default). I'm worried that this lint is a little too specific to Result itself, but I believe that the false positive rate for the unused_result lint is too high to make it useful when turned on by default. --- src/librustc/middle/lint.rs | 80 +++++++++++++++++++++++--- src/test/compile-fail/unused-result.rs | 40 +++++++++++++ 2 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 src/test/compile-fail/unused-result.rs diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 8b139150f4ca1..42858416147e0 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -105,6 +105,9 @@ pub enum Lint { Experimental, Unstable, + UnusedMustUse, + UnusedResult, + Warnings, } @@ -356,12 +359,26 @@ static lint_table: &'static [(&'static str, LintSpec)] = &[ desc: "unknown features found in crate-level #[feature] directives", default: deny, }), - ("unknown_crate_type", - LintSpec { - lint: UnknownCrateType, - desc: "unknown crate type found in #[crate_type] directive", - default: deny, - }), + ("unknown_crate_type", + LintSpec { + lint: UnknownCrateType, + desc: "unknown crate type found in #[crate_type] directive", + default: deny, + }), + + ("unused_must_use", + LintSpec { + lint: UnusedMustUse, + desc: "unused result of an type flagged as #[must_use]", + default: warn, + }), + + ("unused_result", + LintSpec { + lint: UnusedResult, + desc: "unused result of an expression in a statement", + default: allow, + }), ]; /* @@ -934,7 +951,7 @@ static other_attrs: &'static [&'static str] = &[ "crate_map", "cfg", "doc", "export_name", "link_section", "no_freeze", "no_mangle", "no_send", "static_assert", "unsafe_no_drop_flag", "packed", "simd", "repr", "deriving", "unsafe_destructor", "link", "phase", - "macro_export", + "macro_export", "must_use", //mod-level "path", "link_name", "link_args", "nolink", "macro_escape", "no_implicit_prelude", @@ -1016,6 +1033,54 @@ fn check_path_statement(cx: &Context, s: &ast::Stmt) { } } +fn check_unused_result(cx: &Context, s: &ast::Stmt) { + let expr = match s.node { + ast::StmtSemi(expr, _) => expr, + _ => return + }; + let t = ty::expr_ty(cx.tcx, expr); + match ty::get(t).sty { + ty::ty_nil | ty::ty_bot | ty::ty_bool => return, + _ => {} + } + match expr.node { + ast::ExprRet(..) => return, + _ => {} + } + + let t = ty::expr_ty(cx.tcx, expr); + let mut warned = false; + match ty::get(t).sty { + ty::ty_struct(did, _) | + ty::ty_enum(did, _) => { + if ast_util::is_local(did) { + match cx.tcx.items.get(did.node) { + ast_map::NodeItem(it, _) => { + if attr::contains_name(it.attrs, "must_use") { + cx.span_lint(UnusedMustUse, s.span, + "unused result which must be used"); + warned = true; + } + } + _ => {} + } + } else { + csearch::get_item_attrs(cx.tcx.sess.cstore, did, |attrs| { + if attr::contains_name(attrs, "must_use") { + cx.span_lint(UnusedMustUse, s.span, + "unused result which must be used"); + warned = true; + } + }); + } + } + _ => {} + } + if !warned { + cx.span_lint(UnusedResult, s.span, "unused result"); + } +} + fn check_item_non_camel_case_types(cx: &Context, it: &ast::Item) { fn is_camel_case(cx: ty::ctxt, ident: ast::Ident) -> bool { let ident = cx.sess.str_of(ident); @@ -1478,6 +1543,7 @@ impl<'a> Visitor<()> for Context<'a> { fn visit_stmt(&mut self, s: &ast::Stmt, _: ()) { check_path_statement(self, s); + check_unused_result(self, s); visit::walk_stmt(self, s, ()); } diff --git a/src/test/compile-fail/unused-result.rs b/src/test/compile-fail/unused-result.rs new file mode 100644 index 0000000000000..eaf4d7d94c454 --- /dev/null +++ b/src/test/compile-fail/unused-result.rs @@ -0,0 +1,40 @@ +// Copyright 2014 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. + +#[deny(unused_result, unused_must_use)]; +#[allow(dead_code)]; + +#[must_use] +enum MustUse { Test } + +fn foo() -> T { fail!() } + +fn bar() -> int { return foo::(); } +fn baz() -> MustUse { return foo::(); } + +#[allow(unused_result)] +fn test() { + foo::(); + foo::(); //~ ERROR: unused result which must be used +} + +#[allow(unused_result, unused_must_use)] +fn test2() { + foo::(); + foo::(); +} + +fn main() { + foo::(); //~ ERROR: unused result + foo::(); //~ ERROR: unused result which must be used + + let _ = foo::(); + let _ = foo::(); +} From c13a62593c218243b4d3b0e8be0b903c0315571b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 23 Jan 2014 09:53:05 -0800 Subject: [PATCH 2/2] Flag Result as #[must_use] and deal with fallout. --- src/libnative/io/file.rs | 22 ++++++++++------------ src/libnative/io/process.rs | 2 +- src/libnative/io/timer_helper.rs | 2 +- src/libnative/io/timer_other.rs | 2 +- src/libnative/io/timer_timerfd.rs | 4 ++-- src/librustuv/file.rs | 2 +- src/libstd/io/fs.rs | 6 +++--- src/libstd/result.rs | 1 + 8 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/libnative/io/file.rs b/src/libnative/io/file.rs index 53386433d5361..acab7ce3a91b4 100644 --- a/src/libnative/io/file.rs +++ b/src/libnative/io/file.rs @@ -118,7 +118,10 @@ impl io::Reader for FileDesc { impl io::Writer for FileDesc { fn write(&mut self, buf: &[u8]) { - self.inner_write(buf); + match self.inner_write(buf) { + Ok(()) => {} + Err(e) => { io::io_error::cond.raise(e); } + } } } @@ -276,7 +279,7 @@ impl rtio::RtioFileStream for FileDesc { _ => Ok(()) } }; - self.seek(orig_pos as i64, io::SeekSet); + let _ = self.seek(orig_pos as i64, io::SeekSet); return ret; } #[cfg(unix)] @@ -383,12 +386,10 @@ impl rtio::RtioFileStream for CFile { } fn pread(&mut self, buf: &mut [u8], offset: u64) -> Result { - self.flush(); - self.fd.pread(buf, offset) + self.flush().and_then(|()| self.fd.pread(buf, offset)) } fn pwrite(&mut self, buf: &[u8], offset: u64) -> Result<(), IoError> { - self.flush(); - self.fd.pwrite(buf, offset) + self.flush().and_then(|()| self.fd.pwrite(buf, offset)) } fn seek(&mut self, pos: i64, style: io::SeekStyle) -> Result { let whence = match style { @@ -412,16 +413,13 @@ impl rtio::RtioFileStream for CFile { } } fn fsync(&mut self) -> Result<(), IoError> { - self.flush(); - self.fd.fsync() + self.flush().and_then(|()| self.fd.fsync()) } fn datasync(&mut self) -> Result<(), IoError> { - self.flush(); - self.fd.fsync() + self.flush().and_then(|()| self.fd.fsync()) } fn truncate(&mut self, offset: i64) -> Result<(), IoError> { - self.flush(); - self.fd.truncate(offset) + self.flush().and_then(|()| self.fd.truncate(offset)) } } diff --git a/src/libnative/io/process.rs b/src/libnative/io/process.rs index ee4ee29505575..13dd4298777c6 100644 --- a/src/libnative/io/process.rs +++ b/src/libnative/io/process.rs @@ -486,7 +486,7 @@ fn spawn_process_os(prog: &str, args: &[~str], (errno << 8) as u8, (errno << 0) as u8, ]; - output.inner_write(bytes); + assert!(output.inner_write(bytes).is_ok()); intrinsics::abort(); }) } diff --git a/src/libnative/io/timer_helper.rs b/src/libnative/io/timer_helper.rs index 3c20d073f2913..9297679857b0f 100644 --- a/src/libnative/io/timer_helper.rs +++ b/src/libnative/io/timer_helper.rs @@ -101,7 +101,7 @@ mod imp { } pub fn signal(fd: libc::c_int) { - FileDesc::new(fd, false).inner_write([0]); + FileDesc::new(fd, false).inner_write([0]).unwrap(); } pub fn close(fd: libc::c_int) { diff --git a/src/libnative/io/timer_other.rs b/src/libnative/io/timer_other.rs index 4a62a400c8da9..bc005f2fe8dd3 100644 --- a/src/libnative/io/timer_other.rs +++ b/src/libnative/io/timer_other.rs @@ -187,7 +187,7 @@ fn helper(input: libc::c_int, messages: Port) { // drain the file descriptor let mut buf = [0]; - fd.inner_read(buf); + fd.inner_read(buf).unwrap(); } -1 if os::errno() == libc::EINTR as int => {} diff --git a/src/libnative/io/timer_timerfd.rs b/src/libnative/io/timer_timerfd.rs index 2bcaf4d5c7c76..1888b8578a092 100644 --- a/src/libnative/io/timer_timerfd.rs +++ b/src/libnative/io/timer_timerfd.rs @@ -98,7 +98,7 @@ fn helper(input: libc::c_int, messages: Port) { if fd == input { let mut buf = [0, ..1]; // drain the input file descriptor of its input - FileDesc::new(fd, false).inner_read(buf); + FileDesc::new(fd, false).inner_read(buf).unwrap(); incoming = true; } else { let mut bits = [0, ..8]; @@ -106,7 +106,7 @@ fn helper(input: libc::c_int, messages: Port) { // // FIXME: should this perform a send() this number of // times? - FileDesc::new(fd, false).inner_read(bits); + FileDesc::new(fd, false).inner_read(bits).unwrap(); let remove = { match map.find(&fd).expect("fd unregistered") { &(ref c, oneshot) => !c.try_send(()) || oneshot diff --git a/src/librustuv/file.rs b/src/librustuv/file.rs index 8f7ced93fb0c6..31afa7b5c7c80 100644 --- a/src/librustuv/file.rs +++ b/src/librustuv/file.rs @@ -389,7 +389,7 @@ impl Drop for FileWatcher { } } rtio::CloseSynchronously => { - execute_nop(|req, cb| unsafe { + let _ = execute_nop(|req, cb| unsafe { uvll::uv_fs_close(self.loop_.handle, req, self.fd, cb) }); } diff --git a/src/libstd/io/fs.rs b/src/libstd/io/fs.rs index cb98ff21105bb..8904101dd05ed 100644 --- a/src/libstd/io/fs.rs +++ b/src/libstd/io/fs.rs @@ -175,7 +175,7 @@ impl File { /// /// This function will raise on the `io_error` condition on failure. pub fn fsync(&mut self) { - self.fd.fsync().map_err(|e| io_error::cond.raise(e)); + let _ = self.fd.fsync().map_err(|e| io_error::cond.raise(e)); } /// This function is similar to `fsync`, except that it may not synchronize @@ -187,7 +187,7 @@ impl File { /// /// This function will raise on the `io_error` condition on failure. pub fn datasync(&mut self) { - self.fd.datasync().map_err(|e| io_error::cond.raise(e)); + let _ = self.fd.datasync().map_err(|e| io_error::cond.raise(e)); } /// Either truncates or extends the underlying file, updating the size of @@ -203,7 +203,7 @@ impl File { /// /// On error, this function will raise on the `io_error` condition. pub fn truncate(&mut self, size: i64) { - self.fd.truncate(size).map_err(|e| io_error::cond.raise(e)); + let _ = self.fd.truncate(size).map_err(|e| io_error::cond.raise(e)); } /// Tests whether this stream has reached EOF. diff --git a/src/libstd/result.rs b/src/libstd/result.rs index 62049e8b4c99a..c3618bad18ec4 100644 --- a/src/libstd/result.rs +++ b/src/libstd/result.rs @@ -20,6 +20,7 @@ use to_str::ToStr; /// `Result` is a type that represents either success (`Ok`) or failure (`Err`). #[deriving(Clone, DeepClone, Eq, Ord, TotalEq, TotalOrd, ToStr)] +#[must_use] pub enum Result { /// Contains the success value Ok(T),