Tweak more comments

Author: lionel-

crates/ark/src/interface.rs

@@ -309,7 +309,8 @@ pub struct RMain {
     /// consoles to get R to shut down
     read_console_shutdown: Cell<bool>,
 
-    /// Current topmost environment on the stack while waiting for input in ReadConsole
+    /// Current topmost environment on the stack while waiting for input in ReadConsole.
+    /// This is a RefCell since we require `get()` for this field and `RObject` isn't `Copy`.
     pub(crate) read_console_frame: RefCell<RObject>,
 }
 
@@ -344,7 +345,6 @@ impl PendingInputs {
         let status = match harp::parse_status(&input) {
             Err(err) => {
                 // Failed to even attempt to parse the input, something is seriously wrong
-                // FIXME: There are some valid syntax errors going through here, e.g. `identity |> _(1)`.
                 return Ok(ParseResult::SyntaxError(format!("{err}")));
             },
             Ok(status) => status,
@@ -353,7 +353,7 @@ impl PendingInputs {
         // - Incomplete inputs put R into a state where it expects more input that will never come, so we
         //   immediately reject them. Positron should never send us these, but Jupyter Notebooks may.
         // - Complete statements are obviously fine.
-        // - Syntax errors will cause R to throw an error, which is expected.
+        // - Syntax errors will get bubbled up as R errors via an `ConsoleResult::Error`.
         let exprs = match status {
             harp::ParseResult::Complete(exprs) => exprs,
             harp::ParseResult::Incomplete => {
@@ -793,6 +793,7 @@ impl RMain {
             read_console_nested_return: Cell::new(false),
             read_console_threw_error: Cell::new(false),
             read_console_nested_return_next_input: Cell::new(None),
+            // Can't use `R_ENVS.global` here as it isn't initialised yet
             read_console_frame: RefCell::new(RObject::new(unsafe { libr::R_GlobalEnv })),
             read_console_shutdown: Cell::new(false),
         }
@@ -956,16 +957,17 @@ impl RMain {
             // Clear any pending inputs, if any
             self.pending_inputs = None;
 
-            // Reply to active request with error
+            // Reply to active request with error, then fall through to event loop
             self.handle_active_request(&info, ConsoleValue::Error(exception));
         } else if matches!(info.kind, PromptKind::InputRequest) {
-            // Request input reply to the frontend and return it to R
+            // Request input from the frontend and return it to R
             return self.handle_input_request(&info, buf, buflen);
         } else if let Some(input) = self.pop_pending() {
             // Evaluate pending expression if there is any remaining
             return self.handle_pending_input(input, buf, buflen);
         } else {
-            // Otherwise reply to active request with accumulated result
+            // Otherwise reply to active request with accumulated result, then
+            // fall through to event loop
             let result = self.take_result();
             self.handle_active_request(&info, ConsoleValue::Success(result));
         }
@@ -1029,12 +1031,12 @@ impl RMain {
         let tasks_interrupt_index = select.recv(&tasks_interrupt_rx);
         let polled_events_index = select.recv(&polled_events_rx);
 
-        // Don't process idle tasks unless at top level. We currently don't want
-        // idle tasks (e.g. for srcref generation) to run when the call stack is
-        // not empty. We could make this configurable though if needed, i.e. some
-        // idle tasks would be able to run in the browser. Those should be sent
-        // to a dedicated channel that would always be included in the set of
-        // recv channels.
+        // Only process idle at top level. We currently don't want idle tasks
+        // (e.g. for srcref generation) to run when the call stack is not empty.
+        // We could make this configurable though if needed, i.e. some idle
+        // tasks would be able to run in the browser. Those should be sent to a
+        // dedicated channel that would always be included in the set of recv
+        // channels.
         let tasks_idle_index = if matches!(info.kind, PromptKind::TopLevel) {
             Some(select.recv(&tasks_idle_rx))
         } else {
@@ -1175,8 +1177,7 @@ impl RMain {
 
         if autoprint.ends_with('\n') {
             // Remove the trailing newlines that R adds to outputs but that
-            // Jupyter frontends are not expecting. Is it worth taking a
-            // mutable self ref across calling methods to avoid the clone?
+            // Jupyter frontends are not expecting
             autoprint.pop();
         }
         if autoprint.len() != 0 {
@@ -1249,9 +1250,6 @@ impl RMain {
         Some(exception)
     }
 
-    /// Returns:
-    /// - `None` if we should fall through to the event loop to wait for more user input
-    /// - `Some(ConsoleResult)` if we should immediately exit `read_console()`
     fn handle_active_request(&mut self, info: &PromptInfo, value: ConsoleValue) {
         // If we get here we finished evaluating all pending inputs. Check if we
         // have an active request from a previous `read_console()` iteration. If
@@ -2423,7 +2421,8 @@ pub extern "C-unwind" fn r_read_console(
     }
 
     // We've finished evaluating a dummy value to reset state in R's REPL,
-    // and are now ready to evaluate the actual input
+    // and are now ready to evaluate the actual input, which is typically
+    // just `.ark_last_value`.
     if let Some(next_input) = main.read_console_nested_return_next_input.take() {
         RMain::on_console_input(buf, buflen, next_input).unwrap();
         return 1;

crates/ark/src/main.rs

@@ -83,10 +83,6 @@ fn main() -> anyhow::Result<()> {
     let mut capture_streams = true;
     let mut default_repos = DefaultRepos::Auto;
 
-    // We don't support the asking the user whether to save the workspace data
-    // on exit because calling readline during shutdown puts in a precarious
-    // position. So effectively we're implementing "no-save" by default.
-    // Process remaining arguments. TODO: Need an argument that can passthrough args to R
     while let Some(arg) = argv.next() {
         match arg.as_str() {
             "--connection_file" => {

crates/ark/src/sys/unix/interface.rs

@@ -73,12 +73,15 @@ pub fn setup_r(args: &Vec<String>) {
         libr::set(ptr_R_Busy, Some(r_busy));
         libr::set(ptr_R_Suicide, Some(r_suicide));
 
+        // Install a CleanUp hook for integration tests that test the shutdown process.
+        // We confirm that shutdown occurs by waiting in the test until `CLEANUP_SIGNAL`'s
+        // condition variable sends a notification, which occurs in this cleanup method
+        // that is called during R's shutdown process.
         if stdext::IS_TESTING {
-            use libr::ptr_R_CleanUp;
-
-            use crate::interface::r_cleanup_for_tests;
-
-            libr::set(ptr_R_CleanUp, Some(r_cleanup_for_tests));
+            libr::set(
+                libr::ptr_R_CleanUp,
+                Some(crate::interface::r_cleanup_for_tests),
+            );
         }
 
         // In tests R may be run from various threads. This confuses R's stack

crates/ark/tests/kernel.rs

@@ -5,9 +5,6 @@ use amalthea::wire::kernel_info_request::KernelInfoRequest;
 use ark::fixtures::DummyArkFrontend;
 use stdext::assert_match;
 
-// Avoids our global calling handler from rlangifying errors.
-// This causes some test instability across configs.
-
 #[test]
 fn test_kernel_info() {
     let frontend = DummyArkFrontend::lock();
@@ -424,8 +421,8 @@ fn test_execute_request_error() {
 
 #[test]
 fn test_execute_request_error_with_accumulated_output() {
-    // Test that when the very last input output and then throws an error,
-    // the accumulated output is flushed before the error is reported.
+    // Test that when the very last input throws an error after producing
+    // output, the accumulated output is flushed before the error is reported.
     // This tests the autoprint buffer flush logic in error handling.
     let frontend = DummyArkFrontend::lock();
 

crates/harp/src/parse.rs

@@ -220,15 +220,15 @@ mod tests {
                 Ok(ParseResult::Incomplete)
             );
 
-            // Error
+            // Syntax error (error longjump thrown by parser)
             assert_match!(
                 parse_status(&ParseInput::Text("42 + _")),
                 Ok(ParseResult::SyntaxError { message }) => {
                     assert!(message.contains("invalid use of pipe placeholder"));
                 }
             );
 
-            // "normal" syntax error
+            // Syntax error (error code returned by parser)
             assert_match!(
                 parse_status(&ParseInput::Text("1+1\n*42")),
                 Ok(ParseResult::SyntaxError { message }) => {