Skip to content

Use relative file paths for HIE files and Stan's config maps #4023

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 13 commits into from
Feb 2, 2024
Merged

Use relative file paths for HIE files and Stan's config maps #4023

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9509b27
Use relative file paths for HIE files and Stan's config maps
keithfancher Jan 28, 2024
4e95308
Un-exclude Stan diagnostics related to `StrictData`
keithfancher Jan 30, 2024
802b46c
Add tests to ensure the Stan plugin detects a module's language exten…
keithfancher Jan 30, 2024
c0dac2e
Tighten up Stan plugin language extension test cases
keithfancher Jan 31, 2024
e6a8520
Use correct extension/file mappings even in the case of a config fiasco
keithfancher Jan 31, 2024
430c82d
Merge branch 'master' into fix-stan-language-extensions
keithfancher Feb 1, 2024
36e8877
Merge branch 'master' into fix-stan-language-extensions
michaelpj Feb 1, 2024
afa2b9b
Merge branch 'master' into fix-stan-language-extensions
mergify[bot] Feb 1, 2024
188c767
Remove a slew of unused imports
keithfancher Feb 2, 2024
d028cd6
Use OS-agnostic path separators in tests
keithfancher Feb 2, 2024
fc2aff0
Run `stylish-haskell`
keithfancher Feb 2, 2024
1c5e034
Merge branch 'master' into fix-stan-language-extensions
mergify[bot] Feb 2, 2024
c24f564
Ensure `hs-source-dirs` in test cabal files don't contain path separa…
keithfancher Feb 2, 2024
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
21 changes: 11 additions & 10 deletions plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,24 +164,25 @@ rules recorder plId = do
logWith recorder Debug (LogDebugStanEnvVars env)
seTomlFiles <- liftIO $ usedTomlFiles useDefConfig (stanArgsConfigFile stanArgs)

-- Note that Stan works in terms of relative paths, but the HIE come in as absolute. Without
-- making its path relative, the file name(s) won't line up with the associated Map keys.
relativeHsFilePath <- liftIO $ makeRelativeToCurrentDirectory $ fromNormalizedFilePath file
let hieRelative = hie{hie_hs_file=relativeHsFilePath}

(cabalExtensionsMap, checksMap, confIgnored) <- case configTrial of
FiascoL es -> do
logWith recorder Development.IDE.Warning (LogWarnConf es)
pure (Map.empty,
HM.fromList [(LSP.fromNormalizedFilePath file, inspectionsIds)],
[])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I almost forgot to ask: I wasn't sure about desired behavior if we hit this branch.

Of course I could change this file path to relativeHsFilePath and even build a cabalExtensionsMap, I think... but when I checked the corresponding flow in Stan-proper, they seem to bail entirely in the case of a Fiasco here. (Assuming I'm reading the code correctly.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you read the code correctly. Stan stops completely with an error.

We could use this opportunity to define what the plugin should do. When I added the capability for reading the config, I left the previous behavior (stan runs without config) on the branch where something is wrong here, however we could instead stop the analysis completely.

The latter could be preferred in the hypothetical case where there is a bad config, and the user would just prefer to ignore possibly annoying suggestions (that would be ignored taking into account a well formed .stan.toml).

However, I'm not partial to any choice. Any suggestions @michaelpj?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Yeah, I could see a good argument for going either way.

As you say, it could be annoying for a user to see a bunch of irrelevant diagnostics. But on the other hand, it could be a bad experience for the plugin to just fail silently and for the user to see nothing at all. They could break a config and they might not even notice. (Talking through it, that latter case might be worse in the end?)

In any case, if the intended previous behavior was for it to still run sans config, I'd be fine just defaulting to that. (In which case, I'd have to make some minor tweaks to the code, fix the mappings there as well.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's stick to running diagnostics anyway and building the cabalExtensionsMap, so that it gets taken into account!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's stick to running diagnostics anyway and building the cabalExtensionsMap, so that it gets taken into account!

Done! Pushed up e6a8520 to take care of that.

Interestingly, I couldn't "organically" hit that code path from the plugin no matter what I tried -- I wound up just forcing the fiasco for testing purposes.

Seems like Stan will always either 1) throw an IO exception (in the case of a TOML parse error), or 2) recover and return some variety of partial/default config. But that path works properly now, just in case that should ever change 😄

On the plus side (?!), the "invalid config" case is extremely loud and obvious, thanks to its exception-throwing. Basically brings everything screeching to a halt. Here's how it looks for me in neovim:

hls-stan-toml-parse-error

ResultL warnings stanConfig -> do
let currentHSAbs = fromNormalizedFilePath file -- hie_hs_file hie
currentHSRel <- liftIO $ makeRelativeToCurrentDirectory currentHSAbs
cabalExtensionsMap <- liftIO $ createCabalExtensionsMap isLoud (stanArgsCabalFilePath stanArgs) [hie]

-- Files (keys) in checksMap need to have an absolute path for the analysis, but applyConfig needs to receive relative
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Files (keys) in checksMap need to have an absolute path for the analysis

This didn't seem to be the case, when I looked?

I think this logic was added in #3914 ... perhaps @0rphee could confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this was added in that PR. As I remember it, If I used relative paths, it didn't work, because of how an internal function works in runAnalysis, that checks if an absolute path to a file, begins with another filepath, (currentHSAbs I think?).

-- filepaths to apply the config, because the toml config has relative paths. Stan itself seems to work only in terms of relative paths.
let checksMap = HM.mapKeys (const currentHSAbs) $ applyConfig [currentHSRel] stanConfig

let analysis = runAnalysis cabalExtensionsMap checksMap (configIgnored stanConfig) [hie]
-- A Map from *relative* file paths (just one, in this case) to language extension info.
cabalExtensionsMap <- liftIO $ createCabalExtensionsMap isLoud (stanArgsCabalFilePath stanArgs) [hieRelative]
-- HashMap of *relative* file paths to info about enabled checks for those file paths.
let checksMap = applyConfig [relativeHsFilePath] stanConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked both cabalExtensionsMap and checksMap in the CLI version of Stan, and both had relative paths as keys. The HIE file path was also relative.

This seemed to have been causing the miss in the Map lookup. (e.g. looking for /path/to/some/file when the actual key was some/file.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be the reason why I initially had issues with relative/absolute paths. Thanks for the investigation!

pure (cabalExtensionsMap, checksMap, configIgnored stanConfig)
let analysis = runAnalysis cabalExtensionsMap checksMap confIgnored [hie]

let analysis = runAnalysis cabalExtensionsMap checksMap confIgnored [hieRelative]
return (analysisToDiagnostics file analysis, Just ())
else return ([], Nothing)

Expand Down