From 7d756e44a96c1e28f63cab1ea328d01984ac07d2 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 13 Jun 2014 16:03:41 -0700 Subject: [PATCH 1/4] rustrt: Reorganize task usage Most of the comments are available on the Task structure itself, but this commit is aimed at making FFI-style usage of Rust tasks a little nicer. Primarily, this commit enables re-use of tasks across multiple invocations. The method `run` will no longer unconditionally destroy the task itself. Rather, the task will be internally re-usable if the closure specified did not fail. Once a task has failed once it is considered poisoned and it can never be used again. Along the way I tried to document shortcomings of the current method of tearing down a task, opening a few issues as well. For now none of the behavior is a showstopper, but it's useful to acknowledge it. Also along the way I attempted to remove as much `unsafe` code as possible, opting for safer abstractions. --- src/libgreen/lib.rs | 2 +- src/libgreen/task.rs | 4 +- src/libnative/lib.rs | 5 +- src/libnative/task.rs | 3 +- src/librustrt/local_heap.rs | 24 +- src/librustrt/task.rs | 335 ++++++++++++++----- src/librustrt/unwind.rs | 14 - src/libstd/task.rs | 15 +- src/test/run-pass/fail-during-tld-destroy.rs | 53 +++ 9 files changed, 337 insertions(+), 118 deletions(-) create mode 100644 src/test/run-pass/fail-during-tld-destroy.rs diff --git a/src/libgreen/lib.rs b/src/libgreen/lib.rs index 007f63383ac1b..e0e9187a409c6 100644 --- a/src/libgreen/lib.rs +++ b/src/libgreen/lib.rs @@ -299,7 +299,7 @@ pub fn start(argc: int, argv: **u8, let mut ret = None; simple::task().run(|| { ret = Some(run(event_loop_factory, main.take_unwrap())); - }); + }).destroy(); // unsafe is ok b/c we're sure that the runtime is gone unsafe { rt::cleanup() } ret.unwrap() diff --git a/src/libgreen/task.rs b/src/libgreen/task.rs index 692b6e14fe7e1..25c2a95234192 100644 --- a/src/libgreen/task.rs +++ b/src/libgreen/task.rs @@ -110,7 +110,7 @@ extern fn bootstrap_green_task(task: uint, code: *(), env: *()) -> ! { // requested. This is the "try/catch" block for this green task and // is the wrapper for *all* code run in the task. let mut start = Some(start); - let task = task.swap().run(|| start.take_unwrap()()); + let task = task.swap().run(|| start.take_unwrap()()).destroy(); // Once the function has exited, it's time to run the termination // routine. This means we need to context switch one more time but @@ -120,7 +120,7 @@ extern fn bootstrap_green_task(task: uint, code: *(), env: *()) -> ! { // this we could add a `terminate` function to the `Runtime` trait // in libstd, but that seems less appropriate since the coversion // method exists. - GreenTask::convert(task).terminate() + GreenTask::convert(task).terminate(); } impl GreenTask { diff --git a/src/libnative/lib.rs b/src/libnative/lib.rs index 3438661ffb3e3..9b2bcbbdb0ee3 100644 --- a/src/libnative/lib.rs +++ b/src/libnative/lib.rs @@ -134,13 +134,12 @@ pub fn start(argc: int, argv: **u8, main: proc()) -> int { let mut main = Some(main); let mut task = task::new((my_stack_bottom, my_stack_top)); task.name = Some(str::Slice("
")); - let t = task.run(|| { + drop(task.run(|| { unsafe { rt::stack::record_stack_bounds(my_stack_bottom, my_stack_top); } exit_code = Some(run(main.take_unwrap())); - }); - drop(t); + }).destroy()); unsafe { rt::cleanup(); } // If the exit code wasn't set, then the task block must have failed. return exit_code.unwrap_or(rt::DEFAULT_ERROR_CODE); diff --git a/src/libnative/task.rs b/src/libnative/task.rs index 8b7c8e61bc35b..0b863d9f69401 100644 --- a/src/libnative/task.rs +++ b/src/libnative/task.rs @@ -92,8 +92,7 @@ pub fn spawn_opts(opts: TaskOpts, f: proc():Send) { let mut f = Some(f); let mut task = task; task.put_runtime(ops); - let t = task.run(|| { f.take_unwrap()() }); - drop(t); + drop(task.run(|| { f.take_unwrap()() }).destroy()); bookkeeping::decrement(); }) } diff --git a/src/librustrt/local_heap.rs b/src/librustrt/local_heap.rs index d09033e771cf7..d95a4ba49de9d 100644 --- a/src/librustrt/local_heap.rs +++ b/src/librustrt/local_heap.rs @@ -110,7 +110,12 @@ impl LocalHeap { self.memory_region.free(alloc); } - pub unsafe fn annihilate(&mut self) { + /// Immortalize all pending allocations, forcing them to live forever. + /// + /// This function will freeze all allocations to prevent all pending + /// allocations from being deallocated. This is used in preparation for when + /// a task is about to destroy TLD. + pub unsafe fn immortalize(&mut self) { let mut n_total_boxes = 0u; // Pass 1: Make all boxes immortal. @@ -122,6 +127,17 @@ impl LocalHeap { (*alloc).ref_count = RC_IMMORTAL; }); + if debug_mem() { + // We do logging here w/o allocation. + rterrln!("total boxes annihilated: {}", n_total_boxes); + } + } + + /// Continues deallocation of the all pending allocations in this arena. + /// + /// This is invoked from the destructor, and requires that `immortalize` has + /// been called previously. + unsafe fn annihilate(&mut self) { // Pass 2: Drop all boxes. // // In this pass, unique-managed boxes may get freed, but not @@ -142,11 +158,6 @@ impl LocalHeap { self.each_live_alloc(true, |me, alloc| { me.free(alloc); }); - - if debug_mem() { - // We do logging here w/o allocation. - rterrln!("total boxes annihilated: {}", n_total_boxes); - } } unsafe fn each_live_alloc(&mut self, read_next_before: bool, @@ -170,6 +181,7 @@ impl LocalHeap { impl Drop for LocalHeap { fn drop(&mut self) { + unsafe { self.annihilate() } assert!(self.live_allocs.is_null()); } } diff --git a/src/librustrt/task.rs b/src/librustrt/task.rs index ce43f7858b64a..6e27310f09ac9 100644 --- a/src/librustrt/task.rs +++ b/src/librustrt/task.rs @@ -19,8 +19,8 @@ use alloc::arc::Arc; use alloc::owned::{AnyOwnExt, Box}; use core::any::Any; use core::atomics::{AtomicUint, SeqCst}; -use core::finally::Finally; use core::iter::Take; +use core::kinds::marker; use core::mem; use core::raw; @@ -29,14 +29,71 @@ use Runtime; use local::Local; use local_heap::LocalHeap; use rtio::LocalIo; +use unwind; use unwind::Unwinder; use collections::str::SendStr; -/// The Task struct represents all state associated with a rust -/// task. There are at this point two primary "subtypes" of task, -/// however instead of using a subtype we just have a "task_type" field -/// in the struct. This contains a pointer to another struct that holds -/// the type-specific state. +/// State associated with Rust tasks. +/// +/// Rust tasks are primarily built with two separate components. One is this +/// structure which handles standard services such as TLD, unwinding support, +/// naming of a task, etc. The second component is the runtime of this task, a +/// `Runtime` trait object. +/// +/// The `Runtime` object instructs this task how it can perform critical +/// operations such as blocking, rescheduling, I/O constructors, etc. The two +/// halves are separately owned, but one is often found contained in the other. +/// A task's runtime can be reflected upon with the `maybe_take_runtime` method, +/// and otherwise its ownership is managed with `take_runtime` and +/// `put_runtime`. +/// +/// In general, this structure should not be used. This is meant to be an +/// unstable internal detail of the runtime itself. From time-to-time, however, +/// it is useful to manage tasks directly. An example of this would be +/// interoperating with the Rust runtime from FFI callbacks or such. For this +/// reason, there are two methods of note with the `Task` structure. +/// +/// * `run` - This function will execute a closure inside the context of a task. +/// Failure is caught and handled via the task's on_exit callback. If +/// this fails, the task is still returned, but it can no longer be +/// used, it is poisoned. +/// +/// * `destroy` - This is a required function to call to destroy a task. If a +/// task falls out of scope without calling `destroy`, its +/// destructor bomb will go off, aborting the process. +/// +/// With these two methods, tasks can be re-used to execute code inside of its +/// context while having a point in the future where destruction is allowed. +/// More information can be found on these specific methods. +/// +/// # Example +/// +/// ```no_run +/// extern crate native; +/// use std::uint; +/// # fn main() { +/// +/// // Create a task using a native runtime +/// let task = native::task::new((0, uint::MAX)); +/// +/// // Run some code, catching any possible failures +/// let task = task.run(|| { +/// // Run some code inside this task +/// println!("Hello with a native runtime!"); +/// }); +/// +/// // Run some code again, catching the failure +/// let task = task.run(|| { +/// fail!("oh no, what to do!"); +/// }); +/// +/// // Now that the task is failed, it can never be used again +/// assert!(task.is_destroyed()); +/// +/// // Deallocate the resources associated with this task +/// task.destroy(); +/// # } +/// ``` pub struct Task { pub heap: LocalHeap, pub gc: GarbageCollector, @@ -79,7 +136,8 @@ pub enum BlockedTask { /// Per-task state related to task death, killing, failure, etc. pub struct Death { - pub on_exit: Option, + pub on_exit: Option, + marker: marker::NoCopy, } pub struct BlockedTasks { @@ -87,6 +145,13 @@ pub struct BlockedTasks { } impl Task { + /// Creates a new uninitialized task. + /// + /// This method cannot be used to immediately invoke `run` because the task + /// itself will likely require a runtime to be inserted via `put_runtime`. + /// + /// Note that you likely don't want to call this function, but rather the + /// task creation functions through libnative or libgreen. pub fn new() -> Task { Task { heap: LocalHeap::new(), @@ -100,80 +165,181 @@ impl Task { } } - /// Executes the given closure as if it's running inside this task. The task - /// is consumed upon entry, and the destroyed task is returned from this - /// function in order for the caller to free. This function is guaranteed to - /// not unwind because the closure specified is run inside of a `rust_try` - /// block. (this is the only try/catch block in the world). + /// Consumes ownership of a task, runs some code, and returns the task back. /// - /// This function is *not* meant to be abused as a "try/catch" block. This - /// is meant to be used at the absolute boundaries of a task's lifetime, and - /// only for that purpose. - pub fn run(~self, mut f: ||) -> Box { - // Need to put ourselves into TLS, but also need access to the unwinder. - // Unsafely get a handle to the task so we can continue to use it after - // putting it in tls (so we can invoke the unwinder). - let handle: *mut Task = unsafe { - *mem::transmute::<&Box, &*mut Task>(&self) - }; + /// This function can be used as an emulated "try/catch" to interoperate + /// with the rust runtime at the outermost boundary. It is not possible to + /// use this function in a nested fashion (a try/catch inside of another + /// try/catch). Invoking this funciton is quite cheap. + /// + /// If the closure `f` succeeds, then the returned task can be used again + /// for another invocation of `run`. If the closure `f` fails then `self` + /// will be internally destroyed along with all of the other associated + /// resources of this task. The `on_exit` callback is invoked with the + /// cause of failure (not returned here). This can be discovered by querying + /// `is_destroyed()`. + /// + /// Note that it is possible to view partial execution of the closure `f` + /// because it is not guaranteed to run to completion, but this function is + /// guaranteed to return if it fails. Care should be taken to ensure that + /// stack references made by `f` are handled appropriately. + /// + /// It is invalid to call this function with a task that has been previously + /// destroyed via a failed call to `run`. + /// + /// # Example + /// + /// ```no_run + /// extern crate native; + /// use std::uint; + /// # fn main() { + /// + /// // Create a new native task + /// let task = native::task::new((0, uint::MAX)); + /// + /// // Run some code once and then destroy this task + /// task.run(|| { + /// println!("Hello with a native runtime!"); + /// }).destroy(); + /// # } + /// ``` + pub fn run(~self, f: ||) -> Box { + assert!(!self.is_destroyed(), "cannot re-use a destroyed task"); + + // First, make sure that no one else is in TLS. This does not allow + // recursive invocations of run(). If there's no one else, then + // relinquish ownership of ourselves back into TLS. + if Local::exists(None::) { + fail!("cannot run a task recursively inside another"); + } Local::put(self); - // The only try/catch block in the world. Attempt to run the task's - // client-specified code and catch any failures. - let try_block = || { - - // Run the task main function, then do some cleanup. - f.finally(|| { - // First, destroy task-local storage. This may run user dtors. - // - // FIXME #8302: Dear diary. I'm so tired and confused. - // There's some interaction in rustc between the box - // annihilator and the TLS dtor by which TLS is - // accessed from annihilated box dtors *after* TLS is - // destroyed. Somehow setting TLS back to null, as the - // old runtime did, makes this work, but I don't currently - // understand how. I would expect that, if the annihilator - // reinvokes TLS while TLS is uninitialized, that - // TLS would be reinitialized but never destroyed, - // but somehow this works. I have no idea what's going - // on but this seems to make things magically work. FML. - // - // (added after initial comment) A possible interaction here is - // that the destructors for the objects in TLS themselves invoke - // TLS, or possibly some destructors for those objects being - // annihilated invoke TLS. Sadly these two operations seemed to - // be intertwined, and miraculously work for now... - drop({ - let mut task = Local::borrow(None::); - let &LocalStorage(ref mut optmap) = &mut task.storage; - optmap.take() - }); - - // Destroy remaining boxes. Also may run user dtors. - let mut heap = { - let mut task = Local::borrow(None::); - mem::replace(&mut task.heap, LocalHeap::new()) - }; - unsafe { heap.annihilate() } - drop(heap); - }) - }; + // There are two primary reasons that general try/catch is unsafe. The + // first is that we do not support nested try/catch. The above check for + // an existing task in TLS is sufficient for this invariant to be + // upheld. The second is that unwinding while unwinding is not defined. + // We take care of that by having an 'unwinding' flag in the task + // itself. For these reasons, this unsafety should be ok. + let result = unsafe { unwind::try(f) }; + + // After running the closure given return the task back out if it ran + // successfully, or clean up the task if it failed. + let task: Box = Local::take(); + match result { + Ok(()) => task, + Err(cause) => { task.cleanup(Err(cause)) } + } + } - unsafe { (*handle).unwinder.try(try_block); } + /// Destroy all associated resources of this task. + /// + /// This function will perform any necessary clean up to prepare the task + /// for destruction. It is required that this is called before a `Task` + /// falls out of scope. + /// + /// The returned task cannot be used for running any more code, but it may + /// be used to extract the runtime as necessary. + pub fn destroy(~self) -> Box { + if self.is_destroyed() { + self + } else { + self.cleanup(Ok(())) + } + } - // Here we must unsafely borrow the task in order to not remove it from - // TLS. When collecting failure, we may attempt to send on a channel (or - // just run arbitrary code), so we must be sure to still have a local - // task in TLS. - unsafe { - let me: *mut Task = Local::unsafe_borrow(); - (*me).death.collect_failure((*me).unwinder.result()); + /// Cleans up a task, processing the result of the task as appropriate. + /// + /// This function consumes ownership of the task, deallocating it once it's + /// done being processed. It is assumed that TLD and the local heap have + /// already been destroyed and/or annihilated. + fn cleanup(~self, result: Result) -> Box { + // The first thing to do when cleaning up is to deallocate our local + // resources, such as TLD and GC data. + // + // FIXME: there are a number of problems with this code + // + // 1. If any TLD object fails destruction, then all of TLD will leak. + // This appears to be a consequence of #14875. + // + // 2. Failing during GC annihilation aborts the runtime #14876. + // + // 3. Setting a TLD key while destroying TLD or while destroying GC will + // abort the runtime #14807. + // + // 4. Invoking GC in GC destructors will abort the runtime #6996. + // + // 5. The order of destruction of TLD and GC matters, but either way is + // susceptible to leaks (see 3/4) #8302. + // + // That being said, there are a few upshots to this code + // + // 1. If TLD destruction fails, heap destruction will be attempted. + // There is a test for this at fail-during-tld-destroy.rs. Sadly the + // other way can't be tested due to point 2 above. Note that we must + // immortalize the heap first becuase if any deallocations are + // attempted while TLD is being dropped it will attempt to free the + // allocation from the wrong heap (because the current one has been + // replaced). + // + // 2. One failure in destruction is tolerable, so long as the task + // didn't originally fail while it was running. + // + // And with all that in mind, we attempt to clean things up! + let mut task = self.run(|| { + let mut task = Local::borrow(None::); + let tld = { + let &LocalStorage(ref mut optmap) = &mut task.storage; + optmap.take() + }; + let mut heap = mem::replace(&mut task.heap, LocalHeap::new()); + unsafe { heap.immortalize() } + drop(task); + + // First, destroy task-local storage. This may run user dtors. + drop(tld); + + // Destroy remaining boxes. Also may run user dtors. + drop(heap); + }); + + // If the above `run` block failed, then it must be the case that the + // task had previously succeeded. This also means that the code below + // was recursively run via the `run` method invoking this method. In + // this case, we just make sure the world is as we thought, and return. + if task.is_destroyed() { + rtassert!(result.is_ok()) + return task + } + + // After taking care of the data above, we need to transmit the result + // of this task. + let what_to_do = task.death.on_exit.take(); + Local::put(task); + + // FIXME: this is running in a seriously constrained context. If this + // allocates GC or allocates TLD then it will likely abort the + // runtime. Similarly, if this fails, this will also likely abort + // the runtime. + // + // This closure is currently limited to a channel send via the + // standard library's task interface, but this needs + // reconsideration to whether it's a reasonable thing to let a + // task to do or not. + match what_to_do { + Some(f) => { f(result) } + None => { drop(result) } } - let mut me: Box = Local::take(); - me.destroyed = true; - return me; + + // Now that we're done, we remove the task from TLS and flag it for + // destruction. + let mut task: Box = Local::take(); + task.destroyed = true; + return task; } + /// Queries whether this can be destroyed or not. + pub fn is_destroyed(&self) -> bool { self.destroyed } + /// Inserts a runtime object into this task, transferring ownership to the /// task. It is illegal to replace a previous runtime object in this task /// with this argument. @@ -182,6 +348,13 @@ impl Task { self.imp = Some(ops); } + /// Removes the runtime from this task, transferring ownership to the + /// caller. + pub fn take_runtime(&mut self) -> Box { + assert!(self.imp.is_some()); + self.imp.take().unwrap() + } + /// Attempts to extract the runtime as a specific type. If the runtime does /// not have the provided type, then the runtime is not removed. If the /// runtime does have the specified type, then it is removed and returned @@ -374,21 +547,7 @@ impl BlockedTask { impl Death { pub fn new() -> Death { - Death { on_exit: None, } - } - - /// Collect failure exit codes from children and propagate them to a parent. - pub fn collect_failure(&mut self, result: Result) { - match self.on_exit.take() { - Some(f) => f(result), - None => {} - } - } -} - -impl Drop for Death { - fn drop(&mut self) { - // make this type noncopyable + Death { on_exit: None, marker: marker::NoCopy } } } diff --git a/src/librustrt/unwind.rs b/src/librustrt/unwind.rs index 1e9e63c211be8..657d604fcad4b 100644 --- a/src/librustrt/unwind.rs +++ b/src/librustrt/unwind.rs @@ -78,7 +78,6 @@ use uw = libunwind; pub struct Unwinder { unwinding: bool, - cause: Option> } struct Exception { @@ -107,25 +106,12 @@ impl Unwinder { pub fn new() -> Unwinder { Unwinder { unwinding: false, - cause: None, } } pub fn unwinding(&self) -> bool { self.unwinding } - - pub fn try(&mut self, f: ||) { - self.cause = unsafe { try(f) }.err(); - } - - pub fn result(&mut self) -> Result { - if self.unwinding { - Err(self.cause.take().unwrap()) - } else { - Ok(()) - } - } } /// Invoke a closure, capturing the cause of failure if one occurs. diff --git a/src/libstd/task.rs b/src/libstd/task.rs index e2d04a30a54bb..dad241002f86c 100644 --- a/src/libstd/task.rs +++ b/src/libstd/task.rs @@ -295,8 +295,8 @@ impl TaskBuilder { let (tx_done, rx_done) = channel(); // signal that task has exited let (tx_retv, rx_retv) = channel(); // return value from task - let on_exit = proc(res) { tx_done.send(res) }; - self.spawn_internal(proc() { tx_retv.send(f()) }, + let on_exit = proc(res) { let _ = tx_done.send_opt(res); }; + self.spawn_internal(proc() { let _ = tx_retv.send_opt(f()); }, Some(on_exit)); Future::from_fn(proc() { @@ -641,3 +641,14 @@ mod test { // NOTE: the corresponding test for stderr is in run-pass/task-stderr, due // to the test harness apparently interfering with stderr configuration. } + +#[test] +fn task_abort_no_kill_runtime() { + use std::io::timer; + use mem; + + let mut tb = TaskBuilder::new(); + let rx = tb.try_future(proc() {}); + mem::drop(rx); + timer::sleep(1000); +} diff --git a/src/test/run-pass/fail-during-tld-destroy.rs b/src/test/run-pass/fail-during-tld-destroy.rs new file mode 100644 index 0000000000000..835f1fe4d4a80 --- /dev/null +++ b/src/test/run-pass/fail-during-tld-destroy.rs @@ -0,0 +1,53 @@ +// 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. + +use std::task; +use std::gc::{GC, Gc}; +use std::cell::RefCell; + +static mut DROPS: uint = 0; + +struct Foo(bool); +impl Drop for Foo { + fn drop(&mut self) { + let Foo(fail) = *self; + unsafe { DROPS += 1; } + if fail { fail!() } + } +} + +fn tld_fail(fail: bool) { + local_data_key!(foo: Foo); + foo.replace(Some(Foo(fail))); +} + +fn gc_fail(fail: bool) { + struct A { + inner: RefCell>>, + other: Foo, + } + let a = box(GC) A { + inner: RefCell::new(None), + other: Foo(fail), + }; + *a.inner.borrow_mut() = Some(a.clone()); +} + +fn main() { + let _ = task::try(proc() { + tld_fail(true); + gc_fail(false); + }); + + unsafe { + assert_eq!(DROPS, 2); + } +} + From dfef4220242227fb210e58eedc91bcc74de8921f Mon Sep 17 00:00:00 2001 From: OGINO Masanori Date: Fri, 27 Jun 2014 05:22:26 +0900 Subject: [PATCH 2/4] std::io: Use re-exported pathes in examples. We use re-exported pathes (e.g. std::io::Command) and original ones (e.g. std::io::process::Command) together in examples now. Using re-exported ones consistently avoids confusion. Signed-off-by: OGINO Masanori --- src/libstd/io/mod.rs | 2 +- src/libstd/io/net/tcp.rs | 6 +++--- src/libstd/io/process.rs | 4 ++-- src/libstd/io/timer.rs | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libstd/io/mod.rs b/src/libstd/io/mod.rs index 1d6ad7c31e2fd..8014759c88ab0 100644 --- a/src/libstd/io/mod.rs +++ b/src/libstd/io/mod.rs @@ -83,7 +83,7 @@ Some examples of obvious things you might want to do ```rust # #![allow(unused_must_use)] - use std::io::net::tcp::TcpStream; + use std::io::TcpStream; # // connection doesn't fail if a server is running on 8080 # // locally, we still want to be type checking this code, so lets diff --git a/src/libstd/io/net/tcp.rs b/src/libstd/io/net/tcp.rs index 7d6ab9d7419b9..b79e831ff6135 100644 --- a/src/libstd/io/net/tcp.rs +++ b/src/libstd/io/net/tcp.rs @@ -41,7 +41,7 @@ use rt::rtio; /// /// ```no_run /// # #![allow(unused_must_use)] -/// use std::io::net::tcp::TcpStream; +/// use std::io::TcpStream; /// /// let mut stream = TcpStream::connect("127.0.0.1", 34254); /// @@ -162,7 +162,7 @@ impl TcpStream { /// ```no_run /// # #![allow(unused_must_use)] /// use std::io::timer; - /// use std::io::net::tcp::TcpStream; + /// use std::io::TcpStream; /// /// let mut stream = TcpStream::connect("127.0.0.1", 34254).unwrap(); /// let stream2 = stream.clone(); @@ -406,7 +406,7 @@ impl TcpAcceptor { /// /// ```no_run /// # #![allow(experimental)] - /// use std::io::net::tcp::TcpListener; + /// use std::io::TcpListener; /// use std::io::{Listener, Acceptor, TimedOut}; /// /// let mut a = TcpListener::bind("127.0.0.1", 8482).listen().unwrap(); diff --git a/src/libstd/io/process.rs b/src/libstd/io/process.rs index ecd6693990c43..8e1747146e42e 100644 --- a/src/libstd/io/process.rs +++ b/src/libstd/io/process.rs @@ -476,8 +476,8 @@ impl Process { /// /// ```no_run /// # #![allow(experimental)] - /// use std::io::process::{Command, ProcessExit}; - /// use std::io::IoResult; + /// use std::io::{Command, IoResult}; + /// use std::io::process::ProcessExit; /// /// fn run_gracefully(prog: &str) -> IoResult { /// let mut p = try!(Command::new("long-running-process").spawn()); diff --git a/src/libstd/io/timer.rs b/src/libstd/io/timer.rs index 9ae855536acbd..432461c460634 100644 --- a/src/libstd/io/timer.rs +++ b/src/libstd/io/timer.rs @@ -110,7 +110,7 @@ impl Timer { /// # Example /// /// ```rust - /// use std::io::timer::Timer; + /// use std::io::Timer; /// /// let mut timer = Timer::new().unwrap(); /// let ten_milliseconds = timer.oneshot(10); @@ -122,7 +122,7 @@ impl Timer { /// ``` /// /// ```rust - /// use std::io::timer::Timer; + /// use std::io::Timer; /// /// // Incorrect, method chaining-style: /// let mut five_ms = Timer::new().unwrap().oneshot(5); @@ -152,7 +152,7 @@ impl Timer { /// # Example /// /// ```rust - /// use std::io::timer::Timer; + /// use std::io::Timer; /// /// let mut timer = Timer::new().unwrap(); /// let ten_milliseconds = timer.periodic(10); @@ -170,7 +170,7 @@ impl Timer { /// ``` /// /// ```rust - /// use std::io::timer::Timer; + /// use std::io::Timer; /// /// // Incorrect, method chaining-style. /// let mut five_ms = Timer::new().unwrap().periodic(5); From 9a9908405d499e333baa20375bcaadfcca5459de Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 25 Jun 2014 18:10:50 -0700 Subject: [PATCH 3/4] librustc: Ensure that proc upvars have static lifetime. Since procs do not have lifetime bounds, we must do this to maintain safety. This can break code that incorrectly captured references in procedure types. Change such code to not do this, perhaps with a trait object instead. A better solution would be to add higher-rank lifetime support to procs. However, this would be a lot of work for a feature we want to remove in favor of unboxed closures. The corresponding "real fix" is #15067. Closes #14036. [breaking-change] --- src/libnative/io/process.rs | 4 ++++ src/librustc/middle/kind.rs | 10 +++++++-- src/test/compile-fail/proc-static-bound.rs | 26 ++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 src/test/compile-fail/proc-static-bound.rs diff --git a/src/libnative/io/process.rs b/src/libnative/io/process.rs index c421dada205fc..59a075f22b28b 100644 --- a/src/libnative/io/process.rs +++ b/src/libnative/io/process.rs @@ -533,6 +533,10 @@ fn spawn_process_os(cfg: ProcessConfig, let dirp = cfg.cwd.map(|c| c.with_ref(|p| p)).unwrap_or(ptr::null()); + let cfg = unsafe { + mem::transmute::>(cfg) + }; + with_envp(cfg.env, proc(envp) { with_argv(cfg.program, cfg.args, proc(argv) unsafe { let (mut input, mut output) = try!(pipe()); diff --git a/src/librustc/middle/kind.rs b/src/librustc/middle/kind.rs index 970ae36238bd6..34754f045ffce 100644 --- a/src/librustc/middle/kind.rs +++ b/src/librustc/middle/kind.rs @@ -198,8 +198,14 @@ fn with_appropriate_checker(cx: &Context, let fty = ty::node_id_to_type(cx.tcx, id); match ty::get(fty).sty { ty::ty_closure(box ty::ClosureTy { - store: ty::UniqTraitStore, bounds, .. - }) => b(|cx, fv| check_for_uniq(cx, fv, bounds)), + store: ty::UniqTraitStore, + bounds: mut bounds, .. + }) => { + // Procs can't close over non-static references! + bounds.add(ty::BoundStatic); + + b(|cx, fv| check_for_uniq(cx, fv, bounds)) + } ty::ty_closure(box ty::ClosureTy { store: ty::RegionTraitStore(region, _), bounds, .. diff --git a/src/test/compile-fail/proc-static-bound.rs b/src/test/compile-fail/proc-static-bound.rs new file mode 100644 index 0000000000000..f11ddc0151f96 --- /dev/null +++ b/src/test/compile-fail/proc-static-bound.rs @@ -0,0 +1,26 @@ +// 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. + +fn main() { + let mut x = Some(1); + let mut p: proc(&mut Option) = proc(_) {}; + match x { + Some(ref y) => { + p = proc(z: &mut Option) { + *z = None; + let _ = y; + //~^ ERROR cannot capture variable of type `&int`, which does not fulfill `'static` + }; + } + None => {} + } + p(&mut x); +} + From 64019e764f1837a4a19297fc9f7a99595e37cf51 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Wed, 25 Jun 2014 09:00:46 +1000 Subject: [PATCH 4/4] rustc: update the unnecessary parens lint for struct literals. Things like `match X { x: 1 } { ... }` now need to be written with parentheses, so the lint should avoid warning in cases like that. --- src/librustc/lint/builtin.rs | 64 +++++++++++++++---- .../compile-fail/lint-unnecessary-parens.rs | 23 +++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 5078ae80d75c1..339db67b3f1a1 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -975,14 +975,52 @@ declare_lint!(UNNECESSARY_PARENS, Warn, pub struct UnnecessaryParens; impl UnnecessaryParens { - fn check_unnecessary_parens_core(&self, cx: &Context, value: &ast::Expr, msg: &str) { + fn check_unnecessary_parens_core(&self, cx: &Context, value: &ast::Expr, msg: &str, + struct_lit_needs_parens: bool) { match value.node { - ast::ExprParen(_) => { - cx.span_lint(UNNECESSARY_PARENS, value.span, - format!("unnecessary parentheses around {}", msg).as_slice()) + ast::ExprParen(ref inner) => { + let necessary = struct_lit_needs_parens && contains_exterior_struct_lit(&**inner); + if !necessary { + cx.span_lint(UNNECESSARY_PARENS, value.span, + format!("unnecessary parentheses around {}", + msg).as_slice()) + } } _ => {} } + + /// Expressions that syntatically contain an "exterior" struct + /// literal i.e. not surrounded by any parens or other + /// delimiters, e.g. `X { y: 1 }`, `X { y: 1 }.method()`, `foo + /// == X { y: 1 }` and `X { y: 1 } == foo` all do, but `(X { + /// y: 1 }) == foo` does not. + fn contains_exterior_struct_lit(value: &ast::Expr) -> bool { + match value.node { + ast::ExprStruct(..) => true, + + ast::ExprAssign(ref lhs, ref rhs) | + ast::ExprAssignOp(_, ref lhs, ref rhs) | + ast::ExprBinary(_, ref lhs, ref rhs) => { + // X { y: 1 } + X { y: 2 } + contains_exterior_struct_lit(&**lhs) || + contains_exterior_struct_lit(&**rhs) + } + ast::ExprUnary(_, ref x) | + ast::ExprCast(ref x, _) | + ast::ExprField(ref x, _, _) | + ast::ExprIndex(ref x, _) => { + // &X { y: 1 }, X { y: 1 }.y + contains_exterior_struct_lit(&**x) + } + + ast::ExprMethodCall(_, _, ref exprs) => { + // X { y: 1 }.bar(...) + contains_exterior_struct_lit(&**exprs.get(0)) + } + + _ => false + } + } } } @@ -992,16 +1030,16 @@ impl LintPass for UnnecessaryParens { } fn check_expr(&mut self, cx: &Context, e: &ast::Expr) { - let (value, msg) = match e.node { - ast::ExprIf(cond, _, _) => (cond, "`if` condition"), - ast::ExprWhile(cond, _) => (cond, "`while` condition"), - ast::ExprMatch(head, _) => (head, "`match` head expression"), - ast::ExprRet(Some(value)) => (value, "`return` value"), - ast::ExprAssign(_, value) => (value, "assigned value"), - ast::ExprAssignOp(_, _, value) => (value, "assigned value"), + let (value, msg, struct_lit_needs_parens) = match e.node { + ast::ExprIf(cond, _, _) => (cond, "`if` condition", true), + ast::ExprWhile(cond, _) => (cond, "`while` condition", true), + ast::ExprMatch(head, _) => (head, "`match` head expression", true), + ast::ExprRet(Some(value)) => (value, "`return` value", false), + ast::ExprAssign(_, value) => (value, "assigned value", false), + ast::ExprAssignOp(_, _, value) => (value, "assigned value", false), _ => return }; - self.check_unnecessary_parens_core(cx, &*value, msg); + self.check_unnecessary_parens_core(cx, &*value, msg, struct_lit_needs_parens); } fn check_stmt(&mut self, cx: &Context, s: &ast::Stmt) { @@ -1015,7 +1053,7 @@ impl LintPass for UnnecessaryParens { }, _ => return }; - self.check_unnecessary_parens_core(cx, &*value, msg); + self.check_unnecessary_parens_core(cx, &*value, msg, false); } } diff --git a/src/test/compile-fail/lint-unnecessary-parens.rs b/src/test/compile-fail/lint-unnecessary-parens.rs index b2abe025794bf..4d9383aeda2a1 100644 --- a/src/test/compile-fail/lint-unnecessary-parens.rs +++ b/src/test/compile-fail/lint-unnecessary-parens.rs @@ -10,18 +10,41 @@ #![deny(unnecessary_parens)] +#[deriving(Eq, PartialEq)] +struct X { y: bool } +impl X { + fn foo(&self) -> bool { self.y } +} + fn foo() -> int { return (1); //~ ERROR unnecessary parentheses around `return` value } +fn bar() -> X { + return (X { y: true }); //~ ERROR unnecessary parentheses around `return` value +} fn main() { foo(); + bar(); if (true) {} //~ ERROR unnecessary parentheses around `if` condition while (true) {} //~ ERROR unnecessary parentheses around `while` condition match (true) { //~ ERROR unnecessary parentheses around `match` head expression _ => {} } + let v = X { y: false }; + // struct lits needs parens, so these shouldn't warn. + if (v == X { y: true }) {} + if (X { y: true } == v) {} + if (X { y: false }.y) {} + + while (X { y: false }.foo()) {} + while (true | X { y: false }.y) {} + + match (X { y: false }) { + _ => {} + } + let mut _a = (0); //~ ERROR unnecessary parentheses around assigned value _a = (0); //~ ERROR unnecessary parentheses around assigned value _a += (1); //~ ERROR unnecessary parentheses around assigned value