Simplifications to add

[jfmengels]

Here are some of the simplifications I'd like to support.

If you'd like to work on some of these, please reply in a comment! If you want to suggest new ones, please open a new issue.

List

List.range n n
--> [ n ]

-- Possible but potentially quite ugly
List.foldl f x [ a ]
--> f (a) x

list |> List.reverse |> List.foldl f x
--> list |> List.foldr f x

list |> List.reverse |> List.foldr f x
--> list |> List.foldl f x

-- Only if acc is referenced only once
List.foldl (\a acc -> ... a :: acc) [] list |> List.reverse
--> List.foldr (\a acc -> ... a :: acc) [] list

-- Only if acc is referenced only once
List.foldr (\a acc -> ... a :: acc) []
--> List.map (\a -> ... a)

List.drop n (List.drop m list)
--> List.drop (Basics.max 0 n + Basics.max 0 m) -- Skip Basics.max if possible

List.take n (List.take m list)
--> List.take (Basics.max 0 (Basics.min n m)) -- Skip Basics.min/max if possible

-- Only if n >= 1
list
  |> List.take n
  |> List.head
-->
list
  |> List.head

-- Possible but potentially quite ugly
list
  |> List.partition f
  |> Tuple.second
-->
list
  |> List.filter (f >> not) -- if we can insert a `not` in a nice way, that'd be cool!

list
  |> List.indexedMap Tuple.pair -- or equivalent lambda
  |> List.map Tuple.second
--> list

Set

-- Possible but potentially quite ugly
Set.foldl f x (Set.singleton a)
--> f a x

Maybe

-- ???
Maybe.andThen (f >> Maybe.map g) x
--> (Maybe.andThen (f) >> Maybe.map g) x

[stefan-wullems]

For all sum simplifications:

List.foldl (+) 0 data — List.sum data

Can become:

List.foldl (+) n data — List.sum data + n

[stefan-wullems]

Maybe.andThen (\a_ -> Maybe.map (\b_ -> f a b) b)  a
-- Maybe.map2 f a b

Result.andThen (\a_ -> Result.map (\b_ -> f a b) b)  a
-- Result.map2 f a b

Decode.andThen (\a_ -> Decode.map (\b_ -> f a b) b)  a
-- Decode.map2 f a b

[jfmengels]

@stefan-wullems

List.foldl (+) n data — List.sum data + n

I agree with this. It was already in the list actually :slightly_smiling_face:

Maybe.andThen (\a_ -> Maybe.map (\b_ -> f a b) b) a -- Maybe.map2 f a b

These sound okay, but I think in practice quite hard/annoying to detect, especially with all the possible variants that this can be written in. Have you seen these often in practice?

[stefan-wullems]

Have you seen these often in practice?

I personally tend to write it in the first way and then realize I can write it with mapN, which results in much cleaner code imo. Perhaps this simplification would save me some brain juice.

I don’t know if other people agree that this is cleaner, or have the habit of first going for the andThen rather than going directly for mapN. Perhaps they have not made this connection in the first place and this rule would reveal it to them.

[lue-bird]

Not sure about how others will feel about

Tuple.pair a b
--> ( a, b )

Since many prefer signaling at the start what to expect later (it's kind of already the case with the elm-formatted code where the amount of whitespace is different to normal parenthesis)

Using Tuple.pair will be an explicit choice anyway because elm folks will be familiar with tuples before knowing Tuple.pair (from TEA update).

[lue-bird]

Additions have been moved to the top comment

What I really like from the simplifications you suggested is

List.fold f x (Set.toList set)
--> Set.fold f x set

[jfmengels]

Tuple.pair a b --> ( a, b ) Since many prefer signaling at the start what to expect later

I'm not sure what you mean here. I can imagine some cases where you wouldn't want this, like with piping. b |> f |> Tuple.pair a should probably not be changed to ( a, b |> f ). Though we can skip the error when pipes are involved. Other than that, I'm not sure I can see some cases where the tuple pattern would not be preferred to Tuple.pair.

[lue-bird]

I could also tackle sum, product after we merge https://github.com/jfmengels/elm-review-simplify/pull/55

[jfmengels]

That would be great @lue-bird :relaxed:

[lue-bird]

Wrong simplifications:

Maybe.map2 f (Just a) (Just b) -- same for up to map5
--> f (a) (b)

should be

Maybe.map2 f (Just a) (Just b) -- same for up to map5
--> Just (f (a) (b))

member a [ a, b ] --> True is weird with values containing NaN == NaN --> False. Tho we can always just assume it doesn't have NaN, or stuff that crashes elm like functions etc. because the rule would be very helpful most of the time (arguments are similar to #58)

Some parens are missing as well:

Maybe.andThen (f >> Maybe.map g) x
--> (Maybe.andThen (f) >> Maybe.map g) x

should be fixed to

--> (Maybe.andThen (f) >> Maybe.map (g)) x

test for yourself with for example

> identity >> Maybe.map <| (+) 3
<function> : Maybe number -> Maybe number

In almost all cases you really can't mix those, tho

Maybe.map <| (+) 3 >> Maybe.map <| (+) -3 -- error

To add:

Task can pretty much mirror every Result/Json.Decode simplification (which cries for a common helper like

failSuccessChecks : { succeed : String, fail : String, moduleName : String, qualify : Bool } -> ...

)

[jfmengels]

We should absolutely fix Maybe.map2-5. Good catch :+1:

member a [ a, b ] --> True is weird with values containing NaN == NaN --> False

Man... NaN is being a pain :/ I guess we could decide like for #58 to not provide a fix and change the error message to tell what to do if there is a NaN?

Similarly, all the a == (a) simplifications we do can become problematic as well. I'm thinking that maybe we should have a configuration setting for the rule that basically says "I'm never going to do anything with NaN" or the opposite. In which case we can disable the fix for the relevant ones, but keep them if you don't intend to use NaN.

Maybe.andThen (f >> Maybe.map g) x --> (Maybe.andThen (f) >> Maybe.map g) x

I don't think this needs parens around g. But I might be a bit confused by your example, since they don't use Maybe.andThen and therefore I don't really see the connection.

Already implemented

I removed them now :+1:

To add

They all look good, I've added them :+1:

[lue-bird]

I don't think this needs parens around g. But I might be a bit confused by your example, since they don't use Maybe.andThen and therefore I don't really see the connection.

Imagine changing the shape of the the argument function in

Maybe.andThen (f >> Maybe.map g) x
--> (Maybe.andThen (f) >> Maybe.map g) x

looking like ↓

Maybe.andThen (f >> Maybe.map <| g b) x
--> (Maybe.andThen (f) >> Maybe.map g b) x

The unsimplified code would compile, but not putting parens around the result is incorrect.

I guess we could decide like for https://github.com/jfmengels/elm-review-simplify/issues/58 to not provide a fix and change the error message to tell what to do if there is a NaN?

Yes. Making providing a fix a configuration option is a really unsatisfying solution to me because you'll forget about it later on.

[jfmengels]

The unsimplified code would compile, but not putting parens around the result is incorrect.

Hmm, right. I don't think we should be changing the Maybe.map ... part, but maybe it's possible that this will get weird in some cases. Feel free to add parentheses if you think it's better. If they're unneeded then elm-format and/or other simplifications will remove the parentheses anyway. Not adding them would only serve to avoid a round-trip to elm-format. To be avoided when we're confident, otherwise it's fine to add.

[lue-bird]

Ah right! We don't change the map part, you were right

[jfmengels]

I just added the following ideas to the list

Tuple.first ( a, b )
--> a

Tuple.second ( a, b )
--> b

Tuple.pair a b
  |> Tuple.first
--> a

Tuple.pair a b
  |> Tuple.second
--> b

list
  |> List.take n
  |> List.head
-->
list
  |> List.head

list
  |> List.partition f
  |> Tuple.first
-->
list
  |> List.filter f

-- Possible but potentially quite ugly
list
  |> List.partition f
  |> Tuple.second
-->
list
  |> List.filter (f >> not) -- if we can insert a `not` in a nice way, that'd be cool!

[jfmengels]

Added the following ideas (code I just found at work):

-- Only if acc is referenced only once
List.foldl (\a acc -> ... a :: acc) [] list |> List.reverse
--> List.foldr (\a acc -> ... a :: acc) [] list

-- Only if acc is referenced only once
List.foldr (\a acc -> ... a :: acc) []
--> List.map (\a -> ... a)

[lue-bird]

This one doesn't always work:

list |> List.take n |> List.head
--> list |> List.head

For example, for

[1,2,3] |> List.take 0 |> List.head
--> Nothing
-- not Just 1

so we have to check that n >= 1

[jfmengels]

Added the following ideas:

list |> List.reverse |> List.foldl f x
--> list |> List.foldr f x

list |> List.reverse |> List.foldr f x
--> list |> List.foldl f x

(Thanks @miniBill https://github.com/mdgriffith/elm-codegen/pull/70/commits/77300db9f052935ccbf4f0b1c0e7d1e691361cd3#diff-861b206a33742c6174e344b8a34db4c7f2b2892ebef89cfa5bd428d4a6c76109R451-R452)

[miniBill]

Ooooh

[jfmengels]

Added the following ideas:

String.concat (List.repeat n x)
--> String.repeat n x

String.fromList >> String.toList
--> identity

String.toList >> String.fromList
--> identity

-- EDIT: Removed
String.join "<someprefix>" (List.repeat n x) ++ "<someprefix>and some other thing"
--> String.repeat n ("<someprefix>" ++ x) ++ "and some other thing"

[lue-bird]

String.join "<someprefix>" (List.repeat n x) ++ "<someprefix>and some other thing"
--> String.repeat n ("<someprefix>" ++ x) ++ "and some other thing"

Not sure I like this in general. The separator could have different semantic meaning than the string after append.

"Keys missing (_) and found (x): "
    ++ String.join " " (List.repeat keysToFind "_") ++ " " ++ String.join " " (List.repeat keysFound "x")
-->
"Keys missing (_) and found (x): "
    ++ String.repeat keysToFind "_ " ++ String.join " " (List.repeat keysFound "x")

I think I'd be annoyed if simplify did this to my code, personally.

[jfmengels]

That's a good point. I'll remove it :+1:

[lue-bird]

I feel like we already discussed this but ↓ is wrong

Result.andThen (always (Err e)) x
--> (Err e)

[jfmengels]

Correct. I'll remove it :+1:

[jfmengels]

I added this to the list.

list |> List.map f |> List.head
--> list |> List.head |> Maybe.map f

[lue-bird]

We already track this in https://github.com/jfmengels/elm-review-simplify/issues/112 I thought

[jfmengels]

We do... I was surprised not to see it, but I did search for it :sweat_smile: I'll remove it from this one.