Allow execution of included OCaml code blocks

[panglesd]

This PR allows to execute OCaml code blocks.

This is useful whenever someone wants to separate the actual code from the mdx-experiments on it.

For instance:

(** file.ml *)

let rec compute = [%something]

<!-- mdx_doc.md -->

Here is the code of the function we are studying:
```ocaml exec,file=file.ml
let rec compute = [%something]
```
This function is very interesting! 
```ocaml
# compute 1 ;;
- : int = 2
# compute 2 ;;
- : int = 1
```

Without this PR, we would have an "unbound value" error for the compute function. This will prove especially useful for the odoc reference driver.

I added a exec flag to execute an included code block only for backward compatibility.

[gpetiot]

I see the new flag has been added for retro-compatibility, but it sounds confusing to me to have the skip tag to not execute, and the exec tag to execute.

It sounds acceptable to me to make this a breaking change and make people add a skip tag to their existing blocks they don't want to execute. Are there some corner cases I don't see?

[panglesd]

I would be very happy to remove the exec tag since I agree it's confusing!

However, there is a bit more "breaking changes" than what we first think:

• Current behavior, for include blocks:
  • Skip tag is there: Do not include the content from the file
  • No tags: include the content but do not execute
• Behavior as of now in the PR, for include blocks:
  • Skip tag is there: Do not include the content from the file
  • No tags: include the content but do not execute
  • Exec tag: include the content and execute.

If we remove the exec tag, and always execute, we have two breaking changes:

• No tags: include the content <span style="color:red>and execute it
• A skip tag: we have two options here:
  1. Do not include the content (the old "skip" behavior)
  2. Include the content and do not execute it (the old "no tag" behavior)

If we do i. and do not change the "skip" behavior, it is not anymore possible to have the old "no tag" behavior: inclusion but no execution. If we go with ii. we can still have the old "skip" behavior by removing the "file" tag.

So what would you think of ii.:

• Changing skipped inclusion of file to: include content but no execution
• Changing inclusion of file to: include content and execute it

?

I'm happy with this, but I think it is quite a big breaking change for any code that has some side effects (such as creating a file, ...)

[gpetiot]

I have another suggestion based on the behavior of other kind of blocks.

I had a look in lib/test/mdx_test.ml and there is a check on the non-det option for ocaml/cram/toplevel blocks so that:

• non deterministic command: skip everything
• non deterministic output: run the command but keep the old output
• deterministic: run as usual

Previously the non-det tag was not allowed for include blocks.
Now it would make sense, to evaluate and update by default, and then:

• skip: don't include, don't do anything
• non-det command: include, but don't exec, don't update the output
• non-det output: include, exec, but don't update the output
• otherwise, include, exec and update

[panglesd]

That's a sensible idea. But I'm not super happy with reusing a command with an associated semantics (the fact that the output/command is non deterministic). In particular, the MDX_RUN_NON_DETERMINISTIC environment variable has an effect on those blocks, that we might not want in this case?

I think the combination skip + include in a block does not make sense (why specifying the include if we are going to skip it?) so I think it would make more sense to have "skipped blocks" include content but not execute it.
Note that it does not prevent you from using non-det on included code blocks! So, for include blocks:

• skip: include, don't do anything
• non-det command: include, but don't exec, don't update the output, unless MDX_RUN_NON_DETERMINISTIC or the --non-deterministic CLI flag is passed
• non-det output: include, exec, but don't update the output, unless MDX_RUN_NON_DETERMINISTIC or the --non-deterministic CLI flag is passed
• otherwise, include, exec and update
• (Skip and no include tag: don't include, don't exec)

[gpetiot]

I implemented your suggestion. I had to add a disctinction on the ocaml_value type, either Impl or Intf, since we only want to execute implementations. I think it's ready for a first review.

[gpetiot]

@panglesd I moved the ocaml_kind info like you suggested, and improved the tests. Let me know what you think!

[panglesd]

I still think that skip should include the block, but not execute it.

In order to "skip" the inclusion, just omit the include directive! Using non-det flag is not possible since it has some other meaning and thus usecase (such as being sensitive to some environment variable).

[panglesd]

I cannot approve my own pull request, but I do approve!

[panglesd]

I'm a bit concerned with breaking existing users. A search on github yields quite a lot of results which rely on the code not being executed:

• Some side effects, such as line printing
• Some code that expect user input/to be run on a terminal/loops and never return
• Some code that requires JS stubs

So, I think the decision to do this breaking change is OK (mdx is still in early versions) but the change should be well communicated!

[panglesd]

Maybe it would still make sense to add skip to the parts-begin-end test and sync-to-md tests as they were written with the idea of not executing the code, and the multiple errors are making it harder to read the thing that was actually tested.

[gpetiot]

Alright, looks good to me now, thanks @panglesd!
Waiting for the CI, then I will merge.

[brendanzab]

Hello! I think this change broke my build. Previously file parts, eg. in this README.md were not checked, but now I’m getting:

File "lang-shader-graphics/README.md", line 1, characters 0-0:
diff --git a/_build/default/lang-shader-graphics/README.md b/_build/default/lang-shader-graphics/.mdx/README.md.corrected
index 35fa4cc..1717a2e 100644
--- a/_build/default/lang-shader-graphics/README.md
+++ b/_build/default/lang-shader-graphics/.mdx/README.md.corrected
@@ -27,6 +27,10 @@ let scene : (vec3f repr) Env.t  =
   (* The final output colour to render at the current UV coordinate. *)
   Env.pure (background_color |> overlay ~shape:shape ~color:shape_color)
 ```
+```mdx-error
+Line 2, characters 28-33:
+Error: Unbound module Env
+```

 The resulting DSL is clunkier than I’d like. This is due to OCaml’s odd approach
 to custom operators (which don’t allow for custom precedences), and lack of

This is an issue if you want to include file parts where modules and definitions lie outside the code snippets. Is there a way to disable this behavior? I didn’t see any instructions in the changelog or README with guidance, so I’ve limited my MDX version for now, brendanzab/language-garden@3e890a8, following the example of the eio developers in ocaml-multicore/eio#706.

[panglesd]

You can use the skip keyword:

- <!-- $MDX file=examples/Readme.ml,part=scene -->
+ <!-- $MDX file=examples/Readme.ml,part=scene,skip -->

@gpetiot, do you want to announce the release on discuss? It was suggested a few times already, and we can see it is not clear enough how to upgrade to 2.4! Also, mentioning skip in the changelog can be good as that's likely the first thing they'll look when breaking.

So, I think the decision to do this breaking change is OK (mdx is still in early versions) but the change should be well communicated!

Third, I'd recommend you mention the breaking change in some public space like discuss or the mailing list. That might save some headaches for some developers.

[brendanzab]

You can use the skip keyword:

Hmm, I just tried using skip, and it does not seem to keep the file part from examples/Readme.ml in sync with the README – i.e. when I change the source file it does not result in test failure diff.

I had actually assumed this would be the case, which is why I didn’t think there was a workaround, but I should have checked and mentioned it when reporting!

[panglesd]

Mmmh, I tried on your specific case, and it seems that doing dune clean followed by dune test does result in a failure diff.

However, without the clean before, it does not. Maybe you are missing a deps on Readme.ml your dune file? or should this be automatic? I don't know!

[brendanzab]

Ahh! Yeah it seems like maybe skip is interacting with the dependencies somehow? It seems like when you remove it, the changes are recognised, but if you add it, you need to explicitly add the files using deps? This seems very surprising. Perhaps this is because skip is being used for more things now, beyond what the original behavior was intended… previously maybe the dune developers were thinking it meant “skip adding the file to the dependencies” or something.

At any rate, I do think it’s very surprising in general for file parts to be executed by default, seeing as they are usually part of an existing file. I’m also wondering if it would it be better to make the code execution behavior opt-in in? It does seem like a big change in the default behavior for a minor release at any rate? 🤔

[brendanzab]

previously maybe the dune developers were thinking it meant “skip adding the file to the dependencies” or something.

Wait! It’s not dune, it’s part of how mdx collects dependencies!

mdx/lib/dep.ml

Lines 19 to 26 in bf33f76

	let of_block block =
	let open Block in
	match (directory block, file block, skip block) with
	| Some d, Some f, false -> Some (File (Filename.concat d f))
	| Some d, None, false -> Some (Dir d)
	| None, Some f, false -> Some (File f)
	| None, None, false -> None
	| _, _, true -> None

[panglesd]

This PR in its original form included a new exec flag to execute included block, so the execution used to be opt-in instead of opt-out. I don't know if we should revert to this version @gpetiot ?

Wait! It’s not dune, it’s part of how mdx collects dependencies!

Well spotted, thanks!

[gpetiot]

This PR in its original form included a new exec flag to execute included block, so the execution used to be opt-in instead of opt-out. I don't know if we should revert to this version @gpetiot ?

We can but it's gonna take some sweat since the commits have been squashed to clean up the history.

[panglesd]

The commit wit the exec flag, before the force push, is panglesd@85b9142