overloaded record dot plugin intial version (closes #3350)

[joyfulmantis]

This is my first attempt at writing a plugin to provide codeActions that convert record selectors to record dot syntax. Please let me know any areas that can be improved.

[joyfulmantis]

Upon investigation of the failing builds, the GHC parsed syntax has changed enough that this is incompatible with GHC 9.2 (I tested myself only with 9.4). I'll see what I can do about a compatibility layer for 9.2 tomorrow.

[July541]

LGTM! Please add yourself to CODE_OWNERS.

I'm a little worried about the correctness of different scenarios. It would be great if more test cases were available!

Since most codes are similar to hls-explicit-record-fields-plugin, do we have the opportunity to extract some common patterns?

[July541]

Your gif is 17.9 MB, can we reduce the size?😄

[joyfulmantis]

Your gif is 17.9 MB, can we reduce the size?

Done

I'm a little worried about the correctness of different scenarios. It would be great if more test cases were available!

Hmm, it's a relatively simple plugin, so there is not much to test. Could you give some examples of areas that need better coverage?

Since most codes are similar to hls-explicit-record-fields-plugin, do we have the opportunity to extract some common patterns?

While the explicit record fields plugin was used as a template, a lot of code had to be changed. There still certainly are some functions that share a lot of similar code, but nothing that I would think makes sense isolating to a shared function at this point.

[July541]

Hmm, it's a relatively simple plugin, so there is not much to test. Could you give some examples of areas that need better coverage?

Sigh, I don't have a complete test range. I think it's also acceptable if we complement tests when necessary.

[michaelpj]

I'd like to take a look in a bit. A high level comment is that we do have a lot of plugins, so we might want to consider whether we want to merge this into something else. Could potentially go in hls-refactor-plugin? Unsure.

[joyfulmantis]

I'd like to take a look in a bit. A high level comment is that we do have a lot of plugins, so we might want to consider whether we want to merge this into something else. Could potentially go in hls-refactor-plugin? Unsure.

Yes, I wasn't sure exactly how to integrate this functionality into the codebase. hls being very flexible with its plugin architecture coupled with lacking specific guidelines for what should be written as an individual plugin means that there are mammoth plugins, like hlint, and really small ones, like explicit record wildcard fields or explicit imports.

The flexibility in the plugin architecture is certainly helpful for easily including external and legacy tools. Still, an official policy on when to develop a new plugin for hls specific code (and perhaps some work to make legacy hls specific plugins conform to said policy) would be beneficial.

I decided to write this as a separate plugin after comparing it against standalone plugins with similar scopes, such as the explicit record fields plugin. However, I am happy to rewrite this into another plugin, such as hls refactor, if that architecturally would make more sense.

[July541]

The problem is: if we move this plugin into hls-refactor-plugin, should hls-explicit-record-fields-plugin be treated the same? Furthermore, should we have hls-code-actions-plugin? I don't think the answer is yes, in my opinion, the only shortcoming is the release process, we need to update the version manually.

[michaelpj]

Code generally looks good! Mostly high-level suggestions.

[joyfulmantis]

Code generally looks good! Mostly high-level suggestions.

Thanks for all the suggestions!

[joyfulmantis]

All of the comments have been resolved, with decisions that would most appropriately be decided for the whole project moved to #3597

[michaelpj]

Great stuff! Just some nits then I think we can merge.

[michaelpj]

generally this haddock reads rather weirdly, like you meant it to flow between the documentation on the various fields? I'm not quite sure what you're trying to do here

[joyfulmantis]

I'm using the documentation for record fields https://haskell-haddock.readthedocs.io/en/latest/markup.html#constructors-and-record-fields

[michaelpj]

Just thinking about how this will read. If I understand right, for something like

foo bar

we'll get "Convert foo to record dot syntax". This reads a little weirdly to me, since "foo" is not really the thing being converted? What about something like "Use record dot syntax to access foo"?

[joyfulmantis]

There is not really a limit to how long the "record" will be, because we only require it to be an expression that eventually evaluates to a record. The record selector, on the other hand, should be relatively short. I like the "Convert foo to record syntax" option, because, that's the part that's actually changing. I feel "Use record dot syntax to access foo" would be incorrect, as we are not accessing foo, rather we are using foo to access bar. (foo as a record selector, is effectively a function that allows us to acesss part of bar)