Skip to content
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.

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

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

Closed
lsmor opened this issue Sep 15, 2022 · 14 comments
Closed

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

lsmor opened this issue Sep 15, 2022 · 14 comments
Assignees
Labels
type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@lsmor
Copy link

lsmor commented Sep 15, 2022

Your environment

Which OS do you use?
Ubuntu 20.0.4 Focal
Which version of GHC do you use and how did you install it?
ghc-8.10.7 with ghcup
How is your project built (alternative: link to the project)?
with cabal

Which LSP client (editor/plugin) do you use?
VsCode with haskell extension
Which version of HLS do you use and how did you install it?
hls-1.8.0 I did install it with ghcup manually pointing to debian bindist, instead of default fedora as explain in this comment
Have you configured HLS in any way (especially: a hie.yaml file)?
My hie.yaml is just

cradle:
  cabal:

Steps to reproduce

on a normal cabal project write a module

{-# LANGUAGE StrictData #-}

module MyModule where

data A = A {field :: Int, field2 :: String}

Expected behaviour

No error or warnings should happen with respect to strict fields

Actual behaviour

Stan underlies the field's accessors and recommends to add the StrictData pragma. Notice that Stan also recommends adding a bang pattern, and if you do that, then the warning disappears.

Debug information

This is the last output of hls. It seems that StrictData pragma is reconigze within the module, so I guess the problem is completely up to stan plugin. This is double inconviniently because stan can't be disable or at leat I haven't found the way either: #3157

[Trace - 19:26:53] Received notification 'window/logMessage'.
Using extensions for  NormalizedFilePath "/home/luis/Proyectos/<path/to/module>": [ MonomorphismRestriction
                                                                                  , RelaxedPolyRec
                                                                                  , ForeignFunctionInterface
                                                                                  , ImplicitPrelude
                                                                                  , DoAndIfThenElse
                                                                                  , EmptyDataDecls
                                                                                  , PatternGuards
                                                                                  , DatatypeContexts
                                                                                  , TraditionalRecordSyntax
                                                                                  , EmptyCase
                                                                                  , StrictData
                                                                                  , StarIsType
                                                                                  , CUSKs ]
[Trace - 19:26:53] Received notification 'window/logMessage'.
[Info  - 19:26:53] Typechecking reverse dependencies for NormalizedFilePath "//home/luis/Proyectos/<path/to/module>": [  ]
[Trace - 19:26:53] Received notification 'window/logMessage'.
Finished: ParentTC Took: 0.00s
[Trace - 19:26:53] Received request 'window/workDoneProgress/create - (68)'.
[Trace - 19:26:53] Sending response 'window/workDoneProgress/create - (68)'. Processing request took 0ms
[Trace - 19:26:53] Received notification '$/progress'.
[Trace - 19:26:53] Received notification '$/progress'.
[Trace - 19:26:53] Received request 'window/workDoneProgress/create - (69)'.
[Trace - 19:26:53] Sending response 'window/workDoneProgress/create - (69)'. Processing request took 0ms
[Trace - 19:26:53] Received notification '$/progress'.
[Trace - 19:26:53] Received notification '$/progress'.
[Trace - 19:26:54] Received notification '$/progress'.
2022-09-15T17:27:29.314287Z | Info | Live bytes: 181.82MB Heap size: 554.70MB
@lsmor lsmor added status: needs triage type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Sep 15, 2022
@michaelpj
Copy link
Collaborator

Does this appear when running stan directly? It may just be a bug in stan.

@johnhampton
Copy link

Does this appear when running stan directly? It may just be a bug in stan.

I tried stan standalone and via HLS on this project https:/goolord/stan-stricdata-minimal. StrictData is correctly recognized when the code is checked directly with stan while the above error is reported through HLS.

@ncaq
Copy link

ncaq commented Sep 29, 2022

I was able to confirm that the warning disappeared after hard-coding as follows.

let cabalExtensionsMap = Map.fromList [(LSP.fromNormalizedFilePath file, Right $ ParsedExtensions ([On StrictData]) Nothing)]

So I thought about porting the part that reads Extension in the CLI, but when I call it in the CLI without calling it in the API, the StrictData warning did not occur even if I did not write the Pragma in the first place.
So I am finding it difficult to compare and inspect.

@michaelpj
Copy link
Collaborator

There's a fairly obvious TODO in the stan plugin where it needs extension information: https:/haskell/haskell-language-server/blob/master/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs#L78

I think the hlint plugin has the same issue and gets the extensions somehow, so it should be possible to mirror that.

@ncaq
Copy link

ncaq commented Sep 29, 2022

The .stan.toml is not loading either, so it looks like I will have to lower the hls version until this issue is resolved.

@ncaq
Copy link

ncaq commented Sep 29, 2022

I was mistaken when I said that the CLI doesn't warn without a pragma.
You have to generate the .hie to detect it.

@ncaq
Copy link

ncaq commented Sep 29, 2022

I looked at it to some extent, but I couldn't figure it out any way, so I gave up.
The analysisObservations results are inevitably different.
The only argument that differs is FilePath, so there may be a discrepancy there.

@mkohlhaas
Copy link

Is this the same issue?

ndmitchell/hlint#1432

@uhbif19
Copy link
Collaborator

uhbif19 commented Mar 17, 2023

@michaelpj

There's a fairly obvious TODO in the stan plugin where it needs extension information: https:/haskell/haskell-language-server/blob/master/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs#L78

But TODO was about cabal information, I think LANGUAGE pragmas should be different.

So currently the tasks here possibly are:

  • Respect LANGUAGE pragmas
  • Respect cabal enabled extensions
  • Respect Stan configuration

@stephen-smith
Copy link

stephen-smith commented Dec 19, 2023

Just wanted to pop in and mention this still exists on HLS 2.5.0.0. We have StrictData set in our *.cabal file:

common deps-exts
  default-extensions:
    BlockArguments
    DataKinds
    DeriveAnyClass
    DerivingStrategies
    DerivingVia
    DuplicateRecordFields
    LambdaCase
    MultiWayIf
    NumericUnderscores
    OverloadedRecordDot
    OverloadedStrings
    PatternSynonyms
    QuasiQuotes
    RecordWildCards
    StrictData

but HLS continues to report diagnostics like:

1.  ✲ Name:        Data types with non-strict fields
    ✲ Description: Defining lazy fields in data types can lead to unexpected space leaks
    ✲ Severity:    Performance
    ✲ Category:    #SpaceLeak #Syntax
   Possible solutions:
     - Add '!' before the type, e.g. !Int or !(Maybe Bool)
     - Enable the 'StrictData' extension: {-# LANGUAGE StrictData #-}

EDIT: Disabling Stan in the HLS configuration is a work around that works for me, now, since Stan is not part of our CI, yet. I'm using neovim and the provided LSP client:

lua << END_LSPCONFIG
require('lspconfig').hls.setup{ settings = { haskell =
    { plugin =
      { stan = { globalOn = false }
      }
    }
} }
END_LSPCONFIG

@michaelpj
Copy link
Collaborator

The problem was accidentally reintroduced when the stan plugin was re-added.

@keithfancher
Copy link
Contributor

Disabling Stan in the HLS configuration is a work around that works for me

Thanks for the workaround... this is a super-distracting bug! Every field of every data type is called out.

keithfancher added a commit to keithfancher/haskell-language-server that referenced this issue Jan 29, 2024
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
Copy link
Contributor

I took a stab at fixing it :D

Please see above PR if you're affected!

keithfancher added a commit to keithfancher/haskell-language-server that referenced this issue Jan 31, 2024
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.
mergify bot added a commit that referenced this issue Feb 2, 2024
* Use relative file paths for HIE files and Stan's config maps

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

* Un-exclude Stan diagnostics related to `StrictData`

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 extensions

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

* Tighten up Stan plugin language extension test cases

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 #3174.

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

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

* Remove a slew of unused imports

* Use OS-agnostic path separators in tests

* Run `stylish-haskell`

* Ensure `hs-source-dirs` in test cabal files don't contain path separators

 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.

---------

Co-authored-by: Michael Peyton Jones <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@keithfancher
Copy link
Contributor

By the way, I'm not sure what the procedure is for closing out issues in this repo, but this issue should be fixed with PR #4023. (I don't think I have permission to close this out.)

@fendor fendor closed this as completed Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
Development

No branches or pull requests

9 participants