Skip to content

fix: handle case where doc does not lex as ocaml #2684

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

Merged
merged 4 commits into from
May 7, 2025

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Apr 11, 2025

Fixes #2679

Formatting mld files can crash when the input file cannot be lexed as ocaml. This can happen with LaTeX is found in {m ...} or C code in {@c[ ... ]}, for example.

In that case we would crash when building the list of tokens in the file; however it's not used in the mld case.

Fixes ocaml-ppx#2679

Formatting `mld` files can crash when the input file cannot be lexed as
ocaml. This can happen with LaTeX is found in `{m ...}` or C code in
`{@c[ ... ]}`, for example.

In that case we would crash when building the list of tokens in the
file; however it's not used in the `mld` case.
@emillon emillon force-pushed the mld-lexing-errors branch from 794e1c4 to b401c6e Compare April 11, 2025 09:07
@emillon emillon requested a review from Copilot April 11, 2025 09:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (5)
  • lib/Parse_with_comments.ml: Language not supported
  • test/passing/refs.default/doc.mld.ref: Language not supported
  • test/passing/refs.janestreet/doc.mld.ref: Language not supported
  • test/passing/refs.ocamlformat/doc.mld.ref: Language not supported
  • test/passing/tests/doc.mld: Language not supported

@emillon emillon requested a review from EmileTrotignon April 11, 2025 09:08
Co-authored-by: Copilot <[email protected]>
@EmileTrotignon
Copy link
Collaborator

I don't really like that we catch the exception even when we are not parsing documentation, but I checked and we cannot match on fragment at this stage. Maybe an is_documentation bool argument could fix it ?

Still I think its okay to merge as-is, the exception will be raised elsewhere when truly parsing an ocaml file.

@emillon
Copy link
Collaborator Author

emillon commented Apr 11, 2025

yeah I agree it's not ideal. we can't really make the tokens field lazier because it needs the file open, so making it optional would be nice but I'm not sure how to wire that to this call site.

Copy link
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.

Let's merge!

@Julow Julow merged commit e879af1 into ocaml-ppx:main May 7, 2025
8 of 12 checks passed
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.

Bug: crash when backslash is in mld file
3 participants