Merlin Pipeline PPX Cache is Broken when a pipeline is only used partially

[dyedgreen]

We’ve run into the following issue with the PPX caching (cc @pitag-ha) in the merlin pipeline:

1. Construct a pipeline and retrieve only its reader_parsetree
2. Construct a second pipeline and retrieve its ppx_parsetree or anything that depends on it

Now the PPX stage will return its cached result, because the reader parsetree is cached (from 1.), but the cached result is out of date.

This can be observed in particular when using the ocaml LSP (cc @rgrinberg, @ddickstein), since VSCode will send a documentFoldingRange request which only depends on the parse tree (this then leads to stale errors reported by the LSP, via the cache corruption outlined above).

I think a nice way to solve this issue (at least in the LSP, which has a notion of open documents) would be to remove the implicit global cache and instead have explicit per document caches and pipelines that we construct / update as the documents are opened / changed.

Another way to address this would be to compute a proper digest of the parse tree, rather than doing force_invalidation = not source_is_unmodified.

[voodoos]

Here is a more detailed description of the issue as I understand it:

1. Open a document and make a query that requires the ppx to be run. This populates the reader cache and the ppx cache.
2. Make an edit to the source.
3. Run a query that only requires the parsetree. This invalidates the reader cache and repopulate it.
4. Later, without other changes to the buffer, run a query that does require the ppx. Since the reader cache is correctly populated it will return "no changes" and the obsolete ppx cache will be reused as well.

Is that what you described ?

This is due to the ad-hoc coupling mechanism between the reader and the ppx cache.
I agree with your two suggestions, and added a third one:

1. Have per-document caches
2. Use a digest of the parsetree as cache key
3. Always invalidate the ppx cache when the reader cache is invalidated.

Since there is a bug I think we should first fix it before envisaging a design change like 1. I am wondering what would be the cost of marshaling the parsetree everytime however. Another option could also be to have an integer instead of a boolean value to synchronize the two caches, when the reader result change the integer is bumped. I will have a look.

[dyedgreen]

Yes, that description sounds correct to me.

The quick fix we’ve deployed internally so far is to just disable PPX caching entirely, so there is no particular rush for a quick fix from our end / we’d be happy to take time to come up with a new design.

(Although I’m not sure how much this effects the regular merlin protocol / ie if it’s easy to get into that state for most users right now; which might make a quick fix more urgent 😅)

[voodoos]

Ok, I was able to reproduce in the test suite and made a fix based on the numbering idea in #1650.

@dyedgreen could you check that is solves your issue ?

[dyedgreen]

This looks like it should fix the issue I’m seeing!