Display atomic annotation

[ElectreAAS]

Resolves #1405.

This PR simply parses the annotation from the cm(t)i, adds it to TypeDecl.Field.t, and displays it.

I added two simple tests showcasing the feature.

This is my first PR on this project, tell me if I'm doing something stupid

[ElectreAAS]

I added the field in cmi.ml#L1073, but it looks like the function is never called (replacing has_atomic with a failwith doesn't raise in any test.) Is it dead code?

[ElectreAAS]

cc @Leonidas-from-XIV

[Leonidas-from-XIV]

I've researched this and the reason why your test with the implementation (annotation_impl.ml) worked immediately is because the module that handles that is Cmt, which calls the Cmti version: https://github.com/ElectreAAS/odoc/blob/9cfcba5fe70eeb8ef89faeb93c2cba91832bacb4/src/loader/cmt.ml#L540. So that's one mystery from yesterday down.

I've looked into the implementation in Cmi and it is used at least in some places. When you one of the examples I found was when you define an exception with an anonymous record in an .ml file:

exception Atomic_exn of {mutable value : int [@atomic]}

It also would make sense to have a test the case where the record is anonymously attached to a constructor:

type with_annot = Atomic_constr of {mutable value : int [@atomic]}

Also, make sure to add a changelog entry.

[Leonidas-from-XIV]

Looking at the language extensions document it states that both @atomic and @ocaml.atomic are valid, so I think has_atomic should support both (and there should be a test). I think that change is required.

Another possible improvement is to change the type of mutable_. If we change it from bool to an ADT like Immutable | Mutable | Atomically_mutable this could prevent the invalid state where a value is marked as atomic but not mutable. I tested it in the toplevel and the compiler accepts sucha a configuration just fine (and presumably ignores it) but I would argue that it makes sense for us to represent nonsensical states in the docs.

[ElectreAAS]

About making an ADT: I tried my hand at this, and pushed a commit on a separate branch over here -> ElectreAAS@a02bb07

It doesn't compile partially for oxcaml reasons and partly because there seems to be a conversion problem in model_desc/lang_desc
Will look over this again on monday

[Leonidas-from-XIV]

I just took a look and it seems like a good approach!

[jonludlam]

Thanks @ElectreAAS - looking good so far! I've got a few minor comments though:

The annotation is only useful from 5.4+ (and in OxCaml), but we're accepting it in all versions of the compiler. Related to that, as it's meaningful in the compilers that do support the annotation, there's actually a field in the Typedtree types that we can use rather than trying to string match on the name itself (https://github.com/ocaml/ocaml/blob/5.4.0/typing/typedtree.mli#L754). Also, OxCaml has helpfully already done the 'mutable atomic' ADT here: https://github.com/oxcaml/oxcaml/blob/13103cbeb7ae578518d0a3bb3e5c113029000daa/typing/types.mli#L35-L41

Incidentally, as it happens, I think the compiler rejects putting the atomic annotation on a non-mutable field, so I don't think we would ever see the problematic combination.

Lastly, I don't think the atomic annotation is printed at the "right place" in the output. In particular, I think it comes after the modalities rather than before:

OCaml version 5.2.0+ox
Enter #help;; for help.

# type t = { mutable x : string [@atomic] };;
type t = { mutable x : string [@atomic]; }
# type t = { mutable x : string @@ portable [@atomic] };;
type t = { mutable x : string @@ portable [@atomic]; }
# type t = { mutable x : string [@atomic] @@ portable };;
Error: Syntax error

Other than that, looking good!

[Leonidas-from-XIV]

My experimentation with both the toplevel and ocamlc has been that the compiler just silently ignores [@atomic] on immutable fields. At least that is the case with 5.3.0, but that would make sense since generally the compiler is ignoring extension points it doesn't know about and that's one way to keep code backwards compatible. I assume the same is true for the tailcall optimization annotation.