Implement help provider for R #113

jmcphers · 2023-10-17T02:18:05Z

This change implements a help provider for R that can suggest a help topic appropriate to the cursor position and, given a help topic, show help for that topic in the Help pane.

The change is large because the implementation required adding a new channel over which the help topic request could be delivered, which in turn required some somewhat heavy-handed refactoring of how we handle Help. The good news, however, is that I was able to clean up some old TODOs and reduce the reliance on global state; we now only start the Help backend when Positron needs it, and don't need to start the HTTP help server when using a non-Positron front end. Like Environment, we now only engage the Help machinery on demand.

There are two new pieces of functionality here:

A Help comm that sends notifications of new help content, and has a single RPC that can be used for Positron to ask for help to be shown on a given topic.
A new LSP method, textDocument/helpTopic, which is used to query for a Help topic appropriate for the cursor position.

Must not be merged before posit-dev/positron#1593. See that PR for the front-end Help implementation.

Note

The helpTopic implementation here is little more than a proof of concept or stub; it just looks for the nearest identifier at or preceding the cursor. This works as long as you mash F1 directly on or near the keyword you want help on, but it also picks up local variable names and other stuff. I'll improve it in a follow up PR, and/or we can tackle it in a separate effort. The goal here is to have a proof of concept so we can unblock getting this work done for Python.

…ovider

lionel-

Looks good! Just a missing r_task() (see comment inline).

crates/ark/src/browser.rs

crates/ark/src/help/message.rs

crates/ark/src/help/r_help.rs

lionel- · 2023-10-18T10:08:30Z

crates/ark/src/lsp/help_topic.rs

+        let mut node = cursor.find_leaf(point);
+
+        // Find the nearest node that is an identifier.
+        while node.kind() != "identifier" {


This is where we will need a "find function call at point" to avoid foo(bar<> = ) looking up bar instead of foo right? Probably worth extracting this in a helper function that could be reused elsewhere and tested for edge cases more easily.

Probably also need to scope the search to the nearest call so that foo(bar()(baz =<> )) doesn't return foo but instead returns nil.

Less important: In some cases we might still want to look up help for simple symbols, such as list(NULL<>) where we'd show ?NULL instead of ?list.

Yes, these and other improvements need to be made in the topic detector! Will tackle that as a follow up.

crates/ark/src/shell.rs

crates/ark/tests/help.rs

…ovider

DavisVaughan · 2023-10-18T19:33:01Z

Would you mind going into vroom and doing devtools::load_all() followed by ?vroom. It should say

ℹ Rendering development documentation for "vroom"

but for me it no longer opens an external browser showing the help page

(my exact reprex used vctrs and ?vec_sort but i know you have vroom locally)

I see

[R] 2023-10-18T21:01:18.791134000Z [ark-unknown] WARN crates/ark/src/help/r_help.rs:132: Error handling Help request: Help URL '/var/folders/by/4wf2qrmn4_j93s5k5k5fzrx00000gp/T/Rtmpw0UYqk/.R/doc/html/vec_order.html' doesn't have expected prefix 'http://127.0.0.1:15295/'
[R] 
[R] Occurred at:
[R]    5: ark::help::r_help::RHelp::show_help_url
[R]              at /Users/davis/files/programming/positron/amalthea/crates/ark/src/help/r_help.rs:213:24

DavisVaughan

There is currently a bug with dev help, which did used to "work" by opening in an external browser. But otherwise looks good!

DavisVaughan · 2023-10-18T20:14:30Z

crates/ark/src/browser.rs

+    if let Some(help_tx) = help_tx {
+        if let Err(err) = help_tx.send(HelpRequest::ShowHelpUrl(url.to_string())) {
+            log::error!("Failed to send help message: {}", err);
+        }
+    }


Suggested change

if let Some(help_tx) = help_tx {

if let Err(err) = help_tx.send(HelpRequest::ShowHelpUrl(url.to_string())) {

log::error!("Failed to send help message: {}", err);

}

}

let help_tx = unwrap!(help_tx, None => {

log::error!("Failed to access `help_tx`");

return Ok(true);

});

let message = HelpRequest::ShowHelpUrl(url.to_string());

let _ = unwrap!(help_tx.send(message), Err(err) => {

log::error!("Failed to send help message: {err:?}");

return Ok(true);

});

Very much a nit, but I have come to prefer keeping as flat a code structure as possible (either with unwrap! or match) + using early returns in the unexpected cases.

(at the very least applying the {err:?} here too is worth it)

(when do we ever return false from this function?)

I also like this alternative too (we are still figuring out the most idiomatic approaches, but I think this is probably close)

let Some(help_tx) = help_tx else { log::error!("Failed to access `help_tx`"); return Ok(true); }; let message = HelpRequest::ShowHelpUrl(url.to_string()); if let Err(err) = help_tx.send(message) { log::error!("Failed to send help message: {err:?}"); return Ok(true); }

Thanks, I've refactored as suggested!

(when do we ever return false from this function?)

Turning this into a fire-and-forget request to the help thread means we can't tell if it decided to handle the URL or not (which is what the boolean indicates). I realized after you mentioned the vroom example that this means we don't know whether or not to perform the fallback behavior of using open, so I restored the previous behavior by adding a reply in b5e7ecd.

crates/ark/src/help/message.rs

DavisVaughan · 2023-10-18T20:18:06Z

crates/ark/src/help/message.rs

+    /// The content help type. Must be one of 'html', 'markdown', or 'url'.
+    pub kind: String,


Probably worth a HelpContentKind enum?

Good idea; this was inherited from events.yaml which didn't have enums.

DavisVaughan · 2023-10-18T20:22:07Z

crates/ark/src/help/r_help.rs

+    /**
+     * Start the help handler. Returns a channel for sending help requests to
+     * the help thread.
+     *
+     * - `comm`: The socket for communicating with the front end.
+     */


I am sad to report that if you use these block style comments over /// doc string comments then rust-analyzer shows them in this horrible way where each line looks like a bullet 😆 rust-lang/rust-analyzer#1759

Hah, that's awesome!

DavisVaughan · 2023-10-18T20:28:13Z

crates/ark/src/help/r_help.rs

+        // number when it starts, so if it's still at the default value (0), it
+        // hasn't started.
+        let mut started = false;
+        let r_help_port: u16;


I didn't know you could delay the first assignment like this! Cool!

The Rust linter actually made me do this after it realized the code below this guaranteed the variable would be assigned exactly once. Really nice.

DavisVaughan · 2023-10-18T21:06:06Z

crates/ark/src/interface.rs

-        // TODO: Should starting the R help server proxy really be here?
-        // Are we sure we want our own server when ark runs in a Jupyter notebook?
-        // Moving this requires detangling `help_server_port` from
-        // `modules::initialize()`, which seems doable.
-        // Start R help server proxy
-        help_proxy::start(r_module_info.help_server_port);
+        modules::initialize().unwrap();


crates/ark/tests/help.rs

Co-authored-by: Davis Vaughan <[email protected]>

…ovider

DavisVaughan · 2023-10-19T13:00:02Z

crates/ark/src/interface.rs

    pub help_tx: Option<Sender<HelpRequest>>,
+    pub help_rx: Option<Receiver<HelpReply>>,


Should these names be more specific now? i.e. help_request_tx and help_reply_rx?

I like the terser forms, but could also be convinced to rename 'em.

jmcphers added 16 commits October 12, 2023 12:53

stubs for R help handler module

82936fc

add message type and listener

bd9d16e

move help proxy start into comm

36a6c61

fix reentrancy for help server start

4ee49be

begin to split internal requests

f873b43

Merge remote-tracking branch 'origin/main' into feature/help-topic-pr…

ee32288

…ovider

move url manipulation into help module

984ec6c

call into help topic helper method

a70c667

improve comments; clean up unused proxy port message

6b78b04

suppress httpd messages; use proxy port for rewrite

77203e6

reply to show help topic message

0d1efaa

fix condition on length

8bdb73b

impl of helpTopic LSP method

3a05d41

don't try to search; look for adjacent identifiers

a4f29fc

use httpdport to detect R help server port on reconnect

6803173

Merge remote-tracking branch 'origin/main' into feature/help-topic-pr…

0501b41

…ovider

DavisVaughan self-requested a review October 17, 2023 19:45

add very basic help comm test

230e91e

lionel- approved these changes Oct 18, 2023

View reviewed changes

jmcphers added 5 commits October 18, 2023 09:06

better name for help request sender

61832f3

improve naming on help message enums

4b7f061

logging improvements in r_help

3bc7bd3

only access R_MAIN on R thread

6b54dcb

Merge remote-tracking branch 'origin/main' into feature/help-topic-pr…

1a31ddf

…ovider

DavisVaughan approved these changes Oct 18, 2023

View reviewed changes

jmcphers and others added 4 commits October 18, 2023 14:38

Update crates/ark/src/help/message.rs

d112287

Co-authored-by: Davis Vaughan <[email protected]>

clearer error handling for handle_help_url

018cc2e

Merge remote-tracking branch 'origin/main' into feature/help-topic-pr…

e02bd0b

…ovider

add channel to indicate when help is/isn't shown

b5e7ecd

DavisVaughan reviewed Oct 19, 2023

View reviewed changes

jmcphers added 2 commits October 19, 2023 08:30

add tests for help request comm

a2b418c

use HelpContentKind enum

483fd5f

jmcphers merged commit 27734a7 into main Oct 19, 2023

DavisVaughan deleted the feature/help-topic-provider branch October 14, 2024 12:40

-    if let Some(help_tx) = help_tx {
-        if let Err(err) = help_tx.send(HelpRequest::ShowHelpUrl(url.to_string())) {
-            log::error!("Failed to send help message: {}", err);
-        }
-    }
+    let help_tx = unwrap!(help_tx, None => {
+        log::error!("Failed to access `help_tx`");
+        return Ok(true);
+    });
+    let message = HelpRequest::ShowHelpUrl(url.to_string());
+    let _ = unwrap!(help_tx.send(message), Err(err) => {
+        log::error!("Failed to send help message: {err:?}");
+        return Ok(true);
+    });

		/// The content help type. Must be one of 'html', 'markdown', or 'url'.
		pub kind: String,

		pub help_tx: Option<Sender<HelpRequest>>,
		pub help_rx: Option<Receiver<HelpReply>>,

Implement help provider for R #113

Implement help provider for R #113

Uh oh!

Conversation

jmcphers commented Oct 17, 2023

Uh oh!

lionel- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DavisVaughan commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavisVaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DavisVaughan commented Oct 18, 2023 •

edited

Loading