New plugin: Explicit record fields #3304
Conversation
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
| mkCodeActionTitle exts = | ||
| if NamedFieldPuns `elem` exts | ||
| then title | ||
| else title <> " (needs extension: NamedFieldPuns)" |
There was a problem hiding this comment.
Cute, but I think this is inconsistent UX: we never normally say in the code action title if we're going to insert pragmas. I think the right way to do this consistently is AnnotatedTextEdits : #3210
There was a problem hiding this comment.
I have adapted this from the alternate number format plugin, as seen here. I do agree that AnnotatedTextEdits seem to be a nicer way to do this though. I will look into it to see if we can make use of it here.
There was a problem hiding this comment.
No pressure to use it in this PR - AFAIK we use it nowhere yet. But I'd be delighted if someone looked into it!
There was a problem hiding this comment.
I did some testing by converting this to use AnnotatedTextEdit. It looks like it is a pretty new addition to spec as the editors I tried didn't properly support them. Neovim LSP didn't care, and just handled it like a usual TextEdit without the ChangeAnnotation niceties. VSCode didn't display the code action at all.
The spec states that servers shouldn't send AnnotatedTextEdits if the client doesn't support them. Therefore wherever we use them, we need to query the client capabilities and fallback to plain TextEdits if needed. Adding some convenience functions to take care of these kinds of details sounds like a good idea, but seems out-of-scope for this PR, so I am leaving this as is.
There was a problem hiding this comment.
Hmm, that's disappointing, I would have expected vscode to do something 🤔
There was a problem hiding this comment.
related: microsoft/vscode#151240
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Show resolved
Hide resolved
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
| collectRecordsInRange :: MonadIO m => Range -> IdeState -> NormalizedFilePath -> ExceptT String m CollectRecordsResult | ||
| collectRecordsInRange range ideState nfp = do | ||
| CRR renderedRecs exts <- collectRecords' ideState nfp | ||
| pure $ CRR (filter inRange renderedRecs) exts |
There was a problem hiding this comment.
Just thinking through how this works: we collect all the record stuff in the module and cache it with a rule, good. But then we do a linear amount of work every time we get a code action request filtering them to see if they're in range. Maybe that's fine, but I could imagine this blowing up in modules with a lot of record stuff.
A cheap improvement would be to do some kind of binary search for the range filtering. Have a look at the code-ranges plugin, which I think does something similar.
There was a problem hiding this comment.
I have made some changes to the filtering logic: Now the rule builds and caches an IntervalMap rather than a list, so the filtering should be more efficient. Can you take a look to see if the changes are reasonable?
There was a problem hiding this comment.
Looks like hw-prim package (transitively depended on by hw-fingertree) doesn't yet work with GHC 9.4 though. Created a PR to upstream: haskell-works/hw-prim#141
PR got merged, so it should be working now, though I couldn't test it locally
plugins/hls-explicit-record-fields-plugin/src/Ide/Plugin/ExplicitFields.hs
Outdated
Show resolved
Hide resolved
This reverts commit 57646d3.
|
The windows failures are in your testsuite and seem genuine, maybe some path issues. |
@michaelpj Do you have any idea how I might tackle this? The error message mentions a |
|
Sorry, I spoke too soon. I thought it was a problem with your tests looking up paths, but indeed it's some clang thing, so probably indeed random toolchain issues :( I'll rerun again... |
|
Hmmm... I notice that the plugin name is long and so is the name of the test component, such that the path it can't find is extremely long. I know we've had path length issues before, and I notice that some of the other plugins with long names have short names for their test components. What happens if you rename your test component to something short like "tests"? |
|
hooray, thanks for sticking with it! |
|
Ah, one more thing: can you add yourself to CODEOWNERS for the plugin, please? |
What?
This is a plugin to expand record wildcards, explicitly listing all fields as field puns. Here is a little demo. It works in both record construction and pattern binding scenarios, and it works as you would expect regardless of whether there are explicitly provided fields or puns in addition to the wildcard.
Known issues
One of the shortcomings of the current approach is that all fields of the record are expanded, whether they are actually used or not. This results in warnings of unused bindings, if the corresponding warning flag is enabled. I am looking for ways to expand only used fields, but I still think this is useful as it is, hence the PR 🙂