Skip to content

letop-punning for extension nodes#2747

Merged
EmileTrotignon merged 3 commits intoocaml-ppx:mainfrom
WardBrian:feat/let-punning-for-extensions
Nov 6, 2025
Merged

letop-punning for extension nodes#2747
EmileTrotignon merged 3 commits intoocaml-ppx:mainfrom
WardBrian:feat/let-punning-for-extensions

Conversation

@WardBrian
Copy link
Copy Markdown
Contributor

@WardBrian WardBrian commented Oct 30, 2025 •
edited
Loading

Follow-on to #2746, this makes it so the letop-punning setting also affects nodes like let%foo bar = bar in ...

The trickiest piece here is that the ands for these nodes don't have the extension, so I needed to do this at the level of value_bindings, not value_binding

This also fixes an issue with comments when a pun is introduced. As far as I can tell, this was also an issue back in 0.26, when the equivalent of letop-punning=always was the default behavior.

github-actions[bot]
github-actions Bot reviewed Oct 30, 2025
View reviewed changes
Comment thread CHANGES.md Outdated
@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Oct 30, 2025

(note I commented in #2746 that I thought the preserve feature would require parser changes, but it turns out the parser already was tracking this, my tests were just lacking any examples of it)

@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Oct 31, 2025

Hi @EmileTrotignon - I think this is ready for review. The let%ext change is very similar to the code from the previous PR and should be an easy review.

A bit trickier is I noticed that letop-punning=always could lead to errors if the right hand side had associated comments. I believe I found a fix (for both this new code and the existing), but I'm less confident in that part and would appreciate any pointers

@EmileTrotignon
Copy link
Copy Markdown
Collaborator

EmileTrotignon commented Oct 31, 2025

This looks good. Where is the fix about comments ?

@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Oct 31, 2025

The way I tackled the comment issue is by copying the location of the rhs into the pattern (starting in efb3931). I’m not sure if this is the recommended way or not, but it does appear to work on the added test cases

Julow
Julow approved these changes Oct 31, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Awesome :)

Comment thread test/passing/tests/let_punning_denied.ml Outdated
Julow
Julow reviewed Oct 31, 2025
View reviewed changes
Comment thread lib/Extended_ast.ml Outdated
@WardBrian WardBrian mentioned this pull request Nov 3, 2025
Merged
@WardBrian WardBrian force-pushed the feat/let-punning-for-extensions branch from df1653d to 68d651a Compare November 3, 2025 15:47
@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Nov 3, 2025

@Julow I split out the testing and CONTRIBUTING changes into #2748 and then rebased this PR off of that one to try
to make review easier.

I'm now calling Cmts.relocate in Sugar.ml, which seems to be working well

EmileTrotignon
EmileTrotignon reviewed Nov 3, 2025
View reviewed changes
Comment thread CONTRIBUTING.md Outdated
EmileTrotignon
EmileTrotignon reviewed Nov 3, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@EmileTrotignon EmileTrotignon left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contributing documentation, its a very big improvement.

Comment thread CONTRIBUTING.md
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
@EmileTrotignon
Copy link
Copy Markdown
Collaborator

EmileTrotignon commented Nov 3, 2025

Sorry I reviewed the contribution.md changes in the wrong PR

@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Nov 3, 2025

No worries @EmileTrotignon. I've pushed updates to #2748

(stacked PRs still need a better workflow for github -- the diffs quickly diverge without a lot of ugly force pushing)

@WardBrian WardBrian force-pushed the feat/let-punning-for-extensions branch from 68d651a to 18c3a3f Compare November 3, 2025 16:17
@WardBrian WardBrian requested a review from Julow November 3, 2025 19:22
@WardBrian WardBrian force-pushed the feat/let-punning-for-extensions branch from 18c3a3f to 1fe36f0 Compare November 3, 2025 19:24
Julow
Julow approved these changes Nov 4, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Looks perfect :) Let's merge just after #2748

@EmileTrotignon
Copy link
Copy Markdown
Collaborator

EmileTrotignon commented Nov 5, 2025

This can be merged after a rebase on main

WardBrian and others added 3 commits November 5, 2025 10:38
@WardBrian
letop-punning for extension nodes
f989f1a
@WardBrian @github-actions
Update CHANGES.md
6c382ba 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@WardBrian
Fix comments getting dropped by letop-punning=always
425ecc4
@WardBrian WardBrian force-pushed the feat/let-punning-for-extensions branch from 1fe36f0 to 425ecc4 Compare November 5, 2025 15:40
@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Nov 5, 2025

Rebased :)

@EmileTrotignon
Copy link
Copy Markdown
Collaborator

EmileTrotignon commented Nov 5, 2025

Waiting for CI and merging

@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Nov 5, 2025

CI has passed. Thanks for the hand-holding on this feature!

@EmileTrotignon EmileTrotignon merged commit 295fe67 into ocaml-ppx:main Nov 6, 2025
11 of 12 checks passed
@EmileTrotignon
Copy link
Copy Markdown
Collaborator

EmileTrotignon commented Nov 6, 2025

@WardBrian You're very welcome ! If you are interested in contributing further to ocamlformat, maybe we could have a chat about the project.

@WardBrian WardBrian deleted the feat/let-punning-for-extensions branch November 7, 2025 15:07
@WardBrian
Copy link
Copy Markdown
Contributor Author

WardBrian commented Nov 7, 2025

Sure @EmileTrotignon! I've been a happy user of ocamlformat for years, and this is the first time I found a gap in my needs for it that prompted me to contribute, but I have some spare cycles to do more here or there

Julow added a commit to Julow/opam-repository that referenced this pull request Mar 17, 2026
@Julow
[new release] ocamlformat (4 packages) (0.29.0)
f62a590 
CHANGES:

### Highlight

- \* Support OCaml 5.5 syntax
  (ocaml-ppx/ocamlformat#2772, ocaml-ppx/ocamlformat#2774, ocaml-ppx/ocamlformat#2775, ocaml-ppx/ocamlformat#2777, ocaml-ppx/ocamlformat#2780, ocaml-ppx/ocamlformat#2781, ocaml-ppx/ocamlformat#2782, ocaml-ppx/ocamlformat#2783, @Julow)
  The update brings several tiny changes, they are listed below.

- \* Update Odoc's parser to 3.0 (ocaml-ppx/ocamlformat#2757, @Julow)
  The indentation of code-blocks containing OCaml code is reduced by 2 to avoid
  changing the generated documentation. The indentation within code-blocks is
  now significative in Odoc and shows up in generated documentation.

### Added

- Added option `letop-punning` (ocaml-ppx/ocamlformat#2746, @WardBrian) to control whether
  punning is used in extended binding operators.
  For example, the code `let+ x = x in ...` can be formatted as
  `let+ x in ...` when `letop-punning=always`. With `letop-punning=never`, it
  becomes `let+ x = x in ...`. The default is `preserve`, which will
  only use punning when it exists in the source.
  This also applies to `let%ext` bindings (ocaml-ppx/ocamlformat#2747, @WardBrian).

- Support the unnamed functor parameters syntax in module types
  (ocaml-ppx/ocamlformat#2755, ocaml-ppx/ocamlformat#2759, @Julow)
  ```ocaml
  module type F = ARG -> S
  ```
  The following lines are now formatted as they are in the source file:
  ```ocaml
  module M : (_ : S) -> (_ : S) -> S = N
  module M : S -> S -> S = N
  (* The preceding two lines are no longer turned into this: *)
  module M : (_ : S) (_ : S) -> S = N
  ```

### Fixed

- Fix dropped comment in `(function _ -> x (* cmt *))` (ocaml-ppx/ocamlformat#2739, @Julow)

- \* `cases-matching-exp-indent=compact` does not impact `begin end` nodes that
  don't have a match inside. (ocaml-ppx/ocamlformat#2742, @EmileTrotignon)
  ```ocaml
  (* before *)
  begin match () with
  | () -> begin
    f x
  end
  end
  (* after *)
  begin match () with
  | () -> begin
      f x
    end
  end
  ```

- `Ast_mapper` now iterates on *all* locations inside of Longident.t,
  instead of only some.
  (ocaml-ppx/ocamlformat#2737, @v-gb)

- Remove line break in `M with module N = N (* cmt *)` (ocaml-ppx/ocamlformat#2779, @Julow)

### Internal

- Added information on writing tests to `CONTRIBUTING.md` (ocaml-ppx/ocamlformat#2838, @WardBrian)

### Changed

- indentation of the `end` keyword in a match-case is now always at least 2. (ocaml-ppx/ocamlformat#2742, @EmileTrotignon)
  ```ocaml
  (* before *)
  begin match () with
  | () -> begin
    match () with
    | () -> ()
  end
  end
  (* after *)
  begin match () with
  | () -> begin
    match () with
    | () -> ()

- \* use shortcut `begin end` in `match` cases and `if then else` body. (ocaml-ppx/ocamlformat#2744, @EmileTrotignon)
  ```ocaml
  (* before *)
  match () with
  | () -> begin
      match () with
      | () ->
    end
  end
  (* after *)
  match () with
  | () ->
    begin match () with
      | () ->
    end
  end
  ```

- \* Set the `ocaml-version` to `5.4` by default (ocaml-ppx/ocamlformat#2750, @EmileTrotignon)
  The main difference is that the `effect` keyword is recognized without having
  to add `ocaml-version=5.3` to the configuration.
  In exchange, code that use `effect` as an identifier must use
  `ocaml-version=5.2`.

- The work to support OCaml 5.5 come with several improvements:
  + Improve the indentation of `let structure-item` with the
    `[@ocamlformat "disable"]` attribute.
    `let structure-item` means `let module`, `let open`, `let include` and
    `let exception`.
  + `(let open M in e)[@A]` is turned into `let[@A] open M in e`.
  + Long `let open ... in` no longer exceed the margin.
  + Improve indentation of `let structure-item` within parentheses:
    ```ocaml
    (* before *)
    (let module M = M in
    M.foo)
    (* after *)
    (let module M = M in
     M.foo)
    ```
Julow added a commit to Julow/opam-repository that referenced this pull request Mar 17, 2026
@Julow
[new release] ocamlformat (4 packages) (0.29.0)
e67854d 
CHANGES:

### Highlight

- \* Support OCaml 5.5 syntax
  (ocaml-ppx/ocamlformat#2772, ocaml-ppx/ocamlformat#2774, ocaml-ppx/ocamlformat#2775, ocaml-ppx/ocamlformat#2777, ocaml-ppx/ocamlformat#2780, ocaml-ppx/ocamlformat#2781, ocaml-ppx/ocamlformat#2782, ocaml-ppx/ocamlformat#2783, @Julow)
  The update brings several tiny changes, they are listed below.

- \* Update Odoc's parser to 3.0 (ocaml-ppx/ocamlformat#2757, @Julow)
  The indentation of code-blocks containing OCaml code is reduced by 2 to avoid
  changing the generated documentation. The indentation within code-blocks is
  now significative in Odoc and shows up in generated documentation.

### Added

- Added option `letop-punning` (ocaml-ppx/ocamlformat#2746, @WardBrian) to control whether
  punning is used in extended binding operators.
  For example, the code `let+ x = x in ...` can be formatted as
  `let+ x in ...` when `letop-punning=always`. With `letop-punning=never`, it
  becomes `let+ x = x in ...`. The default is `preserve`, which will
  only use punning when it exists in the source.
  This also applies to `let%ext` bindings (ocaml-ppx/ocamlformat#2747, @WardBrian).

- Support the unnamed functor parameters syntax in module types
  (ocaml-ppx/ocamlformat#2755, ocaml-ppx/ocamlformat#2759, @Julow)
  ```ocaml
  module type F = ARG -> S
  ```
  The following lines are now formatted as they are in the source file:
  ```ocaml
  module M : (_ : S) -> (_ : S) -> S = N
  module M : S -> S -> S = N
  (* The preceding two lines are no longer turned into this: *)
  module M : (_ : S) (_ : S) -> S = N
  ```

### Fixed

- Fix dropped comment in `(function _ -> x (* cmt *))` (ocaml-ppx/ocamlformat#2739, @Julow)

- \* `cases-matching-exp-indent=compact` does not impact `begin end` nodes that
  don't have a match inside. (ocaml-ppx/ocamlformat#2742, @EmileTrotignon)
  ```ocaml
  (* before *)
  begin match () with
  | () -> begin
    f x
  end
  end
  (* after *)
  begin match () with
  | () -> begin
      f x
    end
  end
  ```

- `Ast_mapper` now iterates on *all* locations inside of Longident.t,
  instead of only some.
  (ocaml-ppx/ocamlformat#2737, @v-gb)

- Remove line break in `M with module N = N (* cmt *)` (ocaml-ppx/ocamlformat#2779, @Julow)

### Internal

- Added information on writing tests to `CONTRIBUTING.md` (ocaml-ppx/ocamlformat#2838, @WardBrian)

### Changed

- indentation of the `end` keyword in a match-case is now always at least 2. (ocaml-ppx/ocamlformat#2742, @EmileTrotignon)
  ```ocaml
  (* before *)
  begin match () with
  | () -> begin
    match () with
    | () -> ()
  end
  end
  (* after *)
  begin match () with
  | () -> begin
    match () with
    | () -> ()

- \* use shortcut `begin end` in `match` cases and `if then else` body. (ocaml-ppx/ocamlformat#2744, @EmileTrotignon)
  ```ocaml
  (* before *)
  match () with
  | () -> begin
      match () with
      | () ->
    end
  end
  (* after *)
  match () with
  | () ->
    begin match () with
      | () ->
    end
  end
  ```

- \* Set the `ocaml-version` to `5.4` by default (ocaml-ppx/ocamlformat#2750, @EmileTrotignon)
  The main difference is that the `effect` keyword is recognized without having
  to add `ocaml-version=5.3` to the configuration.
  In exchange, code that use `effect` as an identifier must use
  `ocaml-version=5.2`.

- The work to support OCaml 5.5 come with several improvements:
  + Improve the indentation of `let structure-item` with the
    `[@ocamlformat "disable"]` attribute.
    `let structure-item` means `let module`, `let open`, `let include` and
    `let exception`.
  + `(let open M in e)[@A]` is turned into `let[@A] open M in e`.
  + Long `let open ... in` no longer exceed the margin.
  + Improve indentation of `let structure-item` within parentheses:
    ```ocaml
    (* before *)
    (let module M = M in
    M.foo)
    (* after *)
    (let module M = M in
     M.foo)
    ```
@Julow Julow mentioned this pull request Mar 17, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EmileTrotignon EmileTrotignon EmileTrotignon left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@Julow Julow Julow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@WardBrian @EmileTrotignon @Julow