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

Simplifications to add #2

Open
jfmengels opened this issue Apr 22, 2021 · 24 comments
Open

Simplifications to add #2

jfmengels opened this issue Apr 22, 2021 · 24 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jfmengels
Copy link
Owner

jfmengels commented Apr 22, 2021

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
@jfmengels jfmengels added help wanted Extra attention is needed enhancement New feature or request labels Aug 6, 2021
@stefan-wullems
Copy link

For all sum simplifications:

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

Can become:

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

@stefan-wullems
Copy link

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
Copy link
Owner Author

jfmengels commented Mar 28, 2022

@stefan-wullems

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

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

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
Copy link

stefan-wullems commented Mar 28, 2022

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
Copy link
Collaborator

lue-bird commented Jan 6, 2023

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
Copy link
Collaborator

lue-bird commented Jan 7, 2023

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
Copy link
Owner Author

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
Copy link
Collaborator

lue-bird commented Jan 8, 2023

I could also tackle sum, product after we merge #55

@jfmengels
Copy link
Owner Author

That would be great @lue-bird ☺️

@lue-bird
Copy link
Collaborator

lue-bird commented Jan 13, 2023

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
Copy link
Owner Author

We should absolutely fix Maybe.map2-5. Good catch 👍

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 👍

To add

They all look good, I've added them 👍

@lue-bird
Copy link
Collaborator

lue-bird commented Jan 23, 2023

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 #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
Copy link
Owner Author

jfmengels commented Jan 23, 2023

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
Copy link
Collaborator

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

@jfmengels
Copy link
Owner Author

jfmengels commented Jun 24, 2023

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
Copy link
Owner Author

jfmengels commented Jul 12, 2023

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
Copy link
Collaborator

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
Copy link
Owner Author

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 mdgriffith/elm-codegen@77300db#diff-861b206a33742c6174e344b8a34db4c7f2b2892ebef89cfa5bd428d4a6c76109R451-R452)

@miniBill
Copy link
Contributor

Ooooh

@jfmengels
Copy link
Owner Author

jfmengels commented Sep 21, 2023

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
Copy link
Collaborator

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
Copy link
Owner Author

That's a good point. I'll remove it 👍

@lue-bird
Copy link
Collaborator

lue-bird commented Oct 1, 2023

I feel like we already discussed this but ↓ is wrong

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

@jfmengels
Copy link
Owner Author

Correct. I'll remove it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants