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: set input_lexbuf when reading binary AST too #483

Merged

Conversation

anmonteiro
Copy link
Contributor

@NathanReb
Copy link
Collaborator

@anmonteiro I'm not super familiar with melange but I'm guessing this is necessary for reason's syntax support right?

With reason you're using the driver on the marshalled AST resulting from parsing the reason syntax, whereas with regular OCaml syntax, you're using the driver directly on the source files. Are my assumptions correct?

@anmonteiro
Copy link
Contributor Author

I don’t think so. There’s no dialect present in my repro https://github.com/anmonteiro/no-source-quotation-repro

I think this will rather happen on all staged_pps (the instructions in my repro pass the -ppx flag)

@NathanReb
Copy link
Collaborator

Ok thanks for the details!

I'll add a test for this and merge!

@NathanReb NathanReb force-pushed the anmonteiro/lexbuf-for-binary-too branch from db788a9 to 0f3350c Compare March 26, 2024 14:20
@NathanReb NathanReb force-pushed the anmonteiro/lexbuf-for-binary-too branch from 0f3350c to b6e4c93 Compare March 26, 2024 14:26
Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I just fixed the formatting and added a test to show the before/after the fix.

@NathanReb NathanReb merged commit 04e050c into ocaml-ppx:trunk-support Mar 26, 2024
4 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/lexbuf-for-binary-too branch March 26, 2024 22:24
@anmonteiro
Copy link
Contributor Author

Thank you!

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