hasu / emacs-ob-racket

Emacs Org-Mode Babel code block Racket support
https://tero.hasu.is/blog/2011-09-08-on-racket-support-in-emacs-org-mode.html
GNU General Public License v3.0
36 stars 12 forks source link

[WIP] Typed racket #10

Closed scolobb closed 6 months ago

scolobb commented 7 months ago

The goal of this pull request is to make Ob-racket work with Typed Racket for result types list, and table.

Consider the following example:

#+BEGIN_SRC racket :results list
'(1 2 3)
#+END_SRC
#+RESULTS:
- 1
- 2
- 3

Now, switch to Typed Racket:

#+BEGIN_SRC racket :results list
#lang typed/racket
'(1 2 3)
#+END_SRC
#+RESULTS:
: - /tmp/babel-trSf0K/org-babel-kRhEhy.rkt:4:0:

I figured that by changing :results list to :results list drawer I can obtain the complete error message:

#+BEGIN_SRC racket :results list drawer
#lang typed/racket
'(1 2 3)
#+END_SRC
#+RESULTS:
:results:
- /tmp/babel-trSf0K/org-babel-k01r03.rkt:4:0: Type Checker: missing type for identifier;
- consider using `require/typed' to import it
- identifier: datum->table
- from module: (file /home/scolobb/.emacs.d/elisp/emacs-ob-racket/ob-racket-runtime.rkt)
- in: (ob-racket-begin-print-table (quote (1 2 3)))
- location...:
- /tmp/babel-trSf0K/org-babel-k01r03.rkt:4:0
- context...:
- /gnu/store/qsz0284ivqymwmafqn3szrlddnvqcpg7-racket-8.12/lib/racket/pkgs/typed-racket-lib/typed-racket/typecheck/tc-toplevel.rkt:481:0: type-check
- .../private/parse-interp.rkt:643:50
- /gnu/store/qsz0284ivqymwmafqn3szrlddnvqcpg7-racket-8.12/lib/racket/pkgs/typed-racket-lib/typed-racket/tc-setup.rkt:115:12
- /gnu/store/qsz0284ivqymwmafqn3szrlddnvqcpg7-racket-8.12/lib/racket/pkgs/typed-racket-lib/typed-racket/typed-racket.rkt:22:4
:end:

The gist of the error: datum->table imported from ob-racket-runtime.rkt has no type.

After asking the question on Racket Discourse and reading ob-racket.el, I ended up with the idea of adding a Typed Racket runtime for Ob-racket. This pull request does exactly this in the file ob-racket-runtime-typed.rkt. I currently use this file by setting the variable ob-racket-locate-runtime-library-function in Emacs using the following Elisp code:

(setq-local ob-racket-locate-runtime-library-function
      (lambda ()
    "/absolute/path/to/ob-racket-runtime-typed.rkt"))

While converting ob-racket-runtime.rkt to ob-racket-runtime-typed.rkt, I made the following reductions:

While the code I am submitting with this pull request "works for me", it feels like a kludge: using it requires switching the entire Ob-racket runtime, it comes in a separate file for what seems to be "just another language" (i.e., Typed Racket), etc. So, I see it rather as a work in progress and I would be happy to have suggestions about how I could improve it to better integrate with the existing code and also be more transparent to the user. Ideally, it would be nice if the typed runtime was loaded automatically whenever Typed Racket is detected, but I must confess I am not yet comfortable enough with ob-racket.el to attempt such changes on my own.

hasu commented 7 months ago

Thank you for the investigation and the PR.

How to deal with languages that are different enough that they require a different runtime library and/or a different use of it is a question I had neglected so far.

Probably the answer can be something similar to what is already there, but as you suggest for convenience it should be possible to transparently use another runtime.

I've prepared a draft change (1) to have ob-racket-raco-make-runtime-library handle multiple libraries, (2) to move runtime library selection into the code templates, and (3) to allow code fragment selection to be a function of the #lang. I've pushed that into a typed-racket feature branch. Does it look something like what you had in mind?

Switching to a different (typed) runtime for Typed Racket sounds right, but it's true that the current "ob-racket-runtime-typed.rkt" module adds duplication to the codebase. If the "ob-racket-runtime.rkt" internals were to be exposed, perhaps from a "private" module, would that make it possible to implement "ob-racket-runtime-typed.rkt" mostly in terms of the untyped code?

Or maybe that's not even much of an issue in this case, since the basic data types of languages tend not to change that often, and probably these conversion routines don't often need touching either. So if you prefer we can also go with the current "ob-racket-runtime-typed.rkt", at least for the time being.

scolobb commented 6 months ago

Thank you for your feedback @hasu ! I am currently on a research visit, so kind of short on time, but I'll take a deeper look at your suggestion as soon as I have a spare brain moment.

scolobb commented 6 months ago

Hello again @hasu . I read your commit and yes, it is exactly what I wanted, thank a lot! I particularly like the fact that you can now easily switch and fine-tune Racket runtimes directly within code block parameters.

If the "ob-racket-runtime.rkt" internals were to be exposed, perhaps from a "private" module, would that make it possible to implement "ob-racket-runtime-typed.rkt" mostly in terms of the untyped code?

If such a private module existed, then ob-racket-runtime-typed.rkt could be a simple typed module containing a bunch of require/typed/provide statements, assigning type signatures to the identifiers and re-exporting them. This could avoid a lot of code duplication.

As you saw in my commits, a few of the things currently defined in ob-racket-runtime.rkt are not supported by Typed Racket (e.g., mutable lists), but I believe that is straightforward to solve by provide appropriately (lightly) restrictive type signatures.

Now that I have finished writing this, I realize that no private module is needed in fact: ob-racket-runtime-typed.rkt could just import ob-racket-runtime.rkt directly, assigning the type signatures and re-exporting the identifiers!

Or maybe that's not even much of an issue in this case, since the basic data types of languages tend not to change that often, and probably these conversion routines don't often need touching either. So if you prefer we can also go with the current "ob-racket-runtime-typed.rkt", at least for the time being.

I am not really sure, frankly. I suppose it's not such a big deal having ob-racket-runtime-typed.rkt duplicating most of the code from ob-racket-runtime.rkt, since as you say it seems to be quite an edge case, which becomes easier to solve now with the new functionality you added. On the other hand, rewriting ob-racket-runtime-typed.rkt to just re-export identifiers with types seems really easy.

I guess I'd suggest you merge the typed-racket branch, and then I just submit another PR changing ob-racket-runtime-typed.rkt to require/typed/provide identifiers from ob-racket-runtime.rkt.

What do you think?

Also, I start having wondering about the performance penalty of using require/typed and require/typed/provide. I guess I could just write up the PR and ask on Racket Discourse.

hasu commented 6 months ago

Thanks for reviewing my change.

I've merged the initial implementation, and so in that respect this PR is ready to be closed.

I've also pushed another draft change to the typed-racket branch to split a "private" module out of "ob-racket-runtime.rkt", one that actually exports all the implementation functions, for reuse in runtime libraries with different conversion behavior or a different (e.g., typed) interface.

I assume that with those exports available it is then possible to require/typed much of the currently duplicated code, even if that means assigning more restrictive type signatures in some cases, as you say.

Perhaps it's worth doing if it's not much work and doesn't involve a noticeable performance penalty, but I'll leave it to you to decide whether to submit that PR.

scolobb commented 6 months ago

Thanks @hasu ! I will look into submitting the other change on top of your new commit over the next couple of days.