Fix #3574 and support resolve in explicit records

[joyfulmantis]

This PR fixes #3574 in addition to implementing resolve for explicit record fields.

• Find out when renderRecordInfo can fail, and implement that logic in the rule.
  • (It shouldn't fail)
• Add tests for Multiple code action for (at least) explicit record fields #3574 fix
• Add documentation

[michaelpj]

Obsoletes #3579, right?

[joyfulmantis]

Obsoletes #3579, right?

Yes. That PR only fixed part of the bug, and Berk didn't have time to finish it, so I'm expanding on his changes to completely fix the bug in addition to providing resolve support here.

[ozkutuk]

Thanks for doing this. I had a brief look and it all looks good to me. I just have a few questions:

1. I couldn't understand what this "resolve" business about. Admittedly I have been away from HLS codebase for some time. Is that something recently introduced by the LSP spec? Can you point me to a resource (I tried to find the PR that introduced it to HLS but failed to do so) so that I can figure out what is going on with these changes?
2. I realized that you have removed the CollectNames rule and embedded the functionality directly into the rule of the plugin itself. Is there a particular reason to do so? I think the idea with writing it as a separate rule in the first place was that it was a pretty general rule that could be used by other plugins, and in that case the results could be shared between the plugins by means of using a shared rule.

[joyfulmantis]

1. I couldn't understand what this "resolve" business about. Admittedly I have been away from HLS codebase for some time. Is that something recently introduced by the LSP spec? Can you point me to a resource (I tried to find the PR that introduced it to HLS but failed to do so) so that I can figure out what is going on with these changes?

Yes, that's functionality since 3.16 https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeAction_resolve . Theoretically, it allows us to delay some computations until we actually need it. (For example, we only render the text edit now at the last moment right before the client applies it)

2. I realized that you have removed the `CollectNames` rule and embedded the functionality directly into the rule of the plugin itself. Is there a particular reason to do so? I think the idea with writing it as a separate rule in the first place was that it was a pretty general rule that could be used by other plugins, and in that case the results could be shared between the plugins by means of using a shared rule.

I did that to simplify the code, as it's currently not used by anyone else. I don't feel strongly about that, and am happy to add it back in, although it might be more discoverable if moved to ghcide.

[michaelpj]

It does sound like it would be good to write an overview of how resolve works and when you might want to use it. Maybe in the module haddock for the Resolve module in hls-plugin-utils?

[joyfulmantis]

It does sound like it would be good to write an overview of how resolve works and when you might want to use it. Maybe in the module haddock for the Resolve module in hls-plugin-utils?

Done

[michaelpj]

LGTM

[michaelpj]

I would normally think this was just the tests being weird, but two of the windows jobs are hanging for an hour on... the explicit record fields test. Could we somehow have ended up with a case where we'll hang? I would expect lsp-test to time out if things take too long, so I'm not sure how this could happen.

[michaelpj]

I canceled it, and indeed it was hanging in the new HsExpanded tests!

[joyfulmantis]

Crazy because it's a windows only problem too. The Linux tests for those versions fail very quickly as they should

[joyfulmantis]

Trying to set up a Windows machine will be a headache, and there is nothing obvious about why they would hang on Windows. I'll try to run the tests again with debug to see if I can spot the issue, otherwise, I am tempted to just put in CPP to avoid the test on Windows (versus the expected failure that it is now).

[michaelpj]

I am tempted to just put in CPP to avoid the test on Windows (versus the expected failure that it is now).

I'd be fine with that if it was just a failure, but if it's a hang, that can be really bad if people hit it. Not sure what to do :( Can we split out the HsExpanded fix into another PR for now?

[joyfulmantis]

I am tempted to just put in CPP to avoid the test on Windows (versus the expected failure that it is now).

I'd be fine with that if it was just a failure, but if it's a hang, that can be really bad if people hit it. Not sure what to do :( Can we split out the HsExpanded fix into another PR for now?

Yes, that won't be too hard. However, I am pretty sure this is a lsp-test issue. The test file that's failing, in this case, is one that uses GHC's overloaded-record-dot language extension and thus does not compile on GHC versions less than 9.2. Since it doesn't compile, then that means that none of the rules or methods provided by the plugin would end up getting successfully executed. (Hence highly likely to be a lsp-test issue)

[michaelpj]

Hmm. Let's see if it fails again.

[joyfulmantis]

Hmm. Let's see if it fails again.

The debug option doesn't turn on any lsp-test debug flags, so I bet it's gonna hang again without telling us anything useful.

[michaelpj]

Okay, now one of the windows jobs has an unexpected success for the HsExpanded case?

[joyfulmantis]

Okay, now one of the windows jobs has an unexpected success for the HsExpanded case?

Previously I was using a file that uses the overloaded record dot extension to generate the HsExpanded ast, however now I have switched to using if-then-else with rebindable syntax, which I believe is what HsExpanded was first used for. However, even though as of 9.0 they switched if-then-else when rebindable syntax is on to being compiled down to HsExpanded, it previously was still valid syntax (hence the unexpected success).

[michaelpj]

Still has a flags failure I think.