Integration with auto formatters #1252

emillon · 2018-09-11T16:16:55Z

This adds a "fmt" extension in dune-project files. When used, it will setup a @fmt alias that will call ocamlformat on ocaml source code, and refmt on reason source code. The tools are not configured by dune.

📝 TODO

emillon · 2018-09-11T16:23:53Z

I'm having trouble with paths and directories: I'm using Module.iter to get the file paths, but when generating the rules in terms of these paths, I get the following error: Error: No rule found for lib/_build/default/lib/lib.mli (where as I think that I'm after lib/lib.mli).

I see that in Preprocessing, there's some chdir involved, but I naively tried to copy this and it outputs the same error:

dune/src/preprocessing.ml

Line 474 in 5745c8a

let action = Action.Unexpanded.Chdir (workspace_root_var, action) in

Also, that's probably related, but I'd like to have dir/x.ml generate a formatted version to dir/x.ml.formatted, but it's impossible because an alias can only write to a single directory. I used digests for the filename instead, but is there a better solution?

Thanks!

hhugo · 2018-09-11T16:20:37Z

src/ocamlformat.ml

+    let formatted = formatted_path path in
+    let format_action =
+      let args = [sv flag; dep path] in
+      Action.Unexpanded.Redirect


Is this better than having ocamlformat write down the file directly ?

hhugo · 2018-09-11T16:27:12Z

src/dune_file.ml

@@ -678,6 +678,7 @@ module Buildable = struct
    ; ocamlopt_flags           : Ordered_set_lang.Unexpanded.t
    ; js_of_ocaml              : Js_of_ocaml.t
    ; allow_overlapping_dependencies : bool
+    ; ocamlformat : bool


Should this be a property of libs and binary or just of the current directory ?
also, You might want to be able to add exceptions, useful for automatically generated source files

it's now possible to use OSL to perform exclusions.
per directory would work, but we already have a mapping between files and modules in buildables, so that seems to be the correct abstraction (at least it's the simplest one to start)

hhugo · 2018-09-11T16:31:11Z

src/lib_rules.ml

@@ -374,6 +374,7 @@ module Gen (P : Install_rules.Params) = struct
        ~lib_name:(Some lib.name)
        ~dir_kind
    in
+    Ocamlformat.gen_rules sctx lib.buildable ~dir ~scope modules;


Is this used for ocaml files only or also for reasonML files ?

now, this ignores reason files

emillon · 2018-09-11T16:35:55Z

@hhugo at the moment the interface is not defined yet, I just pushed the WIP branch for advice on how to deal with paths :)

hhugo · 2018-09-11T16:50:37Z

Well, you have some early review now

emillon · 2018-09-12T06:52:51Z

Well, nothing a good night of sleep couldn't fix. I just passed the basename and it now works. I'll ping the PR when it's ready for more feedback :) Sorry for the noise! (& thanks for the early review!)

emillon · 2018-09-13T07:15:02Z

I believe that this is ready for a round of review.

Some outstanding problems/questions:

is it ok to just pick a new alias? @format is pretty specific, but this would break user code if some dune files already use this alias.
this is enabled within buildable stanzas (library/executable), but per directory (ie per dune file) would work as well. It's just harder to list the source files per directory compared to per stanza, I believe.
in current versions of ocamlformat (0.7), there is a risk of configuration proliferation: if there is no .ocamlformat file in the source tree, a more global one will be used (see Isolate configuration files ocaml-ppx/ocamlformat#393). Should we wait until a release is cut and hardcode --root? Or detect the version with best effort and add the flag if supported?

Thanks!

samoht · 2018-09-13T07:28:53Z

I think I prefer dune build @fmt or dune fmt instead of @format (e.g. like go fmt)

Also could this just be enabled by default on all the libraries/binaries (e.g. without the need for a specific stanza) so you could just write dune fmt on an existing project and it will just reformat your project?

ghost · 2018-09-13T10:20:16Z

It seems to me that per-directory makes more sense. It's more a property of the source files themselves rather than individual libraries/executables. In fact, personally I would prefer to enable it once and for all for a given project. To get the list of files to format, we can simply look in the file tree. We don't need to bother with generated files since we clearly don't want to format them.

hcarty · 2018-09-13T12:19:38Z

Once this is in, would it be possible to have the same support for Reason syntax and refmt? It would be very helpful to be consistent between the two syntaxes.

Aa far as enabling formatting by default, ocamlformat can be disabled per-file with an attribute. So that provides an option for users who need a file or few to remain untouched by dune fmt.

emillon · 2018-09-19T15:10:55Z

Thanks for your feedback!

I changed this so that it works as a stanza, per directory. This uses Dir_contents, so as I understand it, will only work for in-tree source files (which seems to be the correct thing to do). I kept the OSL field for exclusions.

I suggest the following plan to generalize this a bit:

rename the stanza (format)
call refmt on reason files
auto-format dune files (using what's called dune unstable-fmt at the moment, renaming it to dune format-dune-file?)
add some configuration option to determine what autoformatters are enabled, so that one can e.g. only format dune files. This might replace the current OSL field.
add a new command dune fmt which calls dune build @format - reusing the current convention for dune runtest. Should we call the alias @fmt, then? (the command fmt convention is well adopted now)

This plan would obviously span several PRs, but it's great if we all agree on the direction to take.

avsm · 2018-09-19T15:20:13Z

Why do we need a (format) stanza at all? It seems useful to just make this alias available unconditionally on valid OCaml files. One cool usecase would be to then run dune fmt on a large monorepo like the OCaml Platform one and have it format all the different projects consistently :)

ghost · 2018-09-19T16:36:02Z

For the CLI, I suggest to have a single sub-command to format all kinds of files. dune fmt sounds good. We can make it take an optional filename to reformat a single file. I agree that the alias and command name should match, so dune fmt and @fmt sounds good.

I agree with @avsm that we shouldn't need one stanza per directory, except when there are exceptions. We might want to allow to opt-in or opt-out via the dune-project file though, so that in a mono-repo dune can skip projects that don't want to be auto-formatted. I suggest to make it opt-in via the dune-project file for now, to be conservative, and opt-out with (lang dune 2.0).

emillon · 2018-09-20T08:33:06Z

OK! Let's make this less hidden and more global. I'll deal with the CLI part in a separate PR.

I am not sure how to add new features through dune-project. At the moment there is only lang, name and version in these files. Adding fmt here seems very specific. Should I add a new syntax and (using fmt)? Does (using) enable new syntax in dune-project itself?

Thanks!

ghost · 2018-09-20T09:02:11Z

Yh, the idea is to add (using fmt <version>). Currently (using) only enables new stanzas in dune files, though you can pass parameters to the using stanza:

(using fmt 1.0
 (enabled_for ocaml reason dune))

In this case, rather than or in addition to adding a new stanza in dune files, we probably want to also be able to just check for the presence of this stanza in the dune-project file. I suppose we could change the API of Dune_project.Extension as follow:

module Extension : sig
  type 'a t

  (** [register id parser] registers a new extension. Users will
      enable this extension by writing:

      {[ (using <name> <version> <args>) ]}

      in their [dune-project] file. [parser] is used to describe
      what [<args>] might be. *)
  val register
    :  ?experimental:bool
    -> Syntax.t
    -> ('a * Stanza.Parser.t list) Dsexp.Of_sexp.t
    -> 'a t
end

val get_extension : t -> 'a Extension.t -> 'a option

emillon · 2018-09-20T12:49:38Z

I just did that. For now it just returns the loc but I'll extend that to refmt and add enabled_for.

ghost · 2018-09-20T13:28:18Z

src/format_rules.ml

+    let msg = "Cannot find ocamlformat, skipping.\n" in
+    add_alias (Echo [sv msg])
+  | Some ocamlformat ->
+    let files = Dir_contents.text_files dir_contents in


This includes generated files. We really need to look into the file tree for the actual files to reformat.

Done. I used Path.drop_build_context_exn which works, is that the correct function to use?

ghost · 2018-09-20T13:29:17Z

src/dune_project.ml

@@ -215,6 +215,7 @@ module Extension = struct
    }

  let extensions = Hashtbl.create 32
+  let instanciated_extensions = Hashtbl.create 32


This should be per-project, otherwise it won't work when there are several projects in the same workspace.

It's now stored in the project.

ghost · 2018-09-20T13:31:33Z

BTW, could you benchmark this change? The platform repository should be a good test case. Given that this more work that we will do systematically, we need to know what the cost is.

emillon · 2018-09-20T13:45:01Z

Yes, I'm adding a cache so that we only call which on each formatter as well. But a benchmark will be useful!

emillon · 2018-09-21T09:56:07Z

I:

added a lazy around the which calls
extended to refmt

Next I'll add the configuration syntax and make the instanciation table per project.

emillon · 2018-09-21T10:04:12Z

About testing: rather than installing ocamlformat & refmt, I was thinking that I could bring "fake" binaries in PATH (e.g. prepending each line with a prefix + displaying sys.argv in a header), what do you think about that?

ghost · 2018-09-28T21:59:58Z

Ah ok, I get why we pass the file in the source tree via --name now. However, in case of error the filename reported in the error message will be odd. Why not add dependencies on all the .ocamlformat files up to the root of the workspace?

ghost · 2018-09-28T22:03:07Z

We can use Build.paths_glob or Build.if_file_exists to add a dependency only if the file exists.

emillon · 2018-10-01T09:37:59Z

Build.paths_glob seems to be a nice solution, but if you don't mind can we do this in a separate PR? The current solution is already "correct" in the sense that we pick the correct files - just the incremental aspect can be improved. This PR has been opened for 3 weeks, I'd be grateful if we could merge the main part before improving it. Thanks!

ghost · 2018-10-01T11:27:44Z

Sure, sorry about the nitpicking.

emillon · 2018-10-01T14:39:33Z

No problem! I filed two bugs so that I don't forget about this.

This adds a "fmt" extension in dune-project files. When used, it will setup a `@fmt` alias that will call `ocamlformat` on ocaml source code, and `refmt` on reason source code. The tools are not configured by dune. Closes #1201 Signed-off-by: Etienne Millon <me@emillon.org>

emillon · 2018-10-02T07:29:02Z

Thanks for the thorough review!

ghost · 2018-10-02T10:22:58Z

No problem, should we switch Dune to ocamlformat then? :)

emillon · 2018-10-02T10:27:53Z

Yes! I've been running some tests about which configuration yields the smallest diff. I think our style corresponds to the "sparse" profile with some minor addition. I'll run the tests and post the results in a new issue.

@diml

CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb)

@diml

CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb) - Take argument to self_build_stubs_archive into account. (ocaml/dune#1395, @nojb) - Fix bad interaction between `env` customization and vendored projects: when a vendored project didn't have its own `env` stanza, the `env` stanza from the enclosing project was in effect (ocaml/dune#1408, @diml) - Fix stop early bug when scanning for watermarks (ocaml/dune#1423, @diml)

This adds a "fmt" extension in dune-project files. When used, it will setup a `@fmt` alias that will call `ocamlformat` on ocaml source code, and `refmt` on reason source code. The tools are not configured by dune. Closes ocaml#1201 Signed-off-by: Etienne Millon <me@emillon.org>

hhugo · 2019-01-23T09:15:15Z

I've mentioned that in a comment already (#1252 (comment)).
It would be nice to be able to specify exceptions on a per file basis. It's useful when a file do not parse (e.g file preprocessed with cppo) or when a file is automatically generated (e.g. by ocamllex, yacc, menhir, ...).

ghost · 2019-01-24T09:33:32Z

For generated files that are not promoted, we should probably disable auto-formatting automatically.

ghost · 2019-01-24T10:49:53Z

Actually, looking at the code that's already the case, so there should be no need to disable ocamlformat on files generated by ocamllex, yacc, ...

hhugo · 2019-01-24T11:07:08Z

My use case is with lexers and parsers files that are promoted.

emillon requested a review from a user September 11, 2018 16:16

emillon requested a review from rgrinberg as a code owner September 11, 2018 16:16

hhugo reviewed Sep 11, 2018

View reviewed changes

emillon force-pushed the ocamlformat branch from 7708457 to 7b01914 Compare September 12, 2018 06:51

emillon force-pushed the ocamlformat branch 2 times, most recently from 7501b9a to 6940d4a Compare September 13, 2018 07:08

emillon force-pushed the ocamlformat branch from 6940d4a to 0770939 Compare September 13, 2018 09:52

emillon force-pushed the ocamlformat branch from 0770939 to a886241 Compare September 19, 2018 15:01

emillon force-pushed the ocamlformat branch from a886241 to c8fa4c8 Compare September 20, 2018 12:47

ghost suggested changes Sep 20, 2018

View reviewed changes

emillon force-pushed the ocamlformat branch from c8fa4c8 to 08e74f2 Compare September 21, 2018 09:54

emillon force-pushed the ocamlformat branch from 08e74f2 to f97e929 Compare September 21, 2018 09:59

emillon force-pushed the ocamlformat branch from 643b3c1 to 5be81a5 Compare October 1, 2018 09:31

ghost approved these changes Oct 1, 2018

View reviewed changes

emillon force-pushed the ocamlformat branch 2 times, most recently from 6f911d3 to 56f1e98 Compare October 1, 2018 13:40

This was referenced Oct 1, 2018

Formatters: ocamlformat only depends on .ocamlformat in the current dir #1369

Closed

ocamlformat is passed a path relative to the build dir #1370

Closed

emillon force-pushed the ocamlformat branch from 56f1e98 to e5a30c3 Compare October 1, 2018 16:18

emillon force-pushed the ocamlformat branch from e5a30c3 to f7a3d0d Compare October 2, 2018 07:01

emillon merged commit 928024b into master Oct 2, 2018

emillon deleted the ocamlformat branch October 2, 2018 07:28

rgrinberg mentioned this pull request Oct 4, 2018

[new release] dune (1.4.0) ocaml/opam-repository#12744

Closed

rgrinberg mentioned this pull request Oct 10, 2018

[new release] dune (1.4.0) ocaml/opam-repository#12774

Merged

hhugo mentioned this pull request Jan 31, 2019

allow some files to not be reformatted with @fmt #1793

Closed

kit-ty-kate mentioned this pull request Dec 19, 2019

[new release] routes (0.6.0) ocaml/opam-repository#15565

Merged

Integration with auto formatters #1252

Integration with auto formatters #1252

Conversation

emillon commented Sep 11, 2018 • edited Loading

📝 TODO

emillon commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emillon commented Sep 11, 2018

hhugo commented Sep 11, 2018

emillon commented Sep 12, 2018

emillon commented Sep 13, 2018

samoht commented Sep 13, 2018 • edited Loading

ghost commented Sep 13, 2018

hcarty commented Sep 13, 2018

emillon commented Sep 19, 2018

avsm commented Sep 19, 2018

ghost commented Sep 19, 2018

emillon commented Sep 20, 2018

ghost commented Sep 20, 2018

emillon commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 20, 2018

emillon commented Sep 20, 2018

emillon commented Sep 21, 2018

emillon commented Sep 21, 2018

ghost commented Sep 28, 2018

ghost commented Sep 28, 2018

emillon commented Oct 1, 2018

ghost commented Oct 1, 2018

emillon commented Oct 1, 2018

emillon commented Oct 2, 2018

ghost commented Oct 2, 2018

emillon commented Oct 2, 2018

hhugo commented Jan 23, 2019

ghost commented Jan 24, 2019

ghost commented Jan 24, 2019

hhugo commented Jan 24, 2019

emillon commented Sep 11, 2018 •

edited

Loading

samoht commented Sep 13, 2018 •

edited

Loading