Simplifications for Parser

[jfmengels]

• Parser.map simplifications
• Parser.andThen simplifications
• Parser.succeed () |. p -> p
  • Only if p is a Parser ()
  • For styling purposes, should only be done when there is only a single pipe in the expression, and is at the beginning of the chain
• Parser.succeed identity |= p -> p
  • Same styling requirements as above
• Parser.succeed x |. Parser.succeed y -> Parser.succeed x
• Parser.succeed always |= p |= q -> Parser.succeed identity |. p |= q
• Parser.succeed (\_ y -> x) |= p -> Parser.succeed (\y -> x) |. p (Note, may need import of the |. operator)
• Parser.chompIf (always False) -> Parser.succeed ()
• Parser.chompWhile (always False) -> Parser.succeed ()
• Parser.mapChompedString (always f) p -> Parser.map f p
• Parser.getChompedString (Parser.succeed x) -> Parser.succeed ""

As far as I can tell, all of these would also apply to Parser.Advanced.

[lue-bird]

Parser.succeed (\_ y -> x) |= p
--> Parser.succeed (\y -> x)

needs to be

Parser.succeed (\_ y -> x) |= p
--> Parser.succeed (\y -> x) |. p

(because it will still consume ignored parsers)

And

Parser.mapChompedString (always f) p
--> p

should be

Parser.mapChompedString (always f) p
--> Parser.map f p
-- or alternatively (probably less nice in this context)
--> Parser.succeed f |= p

And maybe add

Parser.mapChompedString (\string parsed -> ..string, parsed..) (Parser.succeed a)
--> Parser.succeed (.."", a..)

[miniBill]

Parser.succeed x |. p -> p

1. This should read Parser.succeed () |. p -> p
2. I think this should only happen if there are no other parsers in the pipeline succeed () |. a |. b |. c should not simplify to a |. b |. c (especially because the former scans better)
3. This is only valid if p is a Parser (). Before we get type inference, we can know it statically about chompIf, chompWhile, ...

Parser.succeed (\_ y -> x) |= p -> Parser.succeed (\_y -> x)
As @lue-bird said, you need to keep the |. p

Parser.mapChompedString (always x) p -> Parser.succeed ()
This should read Parser.mapChompedString (always x) p -> Parser.succeed x |. p (which may then be further simplified)

[miniBill]

As @wolfadex mentioned:
Parser.succeed identity |= p -> p, again only if we only have a single pipe

[Janiczek]

Parser.succeed x |. p -> p

is not a valid simplification. You'd lose the x and instead use result of p that was previously skipped. Maybe you meant |= instead of |.?

[jfmengels]

Thanks all for the feedback and corrections! Please double-check I didn't mess up anything.

@lue-bird I didn't get the last mapChompedString example. Could you clarify it more?

@miniBill Good points about Parser.succeed x needing to be (), I came to the same conclusion. We could potentially do it if we know that x is the same as the value given by p (Parser.succeed "abc" |. Parser.succeed "abc"), but that can be quite tricky. That said, I will add Parser.succeed x |. Parser.succeed y -> Parser.succeed x to the list.

@miniBill I've also changed the Parser.mapChompedString (always f) p simplification to use Parser.map instead. Let me know if you think that that is correct.

@Janiczek I don't think it works with |=, because it would mean that x has to be a function, likely meaning that it's doing something. I think the exception to that is Parser.succeed identity |= p mentioned by @wolfadex and @miniBill (now added to the list).

I've also added Parser.succeed always |= p |= q -> Parser.succeed identity |. p |= q`.

[lue-bird]

An example of how the mapChompedString would trigger:

Parser.mapChompedString (\string parsed -> f string parsed) (Parser.succeed a)
--> Parser.succeed (f "" a)

because the string will always be "" and the parsed value will be the same as the one given to succeed.
So what we do is take the result of the lambda and replace the first arguments with "" and the second arguments with a.

[Janiczek]

This was raised in the Slack thread as well, but: I think the Parser.succeed () |. p -> p and Parser.succeed identity |= p -> p rules might be controversial and unwanted by some (me included) - go against a self-imposed "consistency" rule where most all parsers follow the shape

myParser =
  Parser.succeed {- value or function -}
     {- |. or |= -}

[jfmengels]

@Janiczek My idea (as suggested on Slack) is to only apply these changes "when there is only a single pipe in the expression"

Examples:

myParser0 =
  Parser.succeed f
    |. p
-->
myParser0 =
  p

myParser1 =
  Parser.succeed identity
    |= p
-->
myParser1 =
  p

-- Unchanged
myParser2 =
  Parser.succeed f
    |. p
    |= q

Would this not suit you? If so, do I understand correctly that you prefer:

myParser4 =
  Parser.succeed identity
    |= Parser.token "/"
-- over
myParser4 =
  Parser.token "/"

I'd say the latter is always a simplification over the former. Or are there other scenarios that you're thinking of?

[lue-bird]

A common pattern I tend to use is to use the "identity" part as a description, e.g.

Parser.succeed (\name size -> File { name = name, size = size })
    |. ...
    |. ...
    |= ...
    |. ...
    |. ...
    |= (Parser.succeed (\size -> size)
            |= Parser.int
       )
    |. ...

without the explaining identity function, it gets pretty hard to read as you have to jump around the code to know what you're actually producing.
Also makes it easy to add |.s to just the file size parser in the future, or even upgrade it to e.g. a single-variant type.

To be fair nobody does e.g. Json.Decode.map (\fieldName -> fieldName) fieldJsonDecoder so count my opinion as "this simplification is probably fine".

[Janiczek]

As @lue-bird showed, having the un-simplified succeed-ful expression makes it easier to add |. or |= lines before / after the parsed thing, and has some value, even though having p on a line by itself is a simplification.

It's probably fine, it's just stylistic, but I know you do pay attention to these stylistic issues in some rules ("why would somebody not use this") and this is potentially one of them. The other rules seem non-controversial.

[jfmengels]

@lue-bird Would a comment not serve the same purpose?

Parser.succeed (\name size -> File { name = name, size = size })
    |. ...
    |. ...
    |= ...
    |. ...
    |. ...

    -- Size
    |= Parser.int
    |. ...

Or by extracting it and giving it a name like |= sizeParser.

Also makes it easy to add |.

I absolutely agree with this. But I'd say a lot of simplifications already destroy this. For instance (and I know it's a bit odd, but for the sake of example), one could want to keep List.map identity list because that would make it easier to switch to List.map f list than if you only had list. I think that both this rule and the unused rules show that a lot of the cases where you want to keep something written in a way that makes it easier to retrieve/edit later doesn't matter much.

I'd be more in favor to not apply the simplification if there were some readability benefits.

it's just stylistic, but I know you do pay attention to these stylistic issues in some rules

@Janiczek Absolutely. I'm mostly trying to figure out whether there is a better approach that we can all agree on is better. If there is none or if there is no consensus, then I am very happy to drop this 😄
Just to explicit it, all of my questions are genuine and come out that search and of my own curiosity.

---

I guess where I come from here is that I find it hard to believe that

Parser.succeed identity -- or with lambda
  |= Parser.int

is easier to read than just Parser.int (eventually with a comment or extracted as a variable).

[Janiczek]

I've only found one example in my own code:

https://github.com/cara-lang/compiler/blob/72baa5f9a8239e6f111e1cf98d3a68ed10e3ee2a/src/Parser.elm#L2564

Parser.succeed (\right -> BinaryOp left RangeInclusive right)
    |> Parser.keep (Parser.lazy (\() -> exprAux precedence isRight))

I think I'd be OK with changing it into

Parser.lazy (\() -> ...)
    |> Parser.map (\right -> ...)

if elm-review suggested that, even though it's not how I'd have written things... 👍

[miniBill]

@Janiczek I'm confused, I don't think your example is one of the cases above? 🤔

[Janiczek]

@miniBill You can imagine it being

Parser.succeed identity
    |> Parser.keep (Parser.lazy (\() -> exprAux precedence isRight))

changing into

Parser.lazy (\() -> ...)

[miniBill]

The latter is definitely something we should do, but your example with Parser.succeed (\right -> ... I think we shouldn't touch

[lue-bird]

Parser.succeed always |= p |= q -> Parser.succeed identity |. p |= q

needs to be
Parser.succeed always |= p |= q -> Parser.succeed identity |= p |. q (|= and |. are swapped)
since it's equivalent to
Parser.succeed (\a _ -> a) |= p |= q, not Parser.succeed (\_ a -> a) |= p |= q.