Solve crash with module name plugin under certain circumstances #2518
Conversation
| @@ -48,7 +48,7 @@ renameProvider state pluginId (RenameParams (TextDocumentIdentifier uri) pos _pr | |||
| getFileEdits = ap (getSrcEdits state . renameModRefs newNameText) (locToUri . head) | |||
|
|
|||
| fileEdits <- mapM getFileEdits filesRefs | |||
| pure $ foldl1 (<>) fileEdits | |||
| pure $ foldl' (<>) mempty fileEdits | |||
There was a problem hiding this comment.
If strictness is useful here - foldl1' is also present in base.
afaik foldl1 is not completely strict, while foldl1' is a properly strict left fold. A submitter had a coverage on that & that is pointed out in https://hackage.haskell.org/package/base-4.16.0.0/docs/Data-List.html#v:foldl1-39-, after its submission a lot of basic data types shipped foldl1' implementations.
This note may be skipped.
There was a problem hiding this comment.
TBH, I don't know what level of strictness is needed here. I guess "the more the better" since it is left fold and non-strict left fold does not have much application.
The goal here was to get rid of foldl1 as it is unsafe. It wasn't obvious for me that the fileEdits is a nonempty list. However, since we have monoid here, we can use ordinary foldl(') with mempty as initial condition.
There was a problem hiding this comment.
Actually, I think there's a small policy question here: do we want to return a response with an empty edit here, or fail the response? I think either is arguable, but perhaps worth a note that we might send back an empty edit here.
There was a problem hiding this comment.
As a matter of policy, we don't want any unhandled exceptions to leak out. So we want fold' here, and the empty set of edits is fine.
There was a problem hiding this comment.
I didn't mean to leak an exception, the request itself can fail in the protocol. Probably that's not appropriate since nothing went wrong per se, though.
| @@ -48,7 +48,7 @@ renameProvider state pluginId (RenameParams (TextDocumentIdentifier uri) pos _pr | |||
| getFileEdits = ap (getSrcEdits state . renameModRefs newNameText) (locToUri . head) | |||
There was a problem hiding this comment.
If you're feeling brave, there's a head here you could murder too.
michaelpj
added
merge me
|
Reflexively added merge-me, even though I suggested some comments 😅 they're optional, though, so if you'd rather no I still think this is mergeable. |
|
I feel brave ;). I can try to fix few other unsafe functions of that type I find, but don't have much time in next few days; but should get it done by new year. So if there is no hurry, we can keep this PR for more fixes, otherwise I can open new pr when more fixes are done. |
|
@ttylec, there is no need to wait. You addressed all required entries in the reviews. |
|
🙏 Thank you for improving stability here. P.S. |
On codebase I was working on (unfortunately not public) hls crashed under vscode with
Prelude.foldl1: empty listfor one module (a very basic servant api).By plugin elimination, module name plugin was the source of that crash. There was no foldl1 in the plugin, but as suggested on irc (thank you for help, but I did not store my irc history, so I don't remember who to thank you... please rise your hand if you recognise yourself), there may be some Data.Foldable function used there that uses foldl1. That was indeed the case:
minimumBy.This pull request make call to
minimumBysafe viaData.List.NonEmpty. Additionally, I replaced foldl1 found in other place with foldl' withmemptyas initial value, since it was using monoidal concatenation anyway.