For-Comprehension #879

ElieOaks · 2019-05-08T07:57:41Z

For - Comprehension
A new kind of for-comprehension is being implemented into Encore that replaces the old for-loop.
For-comprehension can:
-- act as a function call :

var x = for y <- someList do
              y
            end

-- iterate over itterable objects, not just Arrays and Ranges:

for x <- linkedList do
        --body
end

-- iterate over multiple data structures in one call:

for x <- [1, 2, 3], y <- ["a", "b", "c"] do
        -- body
end

-- be passed as an argument

foo( for y <- someList do
            y
        end)

The new for-comprehension does not allow:
-- Different kinds of data structures in header

for x <- linkedList, y <- ["a", "b", "c"] do
        -- body
end

-- Range to return a collection. It will compile, but x will be of type unit. Therefor ranges only desugar into foreach.

var x = for y <- [0 .. 10] do
              y
            end

-- A return in the body of the loop to return something outside of it. The bellow example will not return 1.

fun foo() : int
  for x <- [1, 2, 3]
    return x
  end
end

Rather it is equivalent to:

fun foo() : int
  var returnCollection  for x <- [1, 2, 3]
                                        return x
                                   end
 return returnCollection
end

Which of course would break the typing rules, as it returns an array, and not an int.
Under the hood:
The for-comprehension is pure syntactic sugar, and is desugared into combination of calls to the methods map, flatMap, foreach, and if the loop contains break, into maybeForeach. These methods can be found in some of the collections in the standard libraries, such as Array and LinkedList.
the methods map and flatMap are used in combination, when a return collection is expected. Otherwise the method foreach is used, as it's loop has only side effects.

var x = for y <- someList, z <- someListTwo do
              y + z
            end

is desugaraed into:
var x = someListTwo.flatMap(someList.map(y))

for y <- [0 .. 10] do
         print(y)
end

is desugaraed into:
[0 .. 10].foreach(print(y))

for y <- [0 .. 10] do
         print(y)
         if y > 9 then
            break
         end
end

is desugaraed into:

 [0 .. 10].maybeForeach(
  print(y)
  if y > 9 then
     return Nothing
  end
  Just(())
  )

Added compilation steps
The desugaring of for-comprehension depends upon typing information, and is therefore a new step of compilation called TypedDesugarer, and occurs before the optimizations. As the desusaring process breaks down the For-Expr and builds up a sub-AST of new Expr, the AST needs to be re-type checked, and re-capture checked. Therefore the compilation includes Typed Desugaring, as well as Re - Type checking, and Re - Capture checking. When modular type checking, and capure checking becomes available, these compilation steps can hopefully be removed.

Problem with re - type checking
Type checking twice is not possible due to the type checking process of Match clauses. When Match is type checked, it changes the Match-clause so that it violates its own typing rules and cannot be type checked again without throwing an Error. A hack has been created so that programs with for-comprehension can compile and run, but is currently incompatible with the match - type checker. This fork can be merges only when the match type checking problem has been solved.

Arrays
Unlike classes such LinkedList in the standard Library, there is a list of functions, rather than methods that can be applied to arrays. Therefore for-comprehensions that are over arrays are desugared into function calls map, flatMap, foreach, and if the loop contains break, into maybeForeach, rather than into method calls by the same name.

Other changes
This PR has also had to incorporate the PR: Ability to compile pony runtime with gcc 7.x #865. As the latest ubuntu, which this was developed on, runs on gcc 7.
It also desugars ranges into a class RRange, that can be found in the module-file Data. This was in order to have the ability to add methods. As range is a protected word in the compiler, the class has to be named something else, hence the double R:s. As ranges are only used together with for-loops / for-comprehensions, this change is not expected to affect anything else.

…d another typechecking pass, optimizer to desugar the for-loop into method calls

…uded, commented out typechecker 2, and capturechecker 2

kikofernandez

Overall, it looks really good. There are only minor comments on code style. The haskell code seems good.

kikofernandez · 2019-05-09T12:55:18Z

modules/standard/Collections/Mutable/Functor.enc

@@ -0,0 +1,10 @@
+-- JOY for-comprehension


It is nice to have the name, but I would remove this line :)

kikofernandez · 2019-05-09T13:00:33Z

src/front/TopLevel.hs

@@ -341,6 +341,14 @@ main =
       verbose options "== Optimizing =="
       let optimizedTable = fmap optimizeProgram capturecheckedTable

+       -- JOY for-comprehension


be careful with these ones.

kikofernandez · 2019-05-09T13:01:23Z

src/ir/AST/AST.hs

@@ -684,11 +691,10 @@ data Expr = Skip {emeta :: Meta Expr}
                    name :: Name,
                    times :: Expr,
                    body :: Expr}
+          -- JOY for-comprehension


no need to annotate your stuff :)

kikofernandez · 2019-05-09T13:03:01Z

src/ir/AST/AST.hs

@@ -600,6 +600,13 @@ data VarDecl =
             varType :: Type}
  deriving(Eq, Show)

+-- JOY for-comprehension
+data ForSource =
+     ForSource { forVar :: Name,


following Encore compiler convention, this should be name, and the Maybe Type should be ty

kikofernandez · 2019-05-09T13:04:01Z

src/ir/AST/Desugarer.hs

@@ -389,7 +389,8 @@ desugar FunctionCall{emeta, qname = QName{qnlocal = Name "exit"}
    Exit emeta args

 -- Abort
-desugar FunctionCall{emeta, qname=QName{qnlocal=Name "abort"} , args=[msg]} =
+desugar FunctionCall{emeta, qname = QName{qnlocal = Name "abort"}


kikofernandez · 2019-05-09T13:32:51Z

src/types/Typechecker/Typechecker.hs

+            collectionTyped <- doTypecheck collection
+            let collectionType = AST.getType collectionTyped
+            let forVarType = return $ getInnerType collectionType
+            return fors{forVarType = forVarType,


for consistency, the Haskell style does:

fors{... = ... ,... = ...}

kikofernandez · 2019-05-09T13:34:01Z

src/types/Typechecker/Typechecker.hs

+      where
+        typeCheckSources :: [ForSource] -> TypecheckM [ForSource]
+        typeCheckSources sourceList =  do
+          typedSources <- mapM typeCheckSource sourceList


if the return type is the result of the mapM, you move the line mapM typCheckSource sourceList to the line above, removing the do and the return typeSources

If you do the comment above, maybe line 1729 is really a mapM typeCheckSource sourceList, and you have to maintain one less function

kikofernandez · 2019-05-09T13:36:16Z

src/types/Typechecker/Typechecker.hs

+                        collection = setType collectionType collectionTyped}
+
+        getForVarTypeList sourceList = mapM getForVarType sourceList
+        getForVarType ForSource{forVar, collection} = do


I would add the typing to this function, I think it can serve as documentation giving that returning a tuple is not the most common thing for the compiler

kikofernandez · 2019-05-09T13:37:22Z

src/types/Typechecker/Typechecker.hs

+         | isRangeType collectionType = intType
+         | otherwise = head $ getTypeParameters collectionType
+
+        addIteratorVariable forVarList = extendEnvironmentImmutable forVarList


this line says that addIteratorVariable = extendEnvironmentImmutable. Hence, I would remove this equivalence since it is just a renaming and use the extendXXXX

kikofernandez · 2019-05-09T13:39:32Z

src/types/Typechecker/Typechecker.hs

+          let forVarType = getInnerType collectionType
+          return (forVar, forVarType)
+
+        getInnerType collectionType


you should handle errors. That is, what if this function receives an IntLiteral. The compiler would crash.
One way to fix this is to continue with the guards and make sure that the compiler does not crash and throws an error. This will help future compiler writers to know that an IntLiteral was not expected. There may be other types that are not expected, not only the IntLiteral

…piler

kikofernandez

Overall, the changes look mostly good. There is quite some noise by files that should not be there, binaries that have been added, etc. In general, the comments should be pretty straightforward to address. If you have questions, continue in the local thread :)

Thanks for your work!

kikofernandez · 2019-06-20T11:34:07Z

critik

@@ -0,0 +1,9 @@
+The first slide after introduction: Have a picture?


wrong file? This file is not needed here.

kikofernandez · 2019-06-20T11:35:01Z

forComp/output.enc

@@ -0,0 +1,15 @@
+typeCheckSource sourceType fors@(ForSource{fsTy, collection}) = do


this is Haskell but uses the Encore file extension.
I am not sure what this is :)

kikofernandez · 2019-06-20T11:36:38Z

forComp/tests/preliminaryFailSyntaxTests.enc

@@ -0,0 +1,45 @@
+--These are preliminary for-comprehension tests


Encore does not yet return multiple errors.
Could you instead move each of these tests to their own files (and create their own .fail files as well)?

kikofernandez · 2019-06-20T11:38:20Z

forComp/tryEquals.enc

@@ -0,0 +1,36 @@
+active class Equal


I am not sure on what is this file testing.
Thanks

kikofernandez · 2019-06-20T11:39:14Z

forComp/tryEquals.enc

+    end
+
+  end
+end


There is a forComp/tryFor binary. Binaries should not be included in git :)

kikofernandez · 2019-06-20T12:18:31Z

src/tests/encore/optaccess/optAccess_src/Main.encore.c

@@ -0,0 +1,191 @@
+#include "header.h"


this should not be here

kikofernandez · 2019-06-20T12:18:44Z

src/tests/encore/par/extract.enc

@@ -282,6 +282,7 @@ active class Test
      end
      for v <- arr_passive_join do
        if v.tag.eq("pass") then
+          print("{}\n", v)


kikofernandez · 2019-06-20T12:19:04Z

src/tests/encore/stdlib/Data/BoxedTest.enc

+import Boxed.Char
+import Boxed.Real
+import Boxed.Bool
+import Boxed.Unit

 active class Main
  def main() : unit


the binary Count should not be here

kikofernandez · 2019-06-20T12:19:55Z

src/types/Typechecker/Typechecker.hs

@@ -1857,7 +1876,7 @@ instance Checkable Expr where
                  (length expectedTypes) (length args)
      eArgs <- mapM typecheck args
      matchArguments args expectedTypes
-      return $ setType bottomType abort{args=([]::[Expr])}
+      return $ setType bottomType abort{args = eArgs} --args=([]::[Expr])} TODO: is this allowed?


is the comment to address or to remove?

kikofernandez · 2019-06-20T12:20:08Z

src/types/Typechecker/Typechecker.hs

@@ -2237,7 +2256,7 @@ matchTypes expected ty
        bindings <- matchArgs (getTypeParameters expected) (getTypeParameters ty)
          `catchError` (\case
                         TCError (TypeMismatchError _ _) _ ->
-                             tcError $ TypeMismatchError ty expected
+                             tcError $ TypeMismatchError $ trace "2259" $ ty expected


remove the trace when you are not debugging anymore

ElieOaks added 9 commits April 3, 2019 16:05

file to try out Encore, and preliminary tests for for-comprehension

2895d7c

Changes to parser to handle the new for-comprehension, toplevel to ad…

957edcc

…d another typechecking pass, optimizer to desugar the for-loop into method calls

added funcion to array, further dev. of optimizer, mutable boxes incl…

ce50299

…uded, commented out typechecker 2, and capturechecker 2

removed scrap files beofre review PR

97e2549

removed translation of for

61c15fa

changed the names om mutable boxed file names

979bcb6

removed the old named files from git

8b61939

added requirement for functor

067051d

removed old commented out for-parsing

4cb0e56

kikofernandez requested changes May 9, 2019

View reviewed changes

ElieOaks added 4 commits May 16, 2019 10:58

finally got to the boxing problem

17a835d

added another file: TypedDesugaring, which is another pass of the com…

8a365c9

…piler

lots of stuff

c5204b8

for-comprehension works

0c100a5

kikofernandez requested changes Jun 20, 2019

View reviewed changes

ElieOaks added 3 commits June 22, 2019 16:35

break works

e2b1f0b

writing test cases

af9141b

begnning the clean up of code

17f1b6b

ElieOaks changed the title ~~For half-way review~~ For-Comprehension Jun 28, 2019

ElieOaks added 10 commits June 28, 2019 14:59

cleaned up code and worked off of Kiko's comments

499d158

removed stray file

ddce7ab

removed stray file

8a3c951

fixed a space

5dbc4a9

fixed a space

ba10b39

fixed a space

308149e

fixed a space

740946b

fixed a space

2c13220

fixed a space

9edd6c4

smaller changes

b475d04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For-Comprehension #879

For-Comprehension #879

ElieOaks commented May 8, 2019 •

edited

kikofernandez left a comment

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez May 9, 2019

kikofernandez left a comment

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

kikofernandez Jun 20, 2019

		@@ -0,0 +1,9 @@
		The first slide after introduction: Have a picture?

		@@ -0,0 +1,15 @@
		typeCheckSource sourceType fors@(ForSource{fsTy, collection}) = do

		@@ -0,0 +1,45 @@
		--These are preliminary for-comprehension tests

+                  end
+                end
+              end

For-Comprehension #879

Are you sure you want to change the base?

For-Comprehension #879

Conversation

ElieOaks commented May 8, 2019 • edited

kikofernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikofernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElieOaks commented May 8, 2019 •

edited