Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

[WIP] Download hlint data-files if they cant be found. #1540

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions haskell-ide-engine.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ library
, stm
, syb
, tagsoup
, temporary
, text
, transformers
, unix-time >= 0.4.7
Expand Down
130 changes: 123 additions & 7 deletions src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{-# LANGUAGE CPP #-}
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DuplicateRecordFields #-}
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE OverloadedStrings #-}
-- | apply-refact applies refactorings specified by the refact package. It is
-- currently integrated into hlint to enable the automatic application of
Expand All @@ -17,6 +20,7 @@ import Control.Exception ( IOException
import Control.Lens hiding ( List )
import Control.Monad.IO.Class
import Control.Monad.Trans.Except
import Control.Monad ( filterM, forM_ )
import Data.Aeson hiding (Error)
import Data.Maybe
import Data.Monoid ((<>))
Expand All @@ -32,6 +36,18 @@ import Language.Haskell.HLint4 as Hlint
import qualified Language.Haskell.LSP.Types as LSP
import qualified Language.Haskell.LSP.Types.Lens as LSP
import Refact.Apply
import System.Environment ( lookupEnv )
import System.Directory ( doesFileExist
, getXdgDirectory
, XdgDirectory(..)
, findExecutable
, listDirectory
, copyFile
)
import System.FilePath ( (</>) )
import System.Process
import System.IO.Temp ( withSystemTempDirectory )
import System.Exit ( ExitCode(..) )

-- ---------------------------------------------------------------------
{-# ANN module ("HLint: ignore Eta reduce" :: String) #-}
Expand Down Expand Up @@ -106,6 +122,103 @@ applyAllCmd uri = pluginGetFile "applyAll: " uri $ \fp -> do

-- ---------------------------------------------------------------------

newtype HlintDataDir = HlintDataDir
{ getHlintDataDir :: Maybe FilePath
}
deriving (Show, Eq, Read, Ord)


instance ExtensionClass HlintDataDir where
initialValue = HlintDataDir Nothing

data InitialisationError
= NoDataFiles FilePath
| InvalidDataDir FilePath
| Other

ppInitialisationError :: InitialisationError -> String
ppInitialisationError (NoDataFiles fp) =
"We could not find the data-files at: \"" <> fp <> "\""
ppInitialisationError (InvalidDataDir fp) =
"The location \""
<> fp
<> "\" does not exist. We obtained that location from the environment variable: "
<> hlintDataDirEnvironmentVariable
ppInitialisationError Other = "We could not determine why it failed."

data Tool = Stack | Cabal

showTool :: Tool -> String
showTool Stack = "stack"
showTool Cabal = "cabal"

hlintName :: String
hlintName = "hlint-" ++ VERSION_hlint

hlintDataDirDefaultLocation :: MonadIO m => m FilePath
hlintDataDirDefaultLocation = liftIO $ getXdgDirectory XdgData ("hie" </> "hlint")
Comment on lines +155 to +159
Copy link
Collaborator Author

@fendor fendor Jan 2, 2020

Choose a reason for hiding this comment

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

I feel like we have to duplicate this code in install.hs, which I think is problematic, especially since we have to VERSION_hlint no access. Any ideas @jneira?

Copy link
Member

Choose a reason for hiding this comment

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

we are using the same version of hlint for all ghc versions, right? It is hardcoded in all stack-*.yaml, so maybe it is not a big deal hardcoding it in the script? Not very elegant, though.

Mmm maybe overriding the environment variable hlint_datadir defined by cabal in Paths_hlint.hs before installing hie, so hlint data files will go in that location (not sure it works that way)


hlintDataDirEnvironmentVariable :: String
hlintDataDirEnvironmentVariable = "HIE_HLINT_DATADIR"

initializeHlint :: IdeGhcM (Either InitialisationError FilePath)
initializeHlint = do
explicitDataDirLocation <- liftIO $ lookupEnv hlintDataDirEnvironmentVariable
hieHlintDataDir <- hlintDataDirDefaultLocation
dataDirExists <- liftIO (doesFileExist hieHlintDataDir)
result <- case explicitDataDirLocation of
Just explicitDir -> do
liftIO (doesFileExist explicitDir)
>>= \case
True ->
return $ Right explicitDir
Copy link
Collaborator Author

@fendor fendor Jan 2, 2020

Choose a reason for hiding this comment

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

Add a check if the directory contains any files.

False ->
return $ Left $ InvalidDataDir explicitDir
Nothing ->
if dataDirExists
then return $ Right hieHlintDataDir
else return $ Left $ NoDataFiles hieHlintDataDir

case result of
Right dataDir -> put $ HlintDataDir $ Just dataDir
_ -> return ()

return result

downloadHlintDatafiles :: IO ()
downloadHlintDatafiles = do
defaultLocation <- hlintDataDirDefaultLocation
availTools <- liftIO
$ filterM (fmap isJust . findExecutable . showTool) [Stack, Cabal]
case availTools of
[] -> return ()
tool : _ -> withSystemTempDirectory "hie-hlint-datafiles" $ \tmpDir -> do
let toolArgs t = case t of
Cabal -> ["get", hlintName]
Stack -> ["unpack", hlintName]
Comment on lines +197 to +198
Copy link
Collaborator Author

@fendor fendor Jan 2, 2020

Choose a reason for hiding this comment

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

What if the cabal is not updated and doesnt know about the hlint version?
What if the hlint version is not on stackage? (very unlikely, just feels like not good software engineering)

Copy link
Member

@jneira jneira Jan 2, 2020

Choose a reason for hiding this comment

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

Mmm and what about download the data file directly from https://raw.githubusercontent.com/ndmitchell/hlint/v2.2.5/data/hlint.yaml?
Are the rest of files inside data needed for the hie usage?
Hardcoding the url is not ideal, but it uses fewer moving parts

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that is also an option. Just making sure that we get the one as per the version of hlint we are using in the build. Probably the best option.


let process =
(proc (showTool tool) (toolArgs tool)) { cwd = Just tmpDir }

(exitCode, _sout, _serr) <- liftIO
$ readCreateProcessWithExitCode process ""

case exitCode of
ExitFailure _ ->
error
$ "Command `"
++ unwords (showTool tool : toolArgs tool)
++ "` failed."
ExitSuccess -> do
let dirPath = tmpDir </> hlintName </> "data"
dataFiles <- liftIO $ listDirectory dirPath
forM_ dataFiles
$ \dataFile -> do
let absoluteFile = dirPath </> dataFile
logm $ "Copy File \"" ++ absoluteFile ++ "\" to \"" ++ defaultLocation ++ "\""
liftIO $ copyFile absoluteFile defaultLocation


-- AZ:TODO: Why is this in IdeGhcM?
lint :: Uri -> IdeGhcM (IdeResult PublishDiagnosticsParams)
lint uri = pluginGetFile "lint: " uri $ \fp -> do
Expand Down Expand Up @@ -238,8 +351,9 @@ ss2Range ss = Range ps pe

applyHint :: FilePath -> Maybe OneHint -> (FilePath -> FilePath) -> IdeM (Either String WorkspaceEdit)
applyHint fp mhint fileMap = do
dataDir :: HlintDataDir <- get
runExceptT $ do
ideas <- getIdeas fp mhint
ideas <- getIdeas dataDir fp mhint
let commands = map (show &&& ideaRefactoring) ideas
liftIO $ logm $ "applyHint:apply=" ++ show commands
-- set Nothing as "position" for "applyRefactorings" because
Expand Down Expand Up @@ -270,9 +384,9 @@ applyHint fp mhint fileMap = do
throwE (show err)

-- | Gets HLint ideas for
getIdeas :: MonadIO m => FilePath -> Maybe OneHint -> ExceptT String m [Idea]
getIdeas lintFile mhint = do
let hOpts = hlintOpts lintFile (oneHintPos <$> mhint)
getIdeas :: MonadIO m => HlintDataDir -> FilePath -> Maybe OneHint -> ExceptT String m [Idea]
getIdeas dataDir lintFile mhint = do
let hOpts = hlintOpts dataDir lintFile (oneHintPos <$> mhint)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do more locations need to be updates?

ideas <- runHlint lintFile hOpts
pure $ maybe ideas (`filterIdeas` ideas) mhint

Expand All @@ -285,12 +399,14 @@ filterIdeas (OneHint (Position l c) title) ideas =
ideaPos = (srcSpanStartLine &&& srcSpanStartColumn) . ideaSpan
in filter (\i -> ideaHint i == title' && ideaPos i == (l+1, c+1)) ideas

hlintOpts :: FilePath -> Maybe Position -> [String]
hlintOpts lintFile mpos =
hlintOpts :: HlintDataDir -> FilePath -> Maybe Position -> [String]
hlintOpts dataDir lintFile mpos =
let
posOpt (Position l c) = " --pos " ++ show (l+1) ++ "," ++ show (c+1)
opts = maybe "" posOpt mpos
in [lintFile, "--quiet", "--refactor", "--refactor-options=" ++ opts ]
in [ lintFile, "--quiet", "--refactor", "--refactor-options=" ++ opts
]
++ [ "--datadir=" <> dir | HlintDataDir (Just dir) <- [dataDir] ]

runHlint :: MonadIO m => FilePath -> [String] -> ExceptT String m [Idea]
runHlint fp args =
Expand Down
22 changes: 22 additions & 0 deletions src/Haskell/Ide/Engine/Server.hs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,29 @@ reactor inp diagIn = do
callback (Just db) = flip runReaderT renv $ do
reactorSend $ NotLogMessage $
fmServerLogMessageNotification J.MtLog $ "Using hoogle db at: " <> T.pack db

let hlintReq = GReq tn "init-hlint" Nothing Nothing Nothing
hlintCallback (Left ApplyRefact.Other) $ do
logm "Initialise hlint"
res <- ApplyRefact.initializeHlint
logm $ "Finished hlint initialisation: " ++ either ApplyRefact.ppInitialisationError show res
return $ IdeResultOk res

hlintCallback :: Either ApplyRefact.InitialisationError FilePath -> R ()
hlintCallback (Left err) = flip runReaderT renv $ do
reactorSend
$ NotShowMessage
$ fmServerShowMessageNotification J.MtWarning
$ "Required HLint files are missing: " <> T.pack (ApplyRefact.ppInitialisationError err)
logm "Downloading hlint data-files now."
liftIO ApplyRefact.downloadHlintDatafiles
logm "Finished downloading."
Comment on lines +474 to +476
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should probably be messages to the user.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, progress status messages in the status bar would be great for that

hlintCallback (Right dataDir) = flip runReaderT renv $
reactorSend $ NotShowMessage $
fmServerShowMessageNotification J.MtLog $ "Using hlint data-files at: " <> T.pack dataDir

makeRequest hreq
makeRequest hlintReq

-- -------------------------------

Expand Down