auto merge of #18870 : barosl/rust/os-ioresult, r=alexcrichton

Make old-fashioned functions in the `std::os` module utilize `IoResult`.

I'm still investigating the possibility to include more functions in this pull request. Currently, it covers `getcwd()`, `make_absolute()`, and `change_dir()`. The issues covered by this PR are #16946 and #16315.

A few concerns:

- Should we provide `OsError` in distinction from `IoError`? I'm saying this because in Python, those two are distinguished. One advantage that we keep using `IoError` is that we can make the error cascade down other functions whose return type also includes `IoError`. An example of such functions is `std::io::TempDir::new_in()`, which uses `os::make_absolute()` as well as returns `IoResult<TempDir>`.
- `os::getcwd()` uses an internal buffer whose size is 2048 bytes, which is passed to `getcwd(3)`. There is no upper limitation of file paths in the POSIX standard, but typically it is set to 4096 bytes such as in Linux. Should we increase the buffer size? One thing that makes me nervous is that the size of 2048 bytes already seems a bit excessive, thinking that in normal cases, there would be no filenames that even exceeds 512 bytes.

Fixes #16946.
Fixes #16315.

Any ideas are welcomed. Thanks!

Author: bors

src/librustc/metadata/filesearch.rs

@@ -219,7 +219,7 @@ pub fn rust_path() -> Vec<Path> {
         }
         None => Vec::new()
     };
-    let mut cwd = os::getcwd();
+    let mut cwd = os::getcwd().unwrap();
     // now add in default entries
     let cwd_dot_rust = cwd.join(".rust");
     if !env_rust_path.contains(&cwd_dot_rust) {

src/librustc/plugin/load.rs

@@ -134,7 +134,7 @@ impl<'a> PluginLoader<'a> {
     // Dynamically link a registrar function into the compiler process.
     fn dylink_registrar(&mut self, vi: &ast::ViewItem, path: Path, symbol: String) {
         // Make sure the path contains a / or the linker will search for it.
-        let path = os::make_absolute(&path);
+        let path = os::make_absolute(&path).unwrap();
 
         let lib = match DynamicLibrary::open(Some(&path)) {
             Ok(lib) => lib,

src/librustc/session/mod.rs

@@ -231,7 +231,7 @@ pub fn build_session_(sopts: config::Options,
         if path.is_absolute() {
             path.clone()
         } else {
-            os::getcwd().join(&path)
+            os::getcwd().unwrap().join(&path)
         }
     );
 
@@ -246,7 +246,7 @@ pub fn build_session_(sopts: config::Options,
         plugin_registrar_fn: Cell::new(None),
         default_sysroot: default_sysroot,
         local_crate_source_file: local_crate_source_file,
-        working_dir: os::getcwd(),
+        working_dir: os::getcwd().unwrap(),
         lint_store: RefCell::new(lint::LintStore::new()),
         lints: RefCell::new(NodeMap::new()),
         crate_types: RefCell::new(Vec::new()),

src/librustc_back/archive.rs

@@ -229,7 +229,7 @@ impl<'a> ArchiveBuilder<'a> {
     pub fn build(self) -> Archive<'a> {
         // Get an absolute path to the destination, so `ar` will work even
         // though we run it from `self.work_dir`.
-        let abs_dst = os::getcwd().join(&self.archive.dst);
+        let abs_dst = os::getcwd().unwrap().join(&self.archive.dst);
         assert!(!abs_dst.is_relative());
         let mut args = vec![&abs_dst];
         let mut total_len = abs_dst.as_vec().len();
@@ -286,7 +286,7 @@ impl<'a> ArchiveBuilder<'a> {
         // First, extract the contents of the archive to a temporary directory.
         // We don't unpack directly into `self.work_dir` due to the possibility
         // of filename collisions.
-        let archive = os::make_absolute(archive);
+        let archive = os::make_absolute(archive).unwrap();
         run_ar(self.archive.handler, &self.archive.maybe_ar_prog,
                "x", Some(loc.path()), &[&archive]);
 

src/librustc_back/fs.rs

@@ -16,7 +16,7 @@ use std::os;
 /// returned path does not contain any symlinks in its hierarchy.
 pub fn realpath(original: &Path) -> io::IoResult<Path> {
     static MAX_LINKS_FOLLOWED: uint = 256;
-    let original = os::make_absolute(original);
+    let original = os::make_absolute(original).unwrap();
 
     // Right now lstat on windows doesn't work quite well
     if cfg!(windows) {

src/librustc_back/rpath.rs

@@ -102,9 +102,9 @@ fn get_rpath_relative_to_output(config: &mut RPathConfig,
         "$ORIGIN"
     };
 
-    let mut lib = (config.realpath)(&os::make_absolute(lib)).unwrap();
+    let mut lib = (config.realpath)(&os::make_absolute(lib).unwrap()).unwrap();
     lib.pop();
-    let mut output = (config.realpath)(&os::make_absolute(&config.out_filename)).unwrap();
+    let mut output = (config.realpath)(&os::make_absolute(&config.out_filename).unwrap()).unwrap();
     output.pop();
     let relative = lib.path_relative_from(&output);
     let relative = relative.expect("could not create rpath relative to output");
@@ -116,7 +116,7 @@ fn get_rpath_relative_to_output(config: &mut RPathConfig,
 
 fn get_install_prefix_rpath(config: RPathConfig) -> String {
     let path = (config.get_install_prefix_lib_path)();
-    let path = os::make_absolute(&path);
+    let path = os::make_absolute(&path).unwrap();
     // FIXME (#9639): This needs to handle non-utf8 paths
     path.as_str().expect("non-utf8 component in rpath").to_string()
 }

src/libstd/io/process.rs

@@ -974,7 +974,7 @@ mod tests {
         let prog = pwd_cmd().spawn().unwrap();
 
         let output = String::from_utf8(prog.wait_with_output().unwrap().output).unwrap();
-        let parent_dir = os::getcwd();
+        let parent_dir = os::getcwd().unwrap();
         let child_dir = Path::new(output.as_slice().trim());
 
         let parent_stat = parent_dir.stat().unwrap();
@@ -989,7 +989,7 @@ mod tests {
         use os;
         // test changing to the parent of os::getcwd() because we know
         // the path exists (and os::getcwd() is not expected to be root)
-        let parent_dir = os::getcwd().dir_path();
+        let parent_dir = os::getcwd().unwrap().dir_path();
         let prog = pwd_cmd().cwd(&parent_dir).spawn().unwrap();
 
         let output = String::from_utf8(prog.wait_with_output().unwrap().output).unwrap();

src/libstd/io/tempfile.rs

@@ -35,7 +35,8 @@ impl TempDir {
     /// If no directory can be created, `Err` is returned.
     pub fn new_in(tmpdir: &Path, suffix: &str) -> IoResult<TempDir> {
         if !tmpdir.is_absolute() {
-            return TempDir::new_in(&os::make_absolute(tmpdir), suffix);
+            let abs_tmpdir = try!(os::make_absolute(tmpdir));
+            return TempDir::new_in(&abs_tmpdir, suffix);
         }
 
         static CNT: atomic::AtomicUint = atomic::INIT_ATOMIC_UINT;

src/libstd/io/test.rs

@@ -75,7 +75,7 @@ fn base_port() -> u16 {
     ];
 
     // FIXME (#9639): This needs to handle non-utf8 paths
-    let path = os::getcwd();
+    let path = os::getcwd().unwrap();
     let path_s = path.as_str().unwrap();
 
     let mut final_base = base;

src/libstd/os.rs

@@ -38,7 +38,7 @@ pub use self::MapError::*;
 use clone::Clone;
 use error::{FromError, Error};
 use fmt;
-use io::IoResult;
+use io::{IoResult, IoError};
 use iter::Iterator;
 use libc::{c_void, c_int};
 use libc;
@@ -76,72 +76,83 @@ pub fn num_cpus() -> uint {
 pub const TMPBUF_SZ : uint = 1000u;
 const BUF_BYTES : uint = 2048u;
 
-/// Returns the current working directory as a Path.
+/// Returns the current working directory as a `Path`.
 ///
-/// # Failure
+/// # Errors
 ///
-/// Fails if the current working directory value is invalid:
+/// Returns an `Err` if the current working directory value is invalid.
 /// Possible cases:
 ///
 /// * Current directory does not exist.
 /// * There are insufficient permissions to access the current directory.
+/// * The internal buffer is not large enough to hold the path.
 ///
 /// # Example
 ///
 /// ```rust
 /// use std::os;
 ///
 /// // We assume that we are in a valid directory like "/home".
-/// let current_working_directory = os::getcwd();
+/// let current_working_directory = os::getcwd().unwrap();
 /// println!("The current directory is {}", current_working_directory.display());
 /// // /home
 /// ```
 #[cfg(unix)]
-pub fn getcwd() -> Path {
+pub fn getcwd() -> IoResult<Path> {
     use c_str::CString;
 
     let mut buf = [0 as c_char, ..BUF_BYTES];
     unsafe {
         if libc::getcwd(buf.as_mut_ptr(), buf.len() as libc::size_t).is_null() {
-            panic!()
+            Err(IoError::last_error())
+        } else {
+            Ok(Path::new(CString::new(buf.as_ptr(), false)))
         }
-        Path::new(CString::new(buf.as_ptr(), false))
     }
 }
 
-/// Returns the current working directory as a Path.
+/// Returns the current working directory as a `Path`.
 ///
-/// # Failure
+/// # Errors
 ///
-/// Fails if the current working directory value is invalid.
-/// Possibles cases:
+/// Returns an `Err` if the current working directory value is invalid.
+/// Possible cases:
 ///
 /// * Current directory does not exist.
 /// * There are insufficient permissions to access the current directory.
+/// * The internal buffer is not large enough to hold the path.
 ///
 /// # Example
 ///
 /// ```rust
 /// use std::os;
 ///
 /// // We assume that we are in a valid directory like "C:\\Windows".
-/// let current_working_directory = os::getcwd();
+/// let current_working_directory = os::getcwd().unwrap();
 /// println!("The current directory is {}", current_working_directory.display());
 /// // C:\\Windows
 /// ```
 #[cfg(windows)]
-pub fn getcwd() -> Path {
+pub fn getcwd() -> IoResult<Path> {
     use libc::DWORD;
     use libc::GetCurrentDirectoryW;
+    use io::OtherIoError;
 
     let mut buf = [0 as u16, ..BUF_BYTES];
     unsafe {
         if libc::GetCurrentDirectoryW(buf.len() as DWORD, buf.as_mut_ptr()) == 0 as DWORD {
-            panic!();
+            return Err(IoError::last_error());
         }
     }
-    Path::new(String::from_utf16(::str::truncate_utf16_at_nul(&buf))
-              .expect("GetCurrentDirectoryW returned invalid UTF-16"))
+
+    match String::from_utf16(::str::truncate_utf16_at_nul(&buf)) {
+        Some(ref cwd) => Ok(Path::new(cwd)),
+        None => Err(IoError {
+            kind: OtherIoError,
+            desc: "GetCurrentDirectoryW returned invalid UTF-16",
+            detail: None,
+        }),
+    }
 }
 
 #[cfg(windows)]
@@ -411,7 +422,9 @@ pub fn setenv<T: BytesContainer>(n: &str, v: T) {
             with_env_lock(|| {
                 n.with_c_str(|nbuf| {
                     v.with_c_str(|vbuf| {
-                        libc::funcs::posix01::unistd::setenv(nbuf, vbuf, 1);
+                        if libc::funcs::posix01::unistd::setenv(nbuf, vbuf, 1) != 0 {
+                            panic!(IoError::last_error());
+                        }
                     })
                 })
             })
@@ -427,7 +440,9 @@ pub fn setenv<T: BytesContainer>(n: &str, v: T) {
 
         unsafe {
             with_env_lock(|| {
-                libc::SetEnvironmentVariableW(n.as_ptr(), v.as_ptr());
+                if libc::SetEnvironmentVariableW(n.as_ptr(), v.as_ptr()) == 0 {
+                    panic!(IoError::last_error());
+                }
             })
         }
     }
@@ -442,7 +457,9 @@ pub fn unsetenv(n: &str) {
         unsafe {
             with_env_lock(|| {
                 n.with_c_str(|nbuf| {
-                    libc::funcs::posix01::unistd::unsetenv(nbuf);
+                    if libc::funcs::posix01::unistd::unsetenv(nbuf) != 0 {
+                        panic!(IoError::last_error());
+                    }
                 })
             })
         }
@@ -454,11 +471,14 @@ pub fn unsetenv(n: &str) {
         n.push(0);
         unsafe {
             with_env_lock(|| {
-                libc::SetEnvironmentVariableW(n.as_ptr(), ptr::null());
+                if libc::SetEnvironmentVariableW(n.as_ptr(), ptr::null()) == 0 {
+                    panic!(IoError::last_error());
+                }
             })
         }
     }
-    _unsetenv(n);
+
+    _unsetenv(n)
 }
 
 /// Parses input according to platform conventions for the `PATH`
@@ -829,20 +849,21 @@ pub fn tmpdir() -> Path {
 ///
 /// // Assume we're in a path like /home/someuser
 /// let rel_path = Path::new("..");
-/// let abs_path = os::make_absolute(&rel_path);
+/// let abs_path = os::make_absolute(&rel_path).unwrap();
 /// println!("The absolute path is {}", abs_path.display());
 /// // Prints "The absolute path is /home"
 /// ```
 // NB: this is here rather than in path because it is a form of environment
 // querying; what it does depends on the process working directory, not just
 // the input paths.
-pub fn make_absolute(p: &Path) -> Path {
+pub fn make_absolute(p: &Path) -> IoResult<Path> {
     if p.is_absolute() {
-        p.clone()
+        Ok(p.clone())
     } else {
-        let mut ret = getcwd();
-        ret.push(p);
-        ret
+        getcwd().map(|mut cwd| {
+            cwd.push(p);
+            cwd
+        })
     }
 }
 
@@ -855,32 +876,33 @@ pub fn make_absolute(p: &Path) -> Path {
 /// use std::path::Path;
 ///
 /// let root = Path::new("/");
-/// assert!(os::change_dir(&root));
+/// assert!(os::change_dir(&root).is_ok());
 /// println!("Successfully changed working directory to {}!", root.display());
 /// ```
-pub fn change_dir(p: &Path) -> bool {
+pub fn change_dir(p: &Path) -> IoResult<()> {
     return chdir(p);
 
     #[cfg(windows)]
-    fn chdir(p: &Path) -> bool {
-        let p = match p.as_str() {
-            Some(s) => {
-                let mut p = s.utf16_units().collect::<Vec<u16>>();
-                p.push(0);
-                p
-            }
-            None => return false,
-        };
+    fn chdir(p: &Path) -> IoResult<()> {
+        let mut p = p.as_str().unwrap().utf16_units().collect::<Vec<u16>>();
+        p.push(0);
+
         unsafe {
-            libc::SetCurrentDirectoryW(p.as_ptr()) != (0 as libc::BOOL)
+            match libc::SetCurrentDirectoryW(p.as_ptr()) != (0 as libc::BOOL) {
+                true => Ok(()),
+                false => Err(IoError::last_error()),
+            }
         }
     }
 
     #[cfg(unix)]
-    fn chdir(p: &Path) -> bool {
+    fn chdir(p: &Path) -> IoResult<()> {
         p.with_c_str(|buf| {
             unsafe {
-                libc::chdir(buf) == (0 as c_int)
+                match libc::chdir(buf) == (0 as c_int) {
+                    true => Ok(()),
+                    false => Err(IoError::last_error()),
+                }
             }
         })
     }
@@ -1881,11 +1903,11 @@ mod tests {
     fn test() {
         assert!((!Path::new("test-path").is_absolute()));
 
-        let cwd = getcwd();
+        let cwd = getcwd().unwrap();
         debug!("Current working directory: {}", cwd.display());
 
-        debug!("{}", make_absolute(&Path::new("test-path")).display());
-        debug!("{}", make_absolute(&Path::new("/usr/bin")).display());
+        debug!("{}", make_absolute(&Path::new("test-path")).unwrap().display());
+        debug!("{}", make_absolute(&Path::new("/usr/bin")).unwrap().display());
     }
 
     #[test]

src/test/run-pass/process-spawn-with-unicode-params.rs

@@ -26,7 +26,7 @@ use std::path::Path;
 
 fn main() {
     let my_args = os::args();
-    let my_cwd  = os::getcwd();
+    let my_cwd  = os::getcwd().unwrap();
     let my_env  = os::env();
     let my_path = Path::new(os::self_exe_name().unwrap());
     let my_dir  = my_path.dir_path();