elm / compiler

Compiler for Elm, a functional language for reliable webapps.
https://elm-lang.org/
BSD 3-Clause "New" or "Revised" License
7.52k stars 662 forks source link

Crashing with Partial Applications and Partial Functions #849

Closed pseudonom closed 8 years ago

pseudonom commented 9 years ago

I reduced the test case as much as I could, but it's still a little hefty.

import Maybe (..)
import List

type Traversal s t a b = { view : s -> [a]
                         , set : [b] -> s -> t
                         }

view : Traversal s t a b -> s -> [a]
view = .view

set : Traversal s t a b -> [b] -> s -> t
set = .set

over : Traversal s t a b -> (a -> b) -> s -> t
over t f s = set t (List.map f <| view t s) s

single : a -> [a]
single a = [a]

infixr 9 ##
(##) : Traversal s t c d -> Traversal c d a b -> Traversal s t a b
p ## q = Traversal (concatMap (view q) << view p) 
                   (\bs s -> case view p s of
                       [] -> set p (List.map (set q bs) []) s
                       lc -> set p (List.map (set q bs) lc) s)

fst_' : Traversal (a, c) (b, c) a b
fst_' = Traversal (single << fst)
                  (\[b] -> (\b (_, c) -> (b, c)) b)

just_' : Traversal (Maybe a) (Maybe b) a b
just_' = Traversal (maybe [] single)
                   (\bs s -> case s of 
                               Nothing -> Nothing
                               Just _ -> Just <| head bs)

Using this code, elm-repl (and compiled code) report

over (just_' ## fst_') ((+) 3) Nothing
Error: Runtime error in module Foo (on line 29, column 43 to 51):<br/>&nbsp; &nbsp; Non-exhaustive pattern match in case-expression.<br/>&nbsp; &nbsp; Make sure your patterns cover every case!

However, if we change the line in (##)

[] -> set p (List.map (set q bs) []) s

to

[] -> set p (List.map (\x -> set q bs x) []) s

(note the extra lambda), Elm will happily compute the correct answer,

Nothing : Maybe.Maybe (number, b)

. Similarly, if we revert that change and change set from

set = .set

to

set t bs s = t.set bs s

, Elm will again compute the correct answer.

All of this is with Elm 0.13.

evancz commented 9 years ago

Are you able to run it with 0.14? I'm not planning to do patch releases for 0.13. If you make any progress making it smaller, please share that as well.

If you are interested in working on stuff like this, would you mind taking naming cues from the focus library? I get that this stuff can be cool, but I think the whole mix of stab and crazy infix ops makes it more mysterious than it needs to be. For example, a new person is probably gonna have an easier time reading this:

type Traversal big big' small small' =
    { get : big -> [small]
    , set : [small'] -> big -> big'
    }

If it can be simpler for people, I think that's a big deal.

pseudonom commented 9 years ago

Same issue occurs with 0.14. I tried to make it smaller, by inlining fst_ and just_ into ## as the monstrous

foo = Traversal (List.concatMap (single << fst) << (maybe [] single))
                (\bs s ->
                  case (maybe [] single) s of
                    [] -> case s of
                            Nothing -> Nothing
                            Just _ -> Just <| List.head (List.map ((\[b] -> (\b (_, c) -> (b, c))) bs) [])
                    lc -> case s of
                            Nothing -> Nothing
                            Just _ -> Just <| List.head (List.map ((\[b] -> (\b (_, c) -> (b, c))) bs) lc))

but with that definition, the error no longer happens.

As to lensing conventions, I'm not sure it's possible to make the library fit with Elm's philosophy. Because Lens and Prism are actually differerent types you need different functions to compose them which necessitates a sort of grammar (e.g. (@@) to compose lens and lens, (?@) to compose prism and lens). I probably won't inflict it on the Elm community.

evancz commented 9 years ago

Now that I'm over the initial shock of naming, I actually recall running into a transient issue with the focus library that was very very similar, so I'm pretty convinced there's some bug here. Perhaps we'll see it when looking at the generated JS between each case.

With naming, I think it's always possible, but you may have to struggle for ages. It took me months to end up with andThen but I think it's doable. I was thinking of how to extend Focus so that it'd let you change the types of internal parts (more like your API) and felt it'd be called FancyFocus or something.

In my personal experience, it is extremely rare to transform the type of subparts of my data structures. I do it with the AST for Elm, changing the type of variables as I augment them with additional info, but I don't think I have ever done it any other time in my experience. So I'd want to present an API that takes the relative usefulness in practice into account. Ideally people would learn Focus first, and in most cases, that'd take them really far. In some subset of cases, they'll want something extra fancy. For those particular people, they can go look at Focus.Transform that lets you change subtypes. But then we have a great story motivating the complexity. "You were using Focus and needed to transform the type."

I break my rule about infix ops in focus, but it may help to think about what human names you'd give these things, and then try to turn that into a visual depiction. I have noticed that certain symbols always look crappy, and others always look good. For me <>|+-/ tend to work and $%@#&^ tend to suck. And I think a big part of that is that the first set is more pictoral. => and |> are things that I can fit into some existing visual framework, whereas @@ is nothing to me.

evancz commented 9 years ago

Would you be interested in brainstorming together on naming stuff? In addition to making the library easier to pick up, I think I could tell a really cool story around it if we are successful. Like, "here are the principles and patterns that led to an API improvement like this." At the very least, we could learn some lessons that could be useful to others.

pseudonom commented 9 years ago

Sorry. This comment kept growing and growing.

Original bug

Perhaps we'll see it when looking at the generated JS between each case.

I actually had actually taken a brief look at the JS, but didn't see anything alarming:

The non-functional map (set q bs) version:

   _op["##"] = F2(function (p,q) {
      return A2(Traversal,
      function ($) {
         return $List.concatMap(view(q))(view(p)($));
      },
      F2(function (bs,s) {
         return function () {
            var _v0 = A2(view,p,s);
            switch (_v0.ctor)
            {case "[]": return A3(set,
                 p,
                 A2($List.map,
                 A2(set,q,bs),
                 _L.fromArray([])),
                 s);}
            return A3(set,
            p,
            A2($List.map,A2(set,q,bs),_v0),
            s);
         }();
      }));
   });

The functional map (\x -> set q bs x) version:

   _op["##"] = F2(function (p,q) {
      return A2(Traversal,
      function ($) {
         return $List.concatMap(view(q))(view(p)($));
      },
      F2(function (bs,s) {
         return function () {
            var _v0 = A2(view,p,s);
            switch (_v0.ctor)
            {case "[]": return A3(set,
                 p,
                 A2($List.map,
                 function (x) {
                    return A3(set,q,bs,x);
                 },
                 _L.fromArray([])),
                 s);}
            return A3(set,
            p,
            A2($List.map,A2(set,q,bs),_v0),
            s);
         }();
      }));
   });

Lenses

Type changing

In my personal experience, it is extremely rare to transform the type of subparts of my data structures.

I agree it's less common but I end up doing it not too infrequently with ad-hoc types like Maybe a and (a,b). Something like

lookup : Author -> Collection Book -> [(Title, [Chapter])]
lookup = ...

x : [(Title, Int)]
x = List.map (over snd_ List.length) <| lookup a c

doesn't seem impossibly contrived to me.

Naming

Would you be interested in brainstorming together on naming stuff?

Sure.

For me <>|+-/ tend to work and $%@#&^ tend to suck. And I think a big part of that is that the first set is more pictoral. => and |> are things that I can fit into some existing visual framework, whereas @@ is nothing to me.

Ah. I was thinking less pictorially and more associationally when picking operators. I chose @ for "at" (e.g. goomba ^@ health reads as "goomba view at health" (though, admittedly ^ doesn't really scream "view" to me)). And ? for prisms because they may or may not fail (and I'm pretty sure C# lets you promote Int to the analogue of Maybe Int with a ?). And # because as the number sign it made a little sense with Traversals which may target any number of elements. Admittedly, all rather weak justifications.

Abstraction boundaries

I was looking through your Focus documentation and saw this

It is possible that the concept of a Focus is harmful to code quality in that it can help you to be lax with abstraction boundaries.

which I found interesting because I think one of the reasons I value lenses is for their ability to enforce abstraction boundaries. In the spirit of "a ... story motivating the complexity":

Thermostat example
module Thermostat (Thermostat, celsius, fahrenheit, manufacturer, time) where

type Thermostat = Thermostat { temp : Float -- In Kelvins
                             , manufacturer : String
                             , time : Time
                             }

celsius : Lens' Thermostat Float
celsius = Lens (\(Thermostat {temp}) -> kToC temp)
               (\c (Thermostat t) -> Thermostat {t | temp <- cToK c})

fahrenheit : Lens' Thermostat Float
fahrenheit = Lens (\(Thermostat {temp}) -> kToF temp)
                  (\f (Thermostat t) -> Thermostat {t | temp <- fToK f})

manufacturer : Lens' Thermostat String
time : Lens' Thermostat Time

By only exporting the lenses instead of the thermostat constructor, we present an interface which is agnostic between units (the user need never know that the underlying unit is, in fact, Kelvins).

Of course, we could achieve the same effect by providing getCelsius, setCelsius, getFahrenheit, setFahrenheit, but then we're pretty much just manually constructing lenses without all the useful extras that come in a library.

evancz commented 8 years ago

I'm trying to sort this out again, here's a gist with an updated version of the example and the generated JS.

I think the root thing is not too crazy. I think I'll be able to get this down to something smaller.

evancz commented 8 years ago

I'm going to close because of no SSCCE. I don't know what's going on here, and I suspect we will see it again in the future if it persists.