Skip to content

Solve crash with module name plugin under certain circumstances #2518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 21, 2021
Merged

Solve crash with module name plugin under certain circumstances #2518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee7845a
use foldl' instead of foldl1 since there is monoid anyway
ttylec Dec 21, 2021
42bef3d
avoid unsafe minimumBy
ttylec Dec 21, 2021
123d00a
Merge branch 'master' of github.com:ttylec/haskell-language-server
ttylec Dec 21, 2021
5128e19
Merge branch 'master' into master
Anton-Latukha Dec 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugins/hls-module-name-plugin/src/Ide/Plugin/ModuleName.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import Data.Aeson (Value (Null), toJSON)
import Data.Char (isLower)
import qualified Data.HashMap.Strict as HashMap
import Data.List (intercalate, isPrefixOf, minimumBy)
import qualified Data.List.NonEmpty as NE
import Data.Maybe (maybeToList)
import Data.Ord (comparing)
import Data.String (IsString)
Expand Down Expand Up @@ -99,7 +100,7 @@ action state uri =
let emptyModule = maybe True (T.null . T.strip . virtualFileText) contents

correctNames <- liftIO $ traceAs "correctNames" <$> pathModuleNames state nfp fp
let bestName = minimumBy (comparing T.length) correctNames
bestName <- minimumBy (comparing T.length) <$> (MaybeT . pure $ NE.nonEmpty correctNames)

statedNameMaybe <- liftIO $ traceAs "statedName" <$> codeModuleName state nfp
case statedNameMaybe of
Expand Down
2 changes: 1 addition & 1 deletion plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ renameProvider state pluginId (RenameParams (TextDocumentIdentifier uri) pos _pr
getFileEdits = ap (getSrcEdits state . renameModRefs newNameText) (locToUri . head)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're feeling brave, there's a head here you could murder too.


fileEdits <- mapM getFileEdits filesRefs
pure $ foldl1 (<>) fileEdits
pure $ foldl' (<>) mempty fileEdits
Copy link
Collaborator

@Anton-Latukha Anton-Latukha Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


-------------------------------------------------------------------------------
-- Source renaming
Expand Down