Skip to content

Conversation

@lukemaurer
Copy link
Contributor

The flags for parameterized modules are already being parsed, but -parameter is similar to the load path in that it needs to be set up before typechecking begins. Without this feature, editing any module in a parameterized library gives an error for every reference to a parameter or to another parameterized module.

This feature sets up the parameters at the same time the load path is initialized, which is also when the compiler does it. The parameters are kept in Env.persistent_env, which is already handled by the Local_store mechanism, so they should stay in sync with all the other bits of global state lying around.

Signed-off-by: Luke Maurer <[email protected]>
Signed-off-by: Luke Maurer <[email protected]>
Signed-off-by: Luke Maurer <[email protected]>
@liam923
Copy link
Contributor

liam923 commented Jan 31, 2025

I think this needs to get rebased for the tests to pass on CI

@lukemaurer
Copy link
Contributor Author

I think this needs to get rebased for the tests to pass on CI

Yeah, I was expecting the tests to merge before being run? Anyway, they should pass now

Copy link
Contributor

@liam923 liam923 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 easy-to-read tests.

I'd like to see a couple tests that demonstrate that Merlin does the right thing when files/configuations change. Maybe:

  • The parameter changes (ie, p.mli is edited and then recompiled)
  • The set of parameters change (ie, start passing -parameter Q to a file that had been using -parameter P)

I'd also like to see a test where the -parameter flag is passed via a .merlin file. .merlin files are how we pass configuration to Merlin in our build system. To do this, you'll want to put a file named .merlin in the same directory as the .ml file. The contents should probably be:

S .
B .
FLG -parameter P

And then you won't need to pass -parameter on the command line. Note that you can create the .merlin file in the test (and then delete it) so that it isn't sitting around for all the tests that explicitly pass the flag.

@lukemaurer lukemaurer merged commit ff41cf1 into main Feb 11, 2025
2 checks passed
@lukemaurer lukemaurer deleted the use-parameter-flag branch February 11, 2025 13:30
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.

3 participants