Skip to content

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

Closed
nojb wants to merge 6 commits intoocaml:trunkfrom
nojb:normalize_ext_attrs_let_struct_item
Closed

let module/exception/open: refactor parsing rules towards generalization#14005
nojb wants to merge 6 commits intoocaml:trunkfrom
nojb:normalize_ext_attrs_let_struct_item

Conversation

@nojb
Copy link
Copy Markdown
Contributor

@nojb nojb commented May 1, 2025

Following the merge of #13839 and as an intermediate step towards #13835, this PR refactors the parsing rules of let module, let exception and let open to put them into a form that can be readily generalized to other structure items.

Note that some care is needed because the handling of infix extension points and attributes in let module, let exception and let open is slightly different than for their global counterparts module, exception and open. Concretely,

let module%ext[@att] M = N in ...

is parsed as

[%ext (let module M = N in ...)[@att]]

while

module%ext[@att] M = N

is parsed as

[%%ext module M = N [@@att]]

In other words: in the local case, the extension point/attribute is attached to the enclosing let expression, while in the global case, it is attached to the structure item itself. This PR does not change these semantics.

Along the way, this PR introduces a new syntactic possibility: attaching attributes in postfix position to each one of these constructs:

  • let module M = N [@@foo] in ...
  • let exception E [@@foo] in ...
  • let open M [@@foo] in ...

In each case they produce the same AST as if they had been written in infix position (which is allowed today):

  • let module[@foo] M = N in ...
  • let exception[@foo] E in ...
  • let open[@foo] M in ...

This allows sharing more of the parsing rules between local and global forms of these constructs. Note that because of this sharing, the location utilized by the "unused module" warning in the case of a local module is changed (becoming slightly less precise), matching the one used by the same warning in the case of global modules. It is possible to use the more precise location in all cases with a bit more plumbing work, but I wasn't sure if it was worth it.

It is recommended to review the commits individually as each one is self-explanatory.

@gasche
Copy link
Copy Markdown
Member

gasche commented May 1, 2025

Now that we have a let <structure-item> in <expr> construct, I would expect let exception%foo Bar in baz to desugar to let [%%foo exception Bar] in baz, and let%foo exception Bar in baz to desugar to [%foo let exception Bar in baz].

I understand that this is a change of semantics, and orthogonal to the present PR (maybe?) but I wonder if this is not something that we want to consider changing now (in a separate PR), after some impact analysis.

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented May 1, 2025

I understand that this is a change of semantics, and orthogonal to the present PR (maybe?) but I wonder if this is not something that we want to consider changing now (in a separate PR), after some impact analysis.

This was my first thought as well, and I searched using Sherlocode for uses of these constructs. There were no hits for uses of infix extension points, but there was one use of infix attributes in the wild: https://sherlocode.com/?q=let%20module%20%5B%40

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

used to disable the "unused module" warning for M. With the "new" semantics, this needs to be written

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

If we decide that we can break backwards compatibility in this one case, then I wholly agree that these semantics are better (infix extension points/attributes for structure items work the same for both local and global variants, and extensions points/attributes are actually attached to the object next to which they are written syntactically).

Personally I would be in favour of doing this change.

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented May 2, 2025

Personally I would be in favour of doing this change.

I made a PR with the alternative (breaking) approach in #14009.

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented May 2, 2025

Personally I would be in favour of doing this change.

I made a PR with the alternative (breaking) approach in #14009.

I am closing this PR for the time being to focus on #14009 which is my preferred approach.

@nojb nojb closed this May 2, 2025
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.

2 participants