stylewarning / deprecated-coalton-prototype

Coalton is (supposed to be) a dialect of ML embedded in Common Lisp.
MIT License
208 stars 7 forks source link

Failed to parse (TRUE) because: The constructor TRUE is unknown. #11

Open appleby opened 5 years ago

appleby commented 5 years ago

I'm following along with the README and noticed a weird interaction between coalton and ASDF, whereby coalton gets confused if I try to reload it after restarting my inferior lisp (sbcl). For example:

First run, A-OK

On the first compile/load, everything is fine:

CL-USER> (ql:quickload :coalton)
To load "coalton":
  Load 1 ASDF system:
    coalton
; Loading "coalton"
..................................................
[package coalton].................................
[package coalton-user]............................
[package coalton-impl]............................
[package coalton-global-symbols]..................
......
(:COALTON)
CL-USER> (in-package :coalton-user)
#<COMMON-LISP:PACKAGE "COALTON-USER">
COALTON-USER> (coalton-toplevel
                (define (gg x) (if x (left 1) (right x))))
COMMON-LISP:NIL

Second run

But if I then slime-restart-inferior-lisp and reload, coalton gets confused.

CL-USER> (ql:quickload :coalton)
To load "coalton":
  Load 1 ASDF system:
    coalton
; Loading "coalton"
.
(:COALTON)
CL-USER> (in-package :coalton-user)
#<COMMON-LISP:PACKAGE "COALTON-USER">
COALTON-USER> (coalton-toplevel
                (define (gg x) (if x (left 1) (right x))))
; Evaluation aborted on #<COALTON-IMPL:COALTON-PARSE-ERROR {10040AF5A3}>.

The exact error is

Failed to parse (TRUE) because: The constructor TRUE is unknown.
   [Condition of type COALTON-IMPL:COALTON-PARSE-ERROR]

Restarts:
 0: [RETRY] Retry SLIME REPL evaluation request.
 1: [*ABORT] Return to SLIME's top level.
 2: [ABORT] abort thread (#<THREAD "repl-thread" RUNNING {10023F9B63}>)

Backtrace:
  0: (COALTON-IMPL::ERROR-PARSING (TRUE) "The constructor ~S is unknown." TRUE)
  1: ((COMMON-LISP:LABELS COALTON-IMPL::PARSE :IN COALTON-IMPL::PARSE-FORM) (MATCH X (TRUE (LEFT 1)) (FALSE (RIGHT X))))
  2: ((COMMON-LISP:LABELS COALTON-IMPL::PARSE :IN COALTON-IMPL::PARSE-FORM) (FN (X) (IF X (LEFT 1) (RIGHT X))))
  3: (COALTON-IMPL::PARSE-DEFINE-FORM-FUNCTION GG (X) (IF X (LEFT 1) (RIGHT X)))
  4: (COALTON-IMPL::PROCESS-TOPLEVEL-VALUE-DEFINITIONS ((DEFINE (GG X) (IF X # #))))
  5: ((COMMON-LISP:MACRO-FUNCTION COALTON-TOPLEVEL) (COALTON-TOPLEVEL (DEFINE (GG X) (IF X # #))) #<unused argument>)
  6: ((COMMON-LISP:FLET SB-IMPL::PERFORM-EXPANSION :IN COMMON-LISP:MACROEXPAND-1) #<FUNCTION (COMMON-LISP:MACRO-FUNCTION COALTON-TOPLEVEL) {52E143BB}> COMMON-LISP:NIL)
  7: (COMMON-LISP:MACROEXPAND (COALTON-TOPLEVEL (DEFINE (GG X) (IF X # #))) #<NULL-LEXENV>)
  8: (SB-INT:SIMPLE-EVAL-IN-LEXENV (COALTON-TOPLEVEL (DEFINE (GG X) (IF X # #))) #<NULL-LEXENV>)
  9: (COMMON-LISP:EVAL (COALTON-TOPLEVEL (DEFINE (GG X) (IF X # #))))

Workarounds

Any of the following workarounds can be used immediately after restarting sbcl.

  1. Manually LOAD the file library.lisp after loading the system: (ql:quickload :coalton) (load (asdf:system-relative-pathname :coalton "src/library.lisp"))
  2. Use (asdf:load-system :coalton :force t) instead of (ql:quickload :coalton)
  3. Remove cached FASL files and recompile
    1. In shell: $ rm ~/.cache/common-lisp/sbcl-1.4.16-linux-x64/home/ma/src/repos/coalton/src/*.fasl
    2. In REPL: (ql:quickload :coalton)
appleby commented 5 years ago

I think maybe the side-effecty stuff at the top of compile-toplevel-define-type-form that appears to be registering the tycon and it's constructors needs to be pushed down into the macro expansion, possibly within an eval-when?

Proof-of-concept PR coming soon (hopefully)...

appleby commented 5 years ago

Thinking about this a bit more, perhaps things are not so simple as I had hoped (they never are!).

There seem to be at least three competing concerns here:

  1. Definitions inside a single coalton-toplevel form need to be visible to each other at macro-expansion time.
  2. Definitions should also be visible at :load-toplevel time, so that loading a precompiled FASL does the right thing.
  3. Definitions should be re-definable, ideally without gratuitous warnings in the common case of compiling, then loading a file.

The corollary to (3) is that the definitions should only be registered in the global environment once, probably inside the macro form and possibly wrapped in an (eval-when (:compile-toplevel :load-toplevel :execute) ...). (I suspect the :compile-toplevel is not needed which means the eval-when is not needed, but, as usual, staring at the hyperspec section for Processing Toplevel Forms has left me less than certain...).

But pushing the tycon and ctor registrations down into the macro form means that the macro expander needs some other provision to know about in-process definitions. The current implementation already tracks an extra-tycons parameter for this purpose, and parse-type-expression is smart enough to lookup stuff in extra-tycons before trying the "global environment." Perhaps similar provisions could be made for ctors and the like. So far I have been ignoring other toplevel definition forms like define and declare, which may need similar treatment (haven't looked yet).

Maybe instead of just extra-tycons any of the process-toplevel-* functions (and whatever functions they call) could get passed some kind of environment object that tracks all type/ctor/value definitions, similar to the &environment defmacro parameter.

tl;dr - Not quite as simple as I had hoped. I should probably read and understand the coalton source (and maybe the MACROEXPAND-ALL chapter of this Dick Waters paper) before proceeding further :). I have other stuff I want to work on first, but will circle back to this eventually (famous last words).

stylewarning commented 5 years ago

This is a good thing to pick up on. I've felt uneasy about the compile-time side effects Coalton does. There needs to be a better way to manage it all.

stylewarning commented 4 years ago

Probably what needs to happen is side effects need to happen at macroexpansion time, but they also need to be put in the macroexpansion as well so the FASL loader catches them.

It seems to be "working" when I do that, e.g.

diff --git a/src/toplevel-define-type.lisp b/src/toplevel-define-type.lisp
index b016280..9e56c55 100644
--- a/src/toplevel-define-type.lisp
+++ b/src/toplevel-define-type.lisp
@@ -101,6 +101,14 @@ where
     ;; TODO: Structs? Vectors? Classes? This should be thought
     ;; about. Let's start with classes.
     `(progn
+       ;; Make sure database side effects are maintained.
+       (eval-when (:compile-toplevel :load-toplevel :execute)
+         (setf (find-tycon ',tycon-name) ,tycon)
+         ,@(loop :for (_ name ty) :in ctors
+                 :collect `(unless (var-knownp ',name)
+                             (forward-declare-variable ',name))
+                 :collect `(setf (var-declared-type ',name) ,ty)))
+
        ;; Define types. Create the superclass.
        ;;
        ;; TODO: handle special case of 1 ctor.
stylewarning commented 4 years ago

Note: PR in progress https://github.com/stylewarning/coalton/pull/14

appleby commented 4 years ago

The diff in #14 looks similar to what I did in #12. I'm struggling to remember now why I abandoned that PR. Can't remember if I stumbled on a bug or if I was just vaguely unsatisfied with it.