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.

Sign up for GitHub

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

Jump to bottom

feat(describe-pp): print reason files with refmt #10322

Merged
merged 3 commits into from
Mar 29, 2024
Merged

feat(describe-pp): print reason files with refmt #10322

merged 3 commits into from
Mar 29, 2024

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Mar 28, 2024 •
edited
Loading

  • this initial diff has refmt hardcoded in bin/describe_pp.ml
  • It might be better to add a new field to the dialect and model this use case in the dialect directly too

@anmonteiro anmonteiro requested review from rgrinberg and emillon March 28, 2024 05:03
rgrinberg
rgrinberg reviewed Mar 28, 2024
View reviewed changes
test/blackbox-tests/test-cases/describe/describe-pp/describe-pp.t/run.t
@@ -16,4 +16,6 @@ We also make sure that the dump file is not present
This also works for reason code

$ dune describe pp src/main_re.re
;;Util.log "Hello, world!"
# 1 "_build/default/src/main_re.pp.re.ml"
# 1 "src/main_re.pp.re"
Copy link
Member

Choose a reason for hiding this comment

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

How come we're printing the location directives now?

Copy link
Collaborator Author

@anmonteiro anmonteiro Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the tests have a dedicated, fake refmt executable that I changed to match the real refmt by printing to stdout

@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-refmt branch 3 times, most recently from 33f1c35 to 909b6cc Compare March 28, 2024 21:41
@anmonteiro
Copy link
Collaborator Author

  • It might be better to add a new field to the dialect and model this use case in the dialect directly too

I changed the initial implementation to this:

  • added a field dump_ast in dialects
  • described those actions for OCaml and Reason
  • in describe pp, execute the dialect's dump_ast if it exists, falling back to OCaml's, known to exist

@rgrinberg rgrinberg requested a review from nojb March 28, 2024 22:05
rgrinberg
rgrinberg approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM. @nojb could you take a look at the dialect changes?

@anmonteiro
feat(describe-pp): use the dialect printer if available
3e4e152 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-refmt branch from 491b4cf to 3e4e152 Compare March 28, 2024 22:16
@anmonteiro
rename dump_ast to print_ast
51d5e5b 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Collaborator Author

I renamed dump_ast to print_ast, since dump_ast was ambiguous (could also be interpreted as "dump AST to a file")

@anmonteiro
Copy link
Collaborator Author

perhaps print_ast should be versioned?

nojb
nojb approved these changes Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

The dialect changes LGTM. Thanks!

@anmonteiro anmonteiro enabled auto-merge (squash) March 29, 2024 21:59
@anmonteiro anmonteiro merged commit 122c022 into ocaml:main Mar 29, 2024
1 of 2 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/describe-pp-refmt branch March 29, 2024 21:59
This was referenced Mar 30, 2024
Merged
Closed
@rgrinberg rgrinberg added this to the 3.16.0 milestone Apr 1, 2024
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
@anmonteiro
feat(describe-pp): print reason files with refmt (ocaml#10322)
bdd21ea 
* feat(describe-pp): use the dialect printer if available

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

* rename `dump_ast` to `print_ast`

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

---------

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@gridbugs gridbugs mentioned this pull request Jun 5, 2024
Closed
gridbugs added a commit to gridbugs/opam-repository that referenced this pull request Jun 5, 2024
@gridbugs
[new release] dune (15 packages) (3.16.0~alpha1)
0858f47 
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 17, 2024
@emillon
[new release] dune (15 packages) (3.16.0)
a6069b9 
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

- virtual libraries: fix an issue where linking an executable involving several
  virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)
@emillon emillon mentioned this pull request Jun 17, 2024
Merged
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
@emillon @avsm
[new release] dune (15 packages) (3.16.0)
4423f45 
CHANGES:

### Added

- allow libraries with the same `(name ..)` in projects as long as they don't
  conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro,
  @jchavarri)

- `dune describe pp` now finds the exact module and the stanza it belongs to,
  instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro)

- Print the result of `dune describe pp` with the respective dialect printer.
  (ocaml/dune#10322, @anmonteiro)

- Add new flag `--context` to `dune ocaml-merlin`, which allows to select a
  Dune context when requesting Merlin config. Add `dune describe contexts`
  subcommand. Introduce a field `generate_merlin_rules` for contexts declared
  in the workspace, that allows to optionally produce Merlin rules for other
  contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri)

- melange: add include paths for private library `.cmj` files during JS
  emission. (ocaml/dune#10416, @anmonteiro)

- `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`,
  `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the
  index(es). (ocaml/dune#10422, @voodoos)

- Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate
  indexes that can be read by tools such as Merlin to provide project-wide
  references search. (ocaml/dune#10422, @voodoos)

- merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to
  configure a merlin reader (ocaml/dune#8567, @andreypopp)

### Changed

- melange: treat private libraries with `(package ..)` as public libraries,
  fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415,
  @anmonteiro)

- install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego)

### Fixed

- Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes
  ocaml/dune#10056, @jonludlam)

- Make `dune-site`'s `load_all` function look for `META` files so that it
  doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes
  ocaml/dune#10457, @shym)

- Fix incorrect warning for libraries defined inside non-existant directories
  using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525,
  @rgrinberg)

- Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes
  ocaml/dune#7671, @lzy0505)

- Make sure to truncate dune's lock file after locking and unlocking so that
  users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg)

- mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with
  foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

- virtual libraries: fix an issue where linking an executable involving several
  virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@nojb nojb nojb approved these changes

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@anmonteiro @nojb @rgrinberg