Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for "dialects" #2404

Merged
merged 47 commits into from
Jul 12, 2019
Merged

Add support for "dialects" #2404

merged 47 commits into from
Jul 12, 2019

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Jul 11, 2019

Following-up on #2373 and @diml's comment, we propose the introduction of a (dialect ...) stanza. Full form is:

(dialect
  (name <name>)
  (interface
    (extension <string>)
    (preprocess <action>)
    (format <action>))
  (implementation
    (extension <string>)
    (preprocess <action>)
    (format <action>)))
  • Fields preprocess and format are optional, in which case it is assumed that the files in question are already valid OCaml (and/or do not need special formatting).
  • Dialects are uniquely identified by their name on a given project
  • Each dialect has exactly two associated extensions: one for interfaces and one for implementations. The extensions are unique among all dialects of a project.

@nojb nojb requested a review from fpottier as a code owner July 11, 2019 06:43
@nojb
Copy link
Collaborator Author

nojb commented Jul 11, 2019

In the first few commits I added internal plumbing to reimplement the existing logic for OCaml/Reason (not yet the formatting part).

src/dialect.ml Outdated Show resolved Hide resolved
src/dialect.ml Show resolved Hide resolved
@nojb nojb requested a review from emillon as a code owner July 11, 2019 08:41
src/dialect.mli Show resolved Hide resolved
src/module.ml Outdated Show resolved Hide resolved
src/module.ml Outdated Show resolved Hide resolved
src/preprocessing.ml Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jul 11, 2019

I like the name dialect BTW :)

@emillon
Copy link
Collaborator

emillon commented Jul 11, 2019

If I understand it correctly, the interface expected for (format <action>) is using stdin/stdout. Unfortunately, some formatters like ocamlformat don't support this. Do you think there's a way to support that?

@nojb nojb force-pushed the dialects branch 2 times, most recently from 8e9252f to 2810ec2 Compare July 11, 2019 09:09
@nojb
Copy link
Collaborator Author

nojb commented Jul 11, 2019

If I understand it correctly, the interface expected for (format <action>) is using stdin/stdout. Unfortunately, some formatters like ocamlformat don't support this. Do you think there's a way to support that?

  • dune preprocessing actions write to stdout so it makes sense to do the same here
  • if you are formatting ocaml code (ie. you are not specifying a preprocess action for the dialect) you do not need to actually write the formatting action and dune knows how to invoke ocamlformat
  • why doesn't ocamlformat support writing on stdout ?? 😄

@ghost
Copy link

ghost commented Jul 11, 2019

@emillon it's only printing to stdout that's expected. The input file is passed via a variable %{input-file}. For instance:

(format (run ocamlformat %{input-file}))

@nojb
Copy link
Collaborator Author

nojb commented Jul 11, 2019

Ah, sorry for the confusion, I thought the question was about printing to stdout rather than reading from stdin.

src/dir_contents.ml Outdated Show resolved Hide resolved
src/format_rules.ml Outdated Show resolved Hide resolved
@nojb nojb force-pushed the dialects branch 6 times, most recently from d05c610 to 9613868 Compare July 11, 2019 16:34
@nojb
Copy link
Collaborator Author

nojb commented Jul 11, 2019

This is ready for review.

src/dialect.ml Outdated
Action_dune_lang.run (S.virt __POS__ "refmt")
[ S.virt __POS__ "--print"
; S.virt __POS__ "binary"
; S.virt_var __POS__ "input-file"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using __POS__ here is wrong because in case of error the location points to dune's source code. I guess we want to pass Loc.none somehow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want this, I think you can just use make_var and make_text. You can pass any location you want this way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did this change.


type enabled_for =
| Default of Syntax.Version.t
| Only of language list

let enabled_for_field =
let+ r = field_o "enabled_for" (repeat language)
let+ r = field_o "enabled_for" (repeat (map ~f:language string))
Copy link
Collaborator Author

@nojb nojb Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not validate that the language names used in the (enabled_for ...) field actually exists (i.e. are either dune or a dialect name). This validation has to happen after the dune-project file has been parsed, but the "extensions" API would need to be extended to allow doing such a thing.

src/dialect.mli Outdated

val preprocess : t -> Ml_kind.t -> (Loc.t * Action_dune_lang.t) option

val format : t -> Ml_kind.t -> (Loc.t * Action_dune_lang.t * string list) option
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string list is a list of "extra dependencies" for the formatting rule. This is only used to record the dependency of the ocamlformat rule on .ocamlformat and .ocamlformat-ignore, and is not exposed to the user.

src/dialect.ml Outdated Show resolved Hide resolved
src/format_rules.ml Outdated Show resolved Hide resolved
nojb and others added 4 commits July 12, 2019 13:39
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jul 12, 2019

Thanks!

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jul 12, 2019

I added some doc for the manual. I didn't know where to put it so I added a new top-level section (called Dialects). But the functionality is pretty minor, so it would probably be better to insert it inside some existing section.

nojb added 2 commits July 12, 2019 14:16
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jul 12, 2019

I added a few more tests. I will merge after the CI passes unless someone objects.

@nojb nojb closed this Jul 12, 2019
@nojb nojb reopened this Jul 12, 2019
@nojb nojb merged commit f185f7e into ocaml:master Jul 12, 2019

val ml_suffix : t -> Ml_kind.t -> string option

module S : sig
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to comment about this, but what do you think about calling this module DB? This is the name we used in other places for this kind of things:

  • Lib.DB
  • Scope.DB

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will include this change (and anything else that comes up) in #2411

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I had a quick look through the code. Apart from the comment about loc, it all looks good to me!

type t

val empty : t
val add : t -> ?loc:Loc.t -> dialect -> t
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to make loc required here. We can always pass Loc.none as a dummy value, but at least we have to think about what we are doing and whether it is justified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will do.

@bikallem
Copy link
Contributor

This is interesting. I am wondering if this could be used to implement a JSX syntax in ocaml without the use of ppx, i.e. JSX syntax which is supported as part of ocaml syntax (like in ReasonML). Here the scope of the dialect is just to convert JSX into function application and nothing more.

More information can be found in ocaml/ocaml#8802.

How would one go about doing it using the dialect support in ocaml? Is there a simple example repo that I can study/look?

Could the following be perhaps my starting point?

  1. Fork just the parsing bit from https://github.com/ocaml/ocaml/tree/trunk/parsing.
  2. Implement JSX syntax on top of it
  3. Implement AST transformation from JSX to ocaml AST.

@nojb
Copy link
Collaborator Author

nojb commented Jul 15, 2019

  1. Fork just the parsing bit from https://github.com/ocaml/ocaml/tree/trunk/parsing.
  2. Implement JSX syntax on top of it
  3. Implement AST transformation from JSX to ocaml AST.

That sounds like it should work; but maintaining your fork of the OCaml parser is probably going to be quite a bit of work.

@bikallem
Copy link
Contributor

That sounds like it should work; but maintaining your fork of the OCaml parser is probably going to be quite a bit of work.

Yep, that is my main worry too. It would have been nice if menhir supported these type of scenarios, i.e. inheriting the ocaml compiler parser without modifying it and only extending certain sections of it in separate .mly(?) file. Much like in OO how child classes can further specialize parent function/methods without having to maintain/modify the parent classes.

This would make the maintenance burden of small dialects like I am proposing much palatable. Any ideas suggestions if this is possible perhaps?

@ghost
Copy link

ghost commented Jul 16, 2019

Note that this is exactly what Camlp4 allowed to do; it allowed to extend the syntax of OCaml without forking the parser. Camlp4 is the predecessor of ppx. One issue with this approach is that we ended up with many different dialects and it was impossible for editors and tools to know them all. That's why we introduced the extension point and attribute syntax: to provide a uniform syntax for extending the language.

Given that, I wouldn't recommend extending the syntax in this way. Instead, you could design a syntax based on plain extension points, or embed the foreign syntax in a string: [%jsx{| <div> ... </div> |}].

@nojb nojb deleted the dialects branch July 16, 2019 09:25
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 18, 2019
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 22, 2019
CHANGES:

- Don't select all local implementations in `dune utop`. Instead, let the
  default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex,
  review by @rgrinberg)

- Check that selected implementations (either by variants or default
  implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by
  @rgrinberg)

- Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml)

- Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The
  set of variants is now computed when the virtual library is installed.
  Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex,
  review by @diml)

- Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275,
  @rgrinberg)

- Fix error message for missing C++ sources. The `.cc` extension was always
  ignored before. (ocaml/dune#2275, @rgrinberg)

- Add `$ dune init project` subcommand to create project boilerplate according
  to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder)

- Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo)

- Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review
  by @rgrinberg and @diml)

- Add a `cinaps` extension and stanza for better integration with the
  [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269,
  @diml)

- Allow to embed build info in executables such as version and list
  and version of statically linked libraries (ocaml/dune#2224, @diml)

- Set version in `META` and `dune-package` files to the one read from
  the vcs when no other version is available (ocaml/dune#2224, @diml)

- Add a variable `%{target}` to be used in situations where the context
  requires at most one word, so `%{targets}` can be confusing; stdout
  redirections and "-o" arguments of various tools are the main use
  case; also, introduce a separate field `target` that must be used
  instead of `targets` in those situations.  (ocaml/dune#2341, @aalekseyev)

- Fix dependency graph of wrapped_compat modules. Previously, the dependency on
  the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg)

- Allow to promote executables built with an `executable` stanza
  (ocaml/dune#2379, @diml)

- When instantiating an implementation with a variant, make sure it matches
  virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322,
  @TheLortex, review by @rgrinberg)

- Add a variable `%{ignoring_promoted_rules}` that is `true` when
  `--ingore-promoted-rules` is passed on the command line and false
  otherwise (ocaml/dune#2382, @diml)

- Fix a bug in `future_syntax` where the characters `@` and `&` were
  not distinguished in the names of binding operators (`let@` was the
  same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml)

- Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325,
  @rgrinberg)

- Improve opam generation to include the `dune` dependncies with the minimum
  constraint set based on the dune language version specified in the
  `dune-project` file. (2383, @avsm)

- The order of fields in the generated opam file now follows order preferred in
  opam-lib. (@avsm, ocaml/dune#2380)

- Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384)

- Add warning `66` to default set of warnings starting for dune projects with
  language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299)

- Add (dialect ...) stanza
  (@nojb, ocaml/dune#2404)

- Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412)

- Do not warn about merlin files pre 1.9. This warning can only be disabled in
  1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)

- Add a new `inline_tests` field in the env stanza to control inline_tests
  framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review
  by @rgrinberg).

- New binary kind `js` for executables in order to explicitly enable Javascript
  targets, and a switch `(explicit_js_mode)` to require this mode in order to
  declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants