sebastiaanvisser / clay

A CSS preprocessor as embedded Haskell.
Other
357 stars 72 forks source link

Color Operator Precedence #197

Closed Temurson closed 4 years ago

Temurson commented 4 years ago

I found probably unexpected behavior of operators +., -., *. in Color.hs module. Example with steps to reproduce (starting at the project root):

$ ghci
GHCi, version 8.8.1: https://www.haskell.org/ghc/  :? for help\n
Prelude> :set -isrc
Prelude> :l src/Clay/Color
[1 of 3] Compiling Clay.Property    ( src/Clay/Property.hs, interpreted )
[2 of 3] Compiling Clay.Common      ( src/Clay/Common.hs, interpreted )
[3 of 3] Compiling Clay.Color       ( src/Clay/Color.hs, interpreted )
Ok, three modules loaded.
*Clay.Color> c = white
*Clay.Color> c
Rgba 255 255 255 1.0
*Clay.Color> c -. 25 +. 5 * 5
<interactive>:7:1: error:
    • No instance for (Num Color) arising from a use of ‘*’
    • In the expression: c -. 25 +. 5 * 5
      In an equation for ‘it’: it = c -. 25 +. 5 * 5
*Clay.Color> :info +.
(+.) :: Color -> Integer -> Color
        -- Defined at src/Clay/Color.hs:123:1
*Clay.Color> c -. 25 +. (5 * 5)
Rgba 255 255 255 1.0

Note that when doing :info +. in ghci, it does not show precedence, and if you check the definition, there is no precedence there too. I suspect the problem here is that +., -. and *. (the only three defined in Color.hs) have precedence higher than *, which doesn't really make sense. I do not know what is default operator precedence in Haskell, and I always thought that it is required to specify precedence when declaring custom operators, but I could not find it within the entire Clay code. I think this should be fixed by explicitly specifying operator precedence of +., -. and *. in Color.hs.

Temurson commented 4 years ago

I have tried adding infixl 6 +., infixl 6 -. and infixl 7 *. to Color.hs, and this resolved this strange behavior. Note that these precedence values match those of regular operators. Let me know if this is something that should be resolved, and I will create a PR.

turion commented 4 years ago

That's a good find. I agree that it would be good in principle to specify these precedences in a way that makes your example expression typecheck. But this is a big change since other projects might already depend on a different precedence.

It would be great to take a large open source project that uses Clay, and see if it still typechecks with the precedences changed. Do you have an idea?

Temurson commented 4 years ago

Well, I haven't even used Clay myself, and I'm quite new to Haskell, so I don't know. I will try to do some research on that. If you know any projects, let me know, I would gladly check it.

turion commented 4 years ago

I'm not a big Clay user myself yet ;) I've just written 5 lines of it so far on my website. A good starting point is this: http://packdeps.haskellers.com/reverse/clay But I can't find any that use the color operators. If you can't find any projects using the operators prominently either, it's safe to break/change. So feel free to create a PR in that case :)

turion commented 4 years ago

I believe you solved this already in #195, right?

Temurson commented 4 years ago

@turion No, I didn't. I'm still not sure if I should make this change. I tried finding projects that use Clay, but the website you gave seems to only have those from Hackage, which are mostly libraries, while any web project could possibly use Clay, and I'm not sure if there is a way to find out.

turion commented 4 years ago

Oh, but you uploaded a commit doing this to https://github.com/sebastiaanvisser/clay/pull/195/commits, and I merged it.

I think it is fine to have this change, it just triggers a version bump. Possibly a few projects will break, but the will appear in the changelogs.

If you agree, we can close this issue for now.

Temurson commented 4 years ago

I'm so sorry! I thought I removed this change before commiting. I do agree that this change should stay now, because I, just as you, could not find any projects using this specific behavior of color operators. Thanks!

turion commented 4 years ago

No worries :) I'll close then.