Get files from Shake VFS from within plugin handlers

[awjchen]

This PR is a step towards #4057.

This PR modifies plugin handlers to get virtual files from the Shake VFS instead of the language server's VFS. Specifically, this PR removes both pluginGetVirtualFile and pluginGetVersionedTextDoc, and replaces them with the (existing) getFileContents and (new) getVersionedTextDoc Shake actions.

This PR also changes getFileContents to return the Rope at hand instead of converting it to Text, which eliminates a few redundant conversions (specifically, at uses of getCompletionPrefix[fromRope]). There are probably more instances in the plugin handlers where Ropes could replace Text for more efficiency, but this PR does not address them.

[michaelpj]

LGTM, perhaps @fendor could take a look?

I wonder if we should just run more of our handlers in Action at the "top-level", it's always a bit awkward running the individual actions repeatedly...

[fendor]

The idea and execution is good, but could we introduce some plugin util functions that abstract over the most common use cases? I find

liftIO $ runAction "ModuleName.getFileContents" state $ fmap snd $ getFileContents nfp

much less appealing than

lift . pluginGetVirtualFile $ toNormalizedUri uri

In particular

    contents <- liftIO $ runAction "Pragmas.GetFileContents" ide $ maybe (pure Nothing) (fmap snd . getFileContents) $ LSP.uriToNormalizedFilePath $ toNormalizedUri uri

looks much more complicated.

Some combinators that make it easier to figure out at a glance what is going on would be great.

[soulomoon]

I agree with @fendor , it would be great if this is introduced as new util function. And perhaps other plugin can use it.

[awjchen]

@fendor Sorry for the delay! I've added some util functions that make the file accesses look more like they did before this PR

[fendor]

LGTM, I am now happy with the changes :)

[michaelpj]

Looks like it has some warnings, which is failing the build.

[awjchen]

Oops, I think I've fixed it now.

[awjchen]

Ah, there are some actual test failures for cabal-gild and cabal-fmt:

Received an expected error in a response for id IdInt 2:
    TResponseError {_code = InR ErrorCodes_InvalidParams, _message = "cabal-fmt: Invalid Params: Formatter plugin: could not get file contents for Uri {getUri = \"file:///hom
e/alex/src/haskell-language-server/plugins/hls-cabal-fmt-plugin/test/testdata/simple_testdata.cabal\"}", _xdata = Nothing}

This error is caused by the use of Development.IDE.Core.Shake.getVirtualFile in mkFormattingHandlers returning Nothing, indicating that that the URI wasn't found in the Shake VFS. Before this PR, mkFormattingHandlers used pluginGetVirtualFile, which instead looked up the URI in the LSP VFS.

I don't know what the relationship is between the two VFSs, or why they would be out-of-sync for cabal files. I guess I'll start looking into it. Any thoughts on what might be going on? @michaelpj @fendor

[fendor]

I don't have much intel, you seem to have already identified the issue, and I don't think there was any reason why it was done like this, probably a lack of understanding between the differences of ghcide's VFS and LSP's. Perhaps adding the cabal file to the ghcide LSP is sufficient already?

[michaelpj]

So, the test helper does open the file in the client, so we should check that we're getting file change notifications for it, otherwise indeed we won't know about it. I'm not sure how we could not be.

I have noticed a few reports of us not updating properly after cabal file changes, so this might be an actual bug!

[fendor]

@awjchen I figured out the issue, the cabal formatters need the notification handlers provided by hls-cabal-plugin. This issue should be fixed now.