search

kowainik / stan

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

STAN-0208 sounds questionable #313

Closed Bodigrim closed 4 years ago

Bodigrim commented 4 years ago
https://github.com/kowainik/stan/wiki/All-Inspections#STAN-0208 Property Value
ID STAN-0208
Name Anti-pattern: Slow 'length' for Text
Description Usage of 'length' for 'Text' that runs in linear time
Severity Performance
Category #AntiPattern

Possible solutions for STAN-0208

  • {Extra dependency} Switch to 'ByteString' from 'bytesting'

I'm probably missing something, but, putting everything else aside, it does not seem to be a valid suggestion: ByteString length counts bytes, while Text length counts Unicode characters:

> str = Data.Text.pack "ф"
> Data.Text.length str
1
> Data.ByteString.length (Data.Text.Encoding.encodeUtf8 str)
2
vrom911 commented 4 years ago

I think you mean that the possible suggestion is questionable. There are no questions that Text.length is running in linear time. We will improve the suggestion message in here.

Edit: added "running in linear time" due to the misinterpretation of my message.

Bodigrim commented 4 years ago

There is no questions that Text.length is inefficient.

Could you please elaborate? Inefficiency is a relative characteristic, and it is a bit weird to claim that something is inefficient in the absense of a semantically-equivalent substitute. Are we calling every non-constant-time function inefficient just because there could (but does not) exist another package, which pre-evaluates this function and caches the result for immediate access?

vrom911 commented 4 years ago

The inspection says everything, and you have quoted it yourself in this issue. To clarify: Usage of 'length' for 'Text' that runs in linear time. It is not an obvious fact, and I am sure, not that many people know text internals to say that length could be a non-constant operation, especially for users coming from other languages, where this operation runs in constant time. The suggestion is here to point out this fact and let people think more carefully about their code.

If you don't want to see this particular inspection, there are several ways, described in the documentation, how you can turn it off. But, again, it could help some people to reason more about their code, and it is not fair to remove it just because some users are aware of this fact.

Bodigrim commented 4 years ago

It is not an obvious fact, and I am sure, not that many people know text internals to say that length could be a non-constant operation

This fact is plainly stated in haddocks for Data.Text.length: http://hackage.haskell.org/package/text-1.2.4.0/docs/Data-Text.html#v:length. This is a rare case, when Haskell documentation is not that bad.

vrom911 commented 4 years ago

Again, it is possible to turn off inspections that you personally don't like and don't find useful for yourself :slightly_smiling_face: That is plainly stated in the docs.

It seems like the possible suggestion improvement is not the point of the issue at all. The point is in removing this particular inspection. As I already mentioned, this is not a healthy decision, and I don't see this happening without strong basis against the inspection (case when the inspection is harmful, that is not true in here).

I am closing this issue because this inspection is not going to be removed.