Protect sites names against reserved keywords

[Lelio-Brun]

Expected Behavior

Compilation success.

Actual Behavior

Compilation error.

Reproduction

1. define a site whose name is a reserved Ocaml keyword, eg.,
   dune-project:

   (package
    (name my-package)
    (sites (lib include))
   

   dune:

   (generate_sites_module
    (module sites)
    (sites my-package))
   ...
   

2. dune build generates and tries to build the following Sites.ml:

   module Sites = struct
       let include = Dune_site.Private_.Helpers.site
         ~package:"my-package"
         ~section:Dune_section.Lib
         ~suffix:"include"
         ~encoded:(Sys.opaque_identity "%%DUNE_PLACEHOLDER:...%%")
   end

3. Compilation obviously fails with a syntax error.

Specifications

• Version of dune (output of dune --version): 2.8.2
• Version of ocaml (output of ocamlc --version): 4.11.1
• Operating system (distribution and version): Ubuntu 20.04.2

[bobot]

Thank you a lot for the report.

I don't know how to accept those name for sites, and have a predictable value name in the generated module. We can add an underscore at the end: include_. But it could clash if someone defines a site named include_ and include. However I would like to accept a site named include because it is a very sensible name.

So possible Fix1:

• Accept OCaml keyword, but the are suffixed by _ in the OCaml module.
• Forbid to define sites named as an OCaml keyword suffixed by _.

NTS list of OCaml keywords

[Lelio-Brun]

What about suffixing all sites variable by eg., _site ?
It would be a breaking change though.

[rgrinberg]

What about changing the api to be:

val get : string -> site

and the generated code to be:

let sites =
  [ "name", Dune_site.Private_.Helpers.site ..
  ; ..
  ]

You can keep aliases for site names that are valid identifiers for backwards compatibility as well.

[Lelio-Brun]

Seems good!

[bobot]

By doing so we replace a compile-time error into an error at execution if the site doesn't exists.

It would be a breaking change though.

It is not a problem, site is an experimental feature and 3.0 is coming.

What about suffixing all sites variable by eg., _site ?

I prefer that. Another possibility is that module name have less limitations (no keyword capitalized). So Sites.Foo.path is possible and let us extend later with some other values if we need.

[Lelio-Brun]

By doing so we replace a compile-time error into an error at execution if the site doesn't exists.

Ah yes, that's right.

[aalekseyev]

One could also have a rule like this:
If the original name is a keyword followed by any number of underscores, then it's extended by one underscore.

So

"foo" => "foo"
"include" => "include_"
"include_" => "include__"

the advantages are that it is injective while continuing the pattern @bobot proposed in FIX1.
The reason that pattern is worth continuing is that that's the de-facto idiom I've seen people use.

Underscore at the beginning has an additional meaning of "suppress unused variable warning", so I slightly prefer if that's avoided. One could consider unconditionally adding "_site" at the end if we're looking for a user-friendly uniform mapping.

(please take all that with a grain of salt because I know nothing about sites and how their names are to be used)