pointless-lang / pointless

Pointless: a scripting language for learning and fun
https://ptls.dev
MIT License
122 stars 9 forks source link

for loops on sets don't seem to work #11

Open roetlich opened 4 years ago

roetlich commented 4 years ago

In prelude/set.ptls both difference and intersection use a for comprehension on a set. Sadly, when I tried to use them, they crash :(

It can easily be "fixed" by calling toList on the set:

 intersection(a, b) = toSet(interElems)
   where interElems =
-    for elem in a
+    for elem in toList(a)
     when elem in b
     yield elem

I could make a PR with that change, but it looks less nice, and I'm assuming this is a regression (i.e. for on a Set worked at some point). A proper fix would be making for comprehensions on sets work.

If you point me to the right file(s) I could check if can fix this myself.

This is the error message I get:

-------------------------------------------------------------------------------
Type Error:

No built-in field '!getHead' for type 'PtlsSet'
-------------------------------------------------------------------------------

At (line 216 column 19) in '/Users/till/code/ptls/pointless/bin/../prelude/list.ptls'
  else [func(head(list))] ++ map(func, tail(list))
                  ^
At (line 5 column 14) in '/Users/till/code/ptls/pointless/bin/../prelude/list.ptls'
head(list) = list.!getHead
             ^
At (line 38 column 13) in '/Users/till/code/ptls/pointless/bin/../prelude/list.ptls'
  else head(lists) ++ concat(tail(lists))
            ^
At (line 37 column 17) in '/Users/till/code/ptls/pointless/bin/../prelude/set.ptls'
    for elem in a
                ^
At (line 43 column 26) in '/Users/till/code/ptls/pointless/bin/../prelude/list.ptls'
concatMap(func, lists) = lists |> map(func) |> concat
                         ^
At (line 1 column 19) in '<repl>'
difference(digits,digits)
roetlich commented 4 years ago

I tried solving this with #12, but I don't know if that's the right solution. @averynortonsmith do you have feedback?

averynortonsmith commented 4 years ago

Good catch! I'm not sure how intersection and difference ended up broken, but you're right that adding a call to toList in the loop should solve it. Thanks for the clear issue write-up!

You bring up the possibility of expanding map (and possibly other functions like filter, reduce, etc) to work with sets and other structures. This is something I've gone back-and-forth on: on the one hand, reducing the number of types that a function like map accepts helps give the language a more "strongly-typed" feel and makes code more explicit. On the other hand it may be more convenient to have map work on more types without a conversion.

In the end I think the answer lies in the principle of least astonishment. If users of Pointless expect structures like sets to be map-able, then they probably should be. Here's another question though: should mapping a function over a set return a list (as your new implementation in #12 does), or should it first convert the list back to a set? I'd like to pull the changes you've made, but I'd be curious to see what you think about this question first.

roetlich commented 4 years ago

Well, in most functional languages I used, there is module system. That makes it easy to differentiate List.map and Set.map etc. But such an verbose style maybe doesn't fit pointless. You know, to many "points". :)

I agree, I always imagine map to return the collection type that I put in. But in this case, the for comprehension is clearly meant to always return a list. Maybe the solution is to make a new function, mapToList or something like that, that can be used by for?