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

Add --use-compiler-pp flag to standalone driver #555

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NathanReb
Copy link
Collaborator

This flag allows user to force the driver to print AST as source code using the installed compiler printers rather than its own.

This can be useful in situation where one is using the driver source output on older OCaml version to prevent outputting incompatible syntax. In addition it can prevent the driver to output uninterpreted extensions when migrating down newer compiler features before we bumped the internal AST.

@NathanReb
Copy link
Collaborator Author

NathanReb commented Jan 15, 2025

I'm struggling a little bit to write a test for this. Ideally we'd like a situation where the tests will work even after we bump our AST.

That means that we'd like to try and generate new syntax nodes that either can't be migrated down or that are printed out as different source code once migrated down. I initially thought I could work something out with the unit functor parameter but it turns out it's just a representation change, the syntax was already supported before it seems.

Ideally we'd want something that we can test with 4.08 minimum since we're going to drop support for older compilers.

If anyone has a suggestion I'd gladly take it!

@NathanReb NathanReb force-pushed the migrate-before-print branch 2 times, most recently from 8de4df4 to cc26ff8 Compare January 20, 2025 15:19
This flag allows user to force the driver to print AST as source code
using the installed compiler printers rather than its own.

This can be useful in situation where one is using the driver source
output on older OCaml version to prevent outputting incompatible syntax.
In addition it can prevent the driver to output uninterpreted extensions
when migrating down newer compiler features before we bumped the
internal AST.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb NathanReb force-pushed the migrate-before-print branch from cc26ff8 to 0d3e8b9 Compare January 20, 2025 15:20
@NathanReb
Copy link
Collaborator Author

Ok, this should be ready for review. I found a decent way to test it, even though I'm no completely satisfied but I think we'll have to settle for this for now.

I used the named existential syntax which only exist from 4.13 onward:

match x with
| C (type a) ... -> ...

The test runs on 4.12 and lower and shows that by default we successfully output the new syntax because our internal printers and AST are 4.14 at the time of writing. If we use the flag, it tries to migrate this before printing and fails as we expect. I would have prefered a test with a successful outcome but I couldn't figure out a nice printing change for that. Maybe I missed something there so feel free to make any suggestion!

Another thing to note is that the full range of Pprintast functions is only exposed since 4.14 so to make this compatible with older compilers I exposed dummy versions of those functions on old compilers. This is very unlikely to be hit by the vast majority of users as the only commonly used correction based code gen is deriving_inline which only requires structure and signature printers. I'll make it so the error clearly points out that the problem is the combination of OCaml version + --use-compiler-pp.

@NathanReb
Copy link
Collaborator Author

I think this should be ready for review @patricoferris, if you have some spare cycles!

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

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

Thanks Nathan -- this is pretty useful for debugging things too.

test/driver/compiler-pp/run.t Outdated Show resolved Hide resolved
src/reconcile.ml Show resolved Hide resolved
Signed-off-by: Nathan Rebours <[email protected]>

Co-authored-by: Patrick Ferris <[email protected]>
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