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.

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

Fix attributes being dropped from packed module expressions and patterns #89

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

tdelvecchio-jsc
Copy link

@tdelvecchio-jsc tdelvecchio-jsc commented Nov 12, 2024

In the following program, attributes [@attr2] and [@attr3] are both dropped by ocamlformat silently:

module _ : sig
  val foo : (module T with type t = 'a [@annot1]) -> unit
end = struct
  let foo (type a) (module M : T with type t = a [@annot2]) =
    (module M : T with type t = a [@annot3])
  ;;
end

In the standard parser, expressions and patterns like (module M : S) are parsed as constraints wrapping an inner expression / pattern, while in the extended parser, the package type S is part of the same node as the module expression / pattern. In the parsing logic for this, the attributes parsed for the package type are just dropped.

In the second commit, we add the attributes to the package type, and handle it as necessary in all code.

Signed-off-by: Thomas Del Vecchio <[email protected]>
@tdelvecchio-jsc tdelvecchio-jsc force-pushed the tdelvecchio.dropped-attributes-on-packed-types branch from 1f26688 to ee8c9f6 Compare November 14, 2024 20:31
@tdelvecchio-jsc tdelvecchio-jsc force-pushed the tdelvecchio.dropped-attributes-on-packed-types branch from ee8c9f6 to d32387e Compare November 14, 2024 20:33
Copy link

@dvulakh dvulakh left a comment

Choose a reason for hiding this comment

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

Looks good to me. We agreed it would be better to wrap the package type in parentheses for clarity:

(module M : (T with type t = a)[@attr])

Instead of

(module M : T with type t = a[@attr])

But we decided not to block the bugfix on this and have filed a ticket internally instead.

@tdelvecchio-jsc tdelvecchio-jsc merged commit 9ff450e into jane Nov 15, 2024
6 of 7 checks passed
@tdelvecchio-jsc tdelvecchio-jsc deleted the tdelvecchio.dropped-attributes-on-packed-types branch November 15, 2024 18:59
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