auto merge of #11754 : alexcrichton/rust/unused-result, r=brson

The general consensus is that we want to move away from conditions for I/O, and I propose a two-step plan for doing so:

1. Warn about unused `Result` types. When all of I/O returns `Result`, it will require you inspect the return value for an error *only if* you have a result you want to look at. By default, for things like `write` returning `Result<(), Error>`, these will all go silently ignored. This lint will prevent blind ignorance of these return values, letting you know that there's something you should do about them.

2. Implement a `try!` macro:

```
macro_rules! try( ($e:expr) => (match $e { Ok(e) => e, Err(e) => return Err(e) }) )
```

With these two tools combined, I feel that we get almost all the benefits of conditions. The first step (the lint) is a sanity check that you're not ignoring return values at callsites. The second step is to provide a convenience method of returning early out of a sequence of computations. After thinking about this for awhile, I don't think that we need the so-called "do-notation" in the compiler itself because I think it's just *too* specialized. Additionally, the `try!` macro is super lightweight, easy to understand, and works almost everywhere. As soon as you want to do something more fancy, my answer is "use match".

Basically, with these two tools in action, I would be comfortable removing conditions. What do others think about this strategy?

----

This PR specifically implements the `unused_result` lint. I actually added two lints, `unused_result` and `unused_must_use`, and the first commit has the rationale for why `unused_result` is turned off by default.

Author: bors

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<int, IoError> {
-        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<u64, IoError> {
         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))
     }
 }
 

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();
         })
     }

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) {

src/libnative/io/timer_other.rs

@@ -187,7 +187,7 @@ fn helper(input: libc::c_int, messages: Port<Req>) {
 
                 // 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 => {}

src/libnative/io/timer_timerfd.rs

@@ -98,15 +98,15 @@ fn helper(input: libc::c_int, messages: Port<Req>) {
             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];
                 // drain the timerfd of how many times its fired
                 //
                 // 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

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, ());
     }

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)
                 });
             }

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.

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<T, E> {
     /// Contains the success value
     Ok(T),

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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>() -> T { fail!() }
+
+fn bar() -> int { return foo::<int>(); }
+fn baz() -> MustUse { return foo::<MustUse>(); }
+
+#[allow(unused_result)]
+fn test() {
+    foo::<int>();
+    foo::<MustUse>(); //~ ERROR: unused result which must be used
+}
+
+#[allow(unused_result, unused_must_use)]
+fn test2() {
+    foo::<int>();
+    foo::<MustUse>();
+}
+
+fn main() {
+    foo::<int>(); //~ ERROR: unused result
+    foo::<MustUse>(); //~ ERROR: unused result which must be used
+
+    let _ = foo::<int>();
+    let _ = foo::<MustUse>();
+}