Attach infix extension points/attributes on local module/exception/open to structure item instead of enclosing let expression by nojb · Pull Request #14009 · ocaml/ocaml

nojb · 2025-05-02T07:21:13Z

This is an alternative to #14005 that implements the change in semantics mentioned in #14005 (comment).

Concretely, before this PR, infix extension points/attributes appearing in local modules/exceptions/opens are attached to the enclosing let expression:

let module%foo[@bar] M = N in ...

is parsed as

[%foo (let module M = N in ...)[@bar]]

while after this PR, this is parsed as

let [%%foo module M = N [@@bar]] in ...

(This syntax is introduced in this PR; it is not allowed in trunk.)

This has the advantage that:

extension points/attributes are attached to the object next to which they are written, which is more intuitive for the user, and
it coincides with the way they work for global structure items (outside of let expressions), so that converting a global module into a local module (or exception or open or ...) does not change the semantics of its extension points/attributes.

To attach extension points/attributes to the enclosing let node, one can write them next to the let keyword (this is introduced in this PR; it is not allowed in trunk):

let%foo[@bar] module M = N in ...

is parsed as

[%foo (let module M = N in ...)[@bar]]

This is a breaking change, but it is being considered because these forms are not used much in the wild. A search in Sherlocode yielded only use of these forms: https://sherlocode.com/?q=let%20module%20%5B%40.

(Note that that a test is expected to fail until #14008 is merged.)

gasche · 2025-05-02T07:34:13Z

If I understand sherlocode correctly, the in-the-wild usage it found is let module[@warning "-60"] C = ... in ..., and those would still work with the proposed new interpretation, right?

gasche · 2025-05-02T07:34:59Z

([@warning "-60"] is old-OCaml speak for [@warning "-unused-module"].)

nojb · 2025-05-02T07:36:25Z

If I understand sherlocode correctly, the in-the-wild usage it found is let module[@warning "-60"] C = ... in ..., and those would still work with the proposed new interpretation, right?

No, they would not, because the [@warning "-60"] attribute needs to be attached to the enclosing let node to be useful, and that would no longer be the case with the new interpretation. Instead, one would need to write:

let[@warning "-60"] module C = ... in ...

gasche · 2025-05-02T07:36:59Z

testsuite/tests/warnings/w60.compilers.reference

+File "w60.ml", line 40, characters 6-27:
 40 |   let module M = struct end in
-                  ^
+           ^^^^^^^^^^^^^^^^^^^^^


Do you know why the new error location is less precise than the previous one?

Yes, it is mentioned in #14005: this less precise location matches the one used for the warning in case of global module bindings module M = ... and is a consequence that we share more code after this PR. We could make all cases (local and global) use the more precise location with a bit more plumbing work. I wasn't sure if it was worth it.

gasche · 2025-05-02T07:39:55Z

How are people supposed to silence warning 60 on structure-level module declarations?

# module T : sig end = struct module[@warning "-60"] C = struct end end;;
Warning 60 [unused-module]: unused module C.

module T : sig end

# module T : sig end = struct module C = struct end [@@warning "-60"] end;;
Warning 60 [unused-module]: unused module C.

module T : sig end

# module T : sig end = struct [@@@warning "-60"] module C = struct end end;;
module T : sig end

I'm confused. Is this a bug?

nojb · 2025-05-02T07:42:29Z

I'm confused. Is this a bug?

I don't think so. As I understand it, the warning attribute acts on a scope, not on an object, so it is not useful to attach it to the structure item itself, but rather it needs to be activated for the rest of the scope, which is what the third variant does. (Also, note that the first two variants are exactly equivalent.)

gasche · 2025-05-02T07:47:02Z

From a distance the behavior you explain is not quite right, because it does not give a way to selectively silence warnings for one declaration while allowing it for other declarations in the same scope. It would be nice if the behavior I had in mind worked, because it would make the code written by our users correct. But this is arguably a distraction from the wider PR.

I still have the impression that the change proposed in this PR (with the breaking change) is nicer than the one proposed in the other PR, and I would support it, assuming that the resulting implementation is not more complex (than the one resulting from the other PR).

nojb · 2025-05-02T07:48:37Z

assuming that the resulting implementation is not more complex (than the one resulting from the other PR).

Actually, the implementation in this PR is actually simpler as we can share more code between the local and global cases.

nojb · 2025-05-05T17:11:22Z

I have done an OPAM-wide search using opam-grep looking for uses of infix extension points/attributes attached to a let expression. The only hit in user code was Frama-C to attach [@warning "-60"] to local modules:

https://git.frama-c.com/pub/frama-c/-/blob/master/src/plugins/inout/operational_inputs.ml#L645
https://git.frama-c.com/pub/frama-c/-/blob/master/src/kernel_services/ast_queries/logic_typing.ml#L2492

The rest of hits were in vendored copies of compiler code, along a PPX https://github.com/ghilesZ/ppx_wideopen and ocamlformat. But these will have to be adapted anyway to the new Pexp_struct_item AST node, so this PR does not change that.

Given this, I think that the breaking change in this PR is acceptable.

@gasche: do you agree? If yes, are you willing to do a review of the PR?

gasche

This looks good to me, thanks! I agree that the slight behavior change is acceptable.

gasche · 2025-05-16T06:14:07Z

parsing/parser.mly

+      item_extension post_item_attributes
+        { let docs = symbol_docs $sloc in
+          Pstr_extension ($1, add_docs_attrs docs $2) }
+    )


I don't understand why there is a special case for Pstr_extension, I wish it was handled like the rest below (or a comment to explain the difference), but this was already present before the PR.

@gasche Wish granted: 1ea51b7

Planning to merge once CI passes.

Actually, I removed this last commit because the same asymmetry exists for signatures. I will open a follow-up PR doing this cleanup everywhere.

@bar

Before this change, infix extension points and/or attributes were attached to the AST node of the enclosing let expression, ie: let module%foo[@bar] M = N in ... was parsed as [%foo (let module M = N in ...)[@bar]] After this change, this is parsed as: let [%%foo module M = N [@@bar]] in ... Moreover, ext/attrs are now allowed in following the "let" itself to attach them to the enclosing let: let%foo[@bar] module M = N in ... is parsed as [%foo (let module M = N in ...)[@bar]] This is a breaking change.

nojb mentioned this pull request May 2, 2025

let module/exception/open: refactor parsing rules towards generalization #14005

Closed

gasche reviewed May 2, 2025

View reviewed changes

nojb assigned gasche May 14, 2025

gasche approved these changes May 16, 2025

View reviewed changes

nojb added 6 commits May 16, 2025 09:39

parser.mly: uniformize

3b3d14d

parser.mly: introduce non-terminal local_structure_item

20cb820

Promote parser

ee65b31

Accept test

016891d

Changes

b318635

nojb force-pushed the normalize_ext_attrs_let_struct_item-2 branch 2 times, most recently from 90bfe5d to b318635 Compare May 16, 2025 09:12

Changes

daf4711

nojb merged commit 76b1edd into ocaml:trunk May 16, 2025
23 of 24 checks passed

nojb deleted the normalize_ext_attrs_let_struct_item-2 branch May 16, 2025 10:07

This was referenced May 16, 2025

Uniformize parsing rules for structure and signature items #14039

Merged

Allow local form of arbitrary structure items #13835

Closed

314eter mentioned this pull request Jul 27, 2025

Allow arbitrary structure items in let expressions #14040

Merged

nojb mentioned this pull request Jul 30, 2025

inline syntax for [%foo open! struct end] #13334

Open

Julow mentioned this pull request Dec 1, 2025

5.5 support: Structure item embedded in expressions ocaml-ppx/ocamlformat#2700

Closed

Conversation

nojb commented May 2, 2025

Uh oh!

gasche commented May 2, 2025

Uh oh!

gasche commented May 2, 2025

Uh oh!

nojb commented May 2, 2025

Uh oh!

gasche May 2, 2025

Choose a reason for hiding this comment

Uh oh!

nojb May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented May 2, 2025

Uh oh!

nojb commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasche commented May 2, 2025

Uh oh!

nojb commented May 2, 2025

Uh oh!

nojb commented May 5, 2025

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

gasche May 16, 2025

Choose a reason for hiding this comment

Uh oh!

nojb May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb May 16, 2025

Choose a reason for hiding this comment

Uh oh!

nojb May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nojb May 2, 2025 •

edited

Loading

nojb commented May 2, 2025 •

edited

Loading

nojb May 16, 2025 •

edited

Loading