AST exploration features for ppx developers.

[arozovyk]

This PR adds a set of features aimed at simplifying the process of developing (and using) OCaml preprocessors.
Namely,

Exploring the Abstract Syntax Tree

• Live visualization of the OCaml AST inside a separate editor tab.
• The means of exploring the AST both ways: from standard text editor, choosing a piece of code and revealing the corresponding AST node inside the custom UI; Highlighting the corresponding source code when mouse cursor is moved onto a node inside the UI.

This is done by using Custom Editor API that allows defining an arbitrary output for a certain type of files. Under the hood it uses the Webview, that can render (almost) any custom HTML, and it communicates with the extension via message passing.

For the UI itself, i adapted the popular open source option - astexplorer.net. Notably, besides other minor modifications (redefining the communications, adding a separate tab for preprocessed AST), I reduced the application to only contain the Tree view, which is the AST output part.

Working with preprocessed code and its AST.

• Preview of the source code after it has been transformed by a preprocessor.
• A separate mode to navigate the transformed AST (keeping track of the original code).

Since modern ppxes are (and should be) developed using ppxlib, this PR introduces it as a dependency. I use ppxlib for almost all AST operations, and most importantly for its version of AST, because it's the one that will also be used by ppx developers.

However, this PR uses some internal features of ppxlib that are not currently exposed so it also depends on this issue being resolved.
Meanwhile, in order to test it, one can simply use my fork of ppxlib in his local switch:

opam pin ppxlib.0.22.0 git@github.com:arozovyk/ppxlib.git

Then build astexplorer with make view-build followed by make install to install the extension.

Usage:

1. To open AST explorer to the side: Command Palette -> OCaml: AST preview.
2. When astexplorer is visible, click on the code you want to reveal then either Shift+Ctrl+D or right click -> Reveal AST Node to highlight and focus the node in the AST explorer.
3. Another option to reveal AST nodes without executing a command is to activate the "Hover mode" with Shift+Ctrl+H (same to deactivate) which will make hovering over the code reveal corresponding nodes.
4. When moving your mouse cursor inside the AST explorer, the code is automatically highlighted.
5. [Ppx] To view the code after preprocessing by ppx: Command Palette -> Show Preprocessed Document.
6. [Ppx] To open both preprocessed code preview and AST explorer to the side: Command Palette -> Open both ppx editor and AST exploring.
7. [ppx] To explore the preprocessed AST, execute (6.) and choose the Preprocessed tab inside the AST explorer. The navigation is analogical to 2.-4.

[rgrinberg]

Why is this failwith? Isn't this an error that should be displayed in the WebView? (parsing error of sorts).

[arozovyk]

Changed it to managing Error (which should never occur since all of the Traverse_ast2.lift2methods returning it are accounted for by with_reparse (except if I forgot something).

[rgrinberg]

This is a fatal error that the user isn't responsible for and we cannot recover from, right? In that case we should introduce an exception type called Code_error. In general, we shouldn't be using failwith at all.

[arozovyk]

This is not a fatal error, it just means that user tried to activate hover mode without opening an AST editor. The failwith is useless though 8e2c305

[rgrinberg]

Again, this exception needs to be something else:

• User_error if the user is responsible or is part of the normal workflow
• Code_error if it's a bug in our extension or we don't know what happened.

[arozovyk]

Again, replaced failwith with just an Error. 064c5ab

[rgrinberg]

When s = "" as a result of not being able to parse the file, why are we trying to parse it again?

I think fetch_pp_code should return a more compound type and let the callers report errors.

val fetch_pp_code : TextDocument.t -> (string, Ppx_tools.read_error) result

This will also prevent some errors being displayed both in the webview and in the popup.

[arozovyk]

7a6c155

[rgrinberg]

It seems like the read_error type isn't useful anymore. As far as I can tell, Io_error and Read_error are now handled exactly the same way. So we can drop the distinction if that's intended.

[rgrinberg]

Final complaint about the UI:

The Retry/Abandon dialog seems somewhat suboptimal to me. If we had a refresh button/keybinding/action inside the web view that allowed us to refresh the view, that would be more than sufficient. This is less intrusive as it doesn't force the user's attention on this problem, and it also allows the user to restore the webview even if the menu was accidentally dismissed.

[arozovyk]

I'm changing this 9ba660b to a result type since we don't know where to catch exceptions in an elegant way. ( transform_to_ast being a different story).

[rgrinberg]

I'm changing this 9ba660b to a result type since we don't know where to catch exceptions in an elegant way. ( transform_to_ast being a different story).

Sounds good

[rgrinberg]

Let me clarify the general strategy behind error handling. In hindsight, I'm giving out concrete review feedback that is piecemeal. I can see how it might be hard to understand what is the end state that I'm looking for.

1. Whenever we want to abort and give control back to the user, we should use User_error. The message in this exception should clear. Thus including prefixes like "Read_error: " isn't good because it is making it cryptic for the user. If something is invalid (e.g. filename), we should include the invalid data in the error message.

2. Whenever there's an error condition that we must handle differently depending on the caller, we should prefer result types (if possible). The type signature makes it easy to remember that the error case must be explicitly handled.

3. If our error handling code is different based on the kind of error, we should introduce a type to represent the various error conditions we intend to handle. If our error handling code is the same for different errors, this excessive level of detail is confusing and creates opportunities for bugs. We aren't writing library code, so we don't need to write fully general code.

Some consequences of the above are:

1. We never use failwith. Because it is not clear whether the error message is a bug in our code or intended for the user.

2. We should usually not catch User_error unless we are in the top level handler. As this error signifies that whatever operation that is being done is aborted.

Let me know if you have any questions. Thanks for being patient and polishing the code.

[arozovyk]

Final complaint about the UI:

The Retry/Abandon dialog seems somewhat suboptimal to me. If we had a refresh button/keybinding/action inside the web view that allowed us to refresh the view, that would be more than sufficient. This is less intrusive as it doesn't force the user's attention on this problem, and it also allows the user to restore the webview even if the menu was accidentally dismissed.

I agree that dialog shouldn't be the only way to update the Webview, so i added a button for Preprocessed mode. I could remove the prompt but It would mean either

1. User has to close and reopen the Preprocessed Document to update it.
2. We gotta have an additional command to reload it.

[arozovyk]

Let me clarify the general strategy behind error handling. In hindsight, I'm giving out concrete review feedback that is piecemeal. I can see how it might be hard to understand what is the end state that I'm looking for.

1. Whenever we want to abort and give control back to the user, we should use User_error. The message in this exception should clear. Thus including prefixes like "Read_error: " isn't good because it is making it cryptic for the user. If something is invalid (e.g. filename), we should include the invalid data in the error message.
2. Whenever there's an error condition that we must handle differently depending on the caller, we should prefer result types (if possible). The type signature makes it easy to remember that the error case must be explicitly handled.
3. If our error handling code is different based on the kind of error, we should introduce a type to represent the various error conditions we intend to handle. If our error handling code is the same for different errors, this excessive level of detail is confusing and creates opportunities for bugs. We aren't writing library code, so we don't need to write fully general code.

Some consequences of the above are:

1. We never use failwith. Because it is not clear whether the error message is a bug in our code or intended for the user.
2. We should usually not catch User_error unless we are in the top level handler. As this error signifies that whatever operation that is being done is aborted.

Let me know if you have any questions. Thanks for being patient and polishing the code.

Thanks for taking your time and helping me figure this out.

I agree with the points you are making, the only thing that seems kinda wierd is why we don't catch User_error. Because if we don't, the user is never informed about it (unless he opens Dev tools). Is that what we are looking for?

[rgrinberg]

I agree with the points you are making, the only thing that seems kinda wierd is why we don't catch User_error. Because if we don't, the user is never informed about it (unless he opens Dev tools). Is that what we are looking for?

I was looking for a good way to handle these errors, but it looks like it is currently not possible to install a handler in one place: microsoft/vscode#45264

What we can do is a workaround like in this extension: julia-vscode/julia-vscode#1276.

So I suppose that means we need to:

• Add function that will install an exception handler (.catch) to handle User_error before ignoring
• Add try/catch in all providers etc. to catch synchronously thrown User_error and handle them the same way as above.

[rgrinberg]

I merged manually after squashing. Thanks for all the work.

[smorimoto]

There is no change log entry for this yet, so I think we can release 1.9.0 after adding it.

[ulugbekna]

We shouldn't release before we add more principal handling of ocaml-lsp versions in the extension; otherwise, we get false warnings about outdated ocaml-lsp version because people on ocaml v < 4.12 cannot really update to the latest ocaml-lsp. Another thing that should be handled before a release is that jumping between holes keyboard shortcuts coincide with important shortcuts on French keyboard.

…

On Sat, 4 Sept 2021 at 07:57, Sora Morimoto ***@***.***> wrote: There is no change log entry for this yet, so I think we can release 1.9.0 after adding it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#666 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR63USFUZAQRRXCDIDADUAGYNBANCNFSM5AT7DJGA> .

[ulugbekna]

Hi, I'm getting an unbound module error when I try to build the master, which version of ppxlib are we expected to have? both ppxlib master and latest release are failing

dune build src/vscode_ocaml_platform.bc.js
File "src/ppx_tools/ppx_tools.mli", line 5, characters 38-53:
5 | val get_preprocessed_ast : string -> (Ppxlib.Ast_io.t, string) result
                                          ^^^^^^^^^^^^^^^
Error: Unbound module Ppxlib.Ast_io
make: *** [build] Error 1

[rgrinberg]

We shouldn't release before we add more principal handling of ocaml-lsp
versions in the extension; otherwise, we get false warnings about outdated
ocaml-lsp version because people on ocaml v < 4.12 cannot really update to
the latest ocaml-lsp

How about we just make it possible for the user to disable such warnings per project? I'd rather not introduce some heavy weight solution to a problem that is fixed by upgrading.

Another thing that should be handled before a release
is that jumping between holes keyboard shortcuts coincide with important
shortcuts on French keyboard.

Okay, let's get rid of the shortcuts for now then. We can think of better shortcuts later.

[Khady]

would it be possible to use dune to get the root of the project instead of vscode's workspace? The current code doesn't work in the context of a monorepo

[rgrinberg]

Hmm, but why not? Doesn't it make sense to open the monorepo such that its root matches the workspace root?

[Khady]

here I'm thinking about a monorepo in a larger sense than just a dune monorepo. A repo containing multiple projects written in multiple languages with different build systems. It's convenient to have the whole thing open (search in the whole repo, check that a change in one project doesn't break the other ones, ...)

This is not a total deal breaker. It would just make the experience better. I've created #873 to keep track of the idea.

[rgrinberg]

I see. Well I'm definitely not against the idea.