kowainik / stan

🕵️ Haskell STatic ANalyser
https://kowainik.github.io/projects/stan
Mozilla Public License 2.0
562 stars 48 forks source link

How am I supposed to fix STAN-0103 diagnostic? #550

Open Bodigrim opened 6 months ago

Bodigrim commented 6 months ago

I have a call of length xs in the middle of a function, which Stan is unhappy with because of STAN-0103: "Usage of the length function that hangs on infinite lists".

How am I supposed to act on this? Stan offers two suggestions:

I personally do not think that STAN-0103 is worth to complain about, but that's fine, we can agree to disagree. I don't mind opinionated diagnostics, I do mind unactionable ones: if a policy mandates to maintain a clean Stan report, it's virtually impossible to do so.

tomjaguarpaw commented 6 months ago

As far as I can tell the intention of this diagnostic is to encourage users never to use length on a list. That's also consistent with the Kowainik ecosystem which uses slist pervasively, including in stan itself. If it was only suggesting a non-local transformation then it would be debatable whether that is too much for a linting suggestion (cf. suggesting rewriting to NonEmpty if you attempt to use head). However, I'm skeptical whether slist is much safer. For example, concat on Slist has the same problem as length on lists.

stan is configurable so anyone who doesn't like this particular diagnostic can disable it for their codebase. I don't know whether it's it should be disabled by default but would be interested in hearing the opinion of other stan users.

(N.B. I'm a maintainer of the Kowainik ecosystem in the sense that I volunteer to keep it building as newer GHCs are released. I didn't author any of the ecosystem and I don't have much spare bandwidth to make any changes beyond keeping it building. I'm not sure if I have moral authority to change the defaults of stan. I'd have to check with Veronika if it comes to that.)

Bodigrim commented 6 months ago

stan is configurable so anyone who doesn't like this particular diagnostic can disable it for their codebase. I don't know whether it's it should be disabled by default but would be interested in hearing the opinion of other stan users.

IMO the entire STAN-010X range, prohibiting reverse, isSuffixOf, length, genericLength, sum and product, is not a good default.

It's probably fine for applications to use slist extensively, but it's almost never an option for a library. E. g., in my case I have a function

foo :: FilePath -> IO ()
foo fn = if length fn > 255 then do_this else do_that

I cannot reasonably change the type of foo (clients expect to pass FilePath) and replacing length fn with Slist.size (Slist.slist xs) does not make anything safer.


The thing is that HLS 2.5.0.0 enabled hls-stan-plugin by default to all users. I can only imagine confusion of newcomers, who are writing their first HelloWorld.hs and suddenly told that their textbook length, reverse and sum are no good. The fact that HLS currently ignores .stan.toml (https://github.com/haskell/haskell-language-server/issues/3904) makes it even worse.

If stan is to maintain its current defaults, I'll probably argue that hls-stan-plugin should be disabled by default. CC @0rphee.

tomjaguarpaw commented 6 months ago

If stan is to maintain its current defaults, I'll probably argue that hls-stan-plugin should be disabled by default. CC @0rphee.

Your overall preference regarding STAN-010X is a reasonable one, but if this issue is motivated by stan's presence in HLS then the discussion belongs on the HLS issue tracker. There's no reason that HLS's stan needs to have the same defaults as the command line tool (although perhaps this repo could help by incorporating HLS's default config file).

noughtmare commented 5 months ago

I have a function

foo :: FilePath -> IO ()
foo fn = if length fn > 255 then do_this else do_that

You can make it lazier like this:

foo :: FilePath -> IO ()
foo fn = if null (drop 255 fn) then do_that else do_this
Bodigrim commented 5 months ago

That's a good point and it would be a much better suggestion, thanks.

I wonder if a lazy compareLength :: [a] -> Int -> Ordering would be a good addition to base. There is prior art of similar functions in bytestring and text.