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

keithfancher · 2024-01-29T00:19:01Z

Stan expects relative paths. Without this change, file names won't map correctly to their associated language extension data, which means no enabled extensions will be detected. This causes annoying false positives with, e.g., the StrictData extension. (See issue #3174.)

Note that the Stan plugin appears to be disabled by default again, so if you'd like to test this change yourself you'll have to enable it in your HLS config.

I've tested this locally and it seems to be working well. Stan is once again respecting both extensions enabled in .cabal files and in LANGUAGE pragmas.

I've also done a bit of debugging in the CLI version of Stan (which doesn't have this bug), and verified that the file paths in all the associated maps are in fact relative.

I'm new-ish to Haskell and I've never contributed to HLS or any "real" Haskell project before, so please let me know if I'm doing something wacky/bad/etc here.

Thanks!

Stan expects relative paths. Without this change, file names won't map correctly to their associated language extension data, which means no enabled extensions will be detected. This causes annoying false positives with, e.g., the `StrictData` extension. (See issue haskell#3174.)

keithfancher · 2024-01-29T00:24:46Z

plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs

- 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


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?

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?).

keithfancher · 2024-01-29T00:27:45Z

plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs

+ -- 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


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.)

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

0rphee · 2024-01-29T00:54:03Z

Everything seems fine, thanks! I'll test this later today or tomorrow. Just in case, I think it would be appropriate to add a test case to guarantee that stan uses pragmas and flags from .cabal files!

keithfancher · 2024-01-29T02:14:16Z

Everything seems fine, thanks! I'll test this later today or tomorrow.

Awesome, thank you!

Just in case, I think it would be appropriate to add a test case to guarantee that stan uses pragmas and flags from .cabal files!

Good call! I'll take a crack at adding test cases.

EDIT: Test cases added! 👍

michaelpj · 2024-01-29T09:16:51Z

Maybe make an upstream issue? I know stan is now at least a little bit maintained. This is quite a footgun!

keithfancher · 2024-01-29T21:49:53Z

plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs

+ -- 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)],
 [])


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.)

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?

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.)

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

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:

keithfancher · 2024-01-29T21:53:49Z

Maybe make an upstream issue? I know stan is now at least a little bit maintained. This is quite a footgun!

Also a good call! It seems like it'd be a quick fix for Stan to normalize paths coming in, make them relative if that's what it expects. Or something like that...

0rphee · 2024-01-30T00:43:21Z

The path-issue seems to have arose because the original code for the plugin always used the absolute path. It never occurred to me to change that. It would definitely be nice to normalize the paths from stan itself, or at least add a bit of documentation there!

We specifically want to test this diagnostic, so we need it to fire.

…sions Includes test cases for both `LANGUAGE` pragmas and extensions enabled in a project's `.cabal` file.

These changes ensure that the tests will fail given bad mappings in either the `cabalExtensionsMap` OR the `checksMap`. Either of these could cause bad behavior as seen in issue haskell#3174.

keithfancher · 2024-01-31T00:56:25Z

Just a quick update here: test cases should be good to go. I've verified that the new tests fail with the previous version of the plugin. Should definitely catch this issue or others like it moving forward 🎉

~~Only only question left to clear up (see #4023 (comment) ), and then I think this should be ready for merging!~~

EDIT: Above discussion is figured out, final changes pushed. Should be good to go unless anybody has any objections or spots any new issues with the code!

The Stan plugin will still operate as expected even if we can't load a config -- it will simply default to showing all inspections.

0rphee · 2024-02-01T04:10:15Z

Everything is fine by me, thanks!

keithfancher · 2024-02-01T21:45:55Z

How odd, these seem like legitimate (Windows-specific?) test failures. But the same tests passed after I merged master in ~16 hours ago. It does look there have been some test suite changes merged in today...

EDIT: Oh, maybe something was awry after all. The previous runs look pretty weird:

All 0 tests passed (0.01s)

😬

michaelpj · 2024-02-01T22:03:38Z

Yes, CI was a bit broken before. I would guess that those are real failures, especially since you're doing some things with paths!

…tors Related to (what I assume is) a bug in Stan, or its `extensions` library. Regardless of OS, the `hs-source-dirs` field is prepended as-is to the module name to create the file paths used in the cabal extensions map. This means the maps won't work in Windows if your cabal file contains `/` path separators. Working around the limitation here to ensure tests work on all platforms.

keithfancher · 2024-02-02T04:56:43Z

I would guess that those are real failures, especially since you're doing some things with paths!

Yes indeed! I scared up a Windows VM and did some debugging, turned out to be quite a strange issue. The TL;DR is: Stan builds its extension map by sticking the hs-source-dirs value verbatim onto the module names. This means that if your .cabal file contains / as directory separators, Stan's extension map is broken in Windows.

Good new is: the HLS plugin code is fine, and the tests are sound. And in fact, the new tests uncovered this very real issue! 😄 I just had to work around it by collapsing my test data directory structure.

I don't have a ton of context for how .cabal files are supposed to work (mine have all been auto-generated so far!), but this seems like a bug to me. Likely in the kowainik extension package, probably around here.

Or maybe this is a known thing? I do see a #if MIN_VERSION_Cabal(3,6,0) check... maybe it works fine with higher versions of Cabal? I'm happy to file an issue on the extensions repo though, if this is a legit bug.

michaelpj · 2024-02-02T09:22:42Z

Great, do please open an upstream issue, even if it's not a heavily maintained package. It really helps if these things come up again!

keithfancher · 2024-02-03T00:32:50Z

Maybe make an upstream issue? I know stan is now at least a little bit maintained. This is quite a footgun!

Great, do please open an upstream issue, even if it's not a heavily maintained package. It really helps if these things come up again!

Filed issues for both! For Stan and extensions, respectively. They're linked above thanks to Github magic 😄

keithfancher requested a review from 0rphee as a code owner January 29, 2024 00:19

keithfancher commented Jan 29, 2024

View reviewed changes

keithfancher added 3 commits January 29, 2024 22:03

Un-exclude Stan diagnostics related to StrictData

4e95308

We specifically want to test this diagnostic, so we need it to fire.

Add tests to ensure the Stan plugin detects a module's language exten…

802b46c

…sions Includes test cases for both `LANGUAGE` pragmas and extensions enabled in a project's `.cabal` file.

Tighten up Stan plugin language extension test cases

c0dac2e

These changes ensure that the tests will fail given bad mappings in either the `cabalExtensionsMap` OR the `checksMap`. Either of these could cause bad behavior as seen in issue haskell#3174.

Use correct extension/file mappings even in the case of a config fiasco

e6a8520

The Stan plugin will still operate as expected even if we can't load a config -- it will simply default to showing all inspections.

0rphee approved these changes Feb 1, 2024

View reviewed changes

Merge branch 'master' into fix-stan-language-extensions

430c82d

michaelpj added the merge me Label to trigger pull request merge label Feb 1, 2024

michaelpj and others added 2 commits February 1, 2024 18:14

Merge branch 'master' into fix-stan-language-extensions

36e8877

Merge branch 'master' into fix-stan-language-extensions

afa2b9b

keithfancher and others added 5 commits February 1, 2024 16:03

Remove a slew of unused imports

188c767

Use OS-agnostic path separators in tests

d028cd6

Run stylish-haskell

fc2aff0

Merge branch 'master' into fix-stan-language-extensions

1c5e034

mergify bot merged commit 5c11636 into haskell:master Feb 2, 2024
41 checks passed

This was referenced Feb 3, 2024

Stan should normalize incoming file paths to be relative kowainik/stan#556

Open

Module paths with / as separator are broken in Windows kowainik/extensions#93

Open

keithfancher mentioned this pull request Feb 26, 2024

Stan plugin recommends StrictData pragma even when it is in scope #3174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

keithfancher commented Jan 29, 2024 •

edited

Loading

keithfancher Jan 29, 2024

0rphee Jan 29, 2024

keithfancher Jan 29, 2024

0rphee Jan 29, 2024

0rphee commented Jan 29, 2024

keithfancher commented Jan 29, 2024 •

edited

Loading

michaelpj commented Jan 29, 2024

keithfancher Jan 29, 2024

0rphee Jan 30, 2024

keithfancher Jan 31, 2024

0rphee Jan 31, 2024

keithfancher Jan 31, 2024

keithfancher commented Jan 29, 2024

0rphee commented Jan 30, 2024

keithfancher commented Jan 31, 2024 •

edited

Loading

0rphee commented Feb 1, 2024

keithfancher commented Feb 1, 2024 •

edited

Loading

michaelpj commented Feb 1, 2024

keithfancher commented Feb 2, 2024

michaelpj commented Feb 2, 2024

keithfancher commented Feb 3, 2024

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

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

Conversation

keithfancher commented Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0rphee commented Jan 29, 2024

keithfancher commented Jan 29, 2024 • edited Loading

michaelpj commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithfancher commented Jan 29, 2024

0rphee commented Jan 30, 2024

keithfancher commented Jan 31, 2024 • edited Loading

0rphee commented Feb 1, 2024

keithfancher commented Feb 1, 2024 • edited Loading

michaelpj commented Feb 1, 2024

keithfancher commented Feb 2, 2024

michaelpj commented Feb 2, 2024

keithfancher commented Feb 3, 2024

keithfancher commented Jan 29, 2024 •

edited

Loading

keithfancher commented Jan 29, 2024 •

edited

Loading

keithfancher commented Jan 31, 2024 •

edited

Loading

keithfancher commented Feb 1, 2024 •

edited

Loading