Opened 6 years ago

Closed 6 years ago

#1437 closed defect (fixed)

csi needs double imports

Reported by: Mario Domenech Goulart Owned by:
Priority: major Milestone: 5.0
Component: expander Version: 5.0.0
Keywords: double import Cc:
Estimated difficulty:

Description (last modified by Mario Domenech Goulart)

Here's a simple test case:

$ cat assoc.scm                                                                                                    
(import srfi-1)
(define e2 '((2 3) (5 7) (11 13)))
(assoc 5 e2 <)

$ ~/local/chicken-5-head/bin/csi -s assoc.scm 

Error: bad argument count - received 3 but expected 2: #<procedure (scheme#assoc x231 lst232)>

        Call history:

        <syntax>          (##core#begin (##core#require expand chicken.syntax#))
        <syntax>          (##core#require expand chicken.syntax#)
        <syntax>          (##sys#load-library (##core#quote expand))
        <syntax>          (##core#quote expand)
        <eval>    (##sys#load-library (##core#quote library))
        <eval>    (##sys#load-library (##core#quote library))
        <eval>    (##sys#load-library (##core#quote expand))
        <syntax>          (define e2 (quote ((2 3) (5 7) (11 13))))
        <syntax>          (##core#begin (##core#ensure-toplevel-definition e2) (##core#set! e2 (quote ((2 3) (5 7) (11 13)))))
        <syntax>          (##core#ensure-toplevel-definition e2)
        <syntax>          (##core#undefined)
        <syntax>          (##core#set! e2 (quote ((2 3) (5 7) (11 13))))
        <syntax>          (quote ((2 3) (5 7) (11 13)))
        <syntax>          (##core#quote ((2 3) (5 7) (11 13)))
        <syntax>          (assoc 5 e2 <)
        <eval>    (assoc 5 e2 <)        <--

The compiled version works ok:

$ ~/local/chicken-5-head/bin/csc assoc.scm 
In file included from assoc.c:8:
/home/mario/local/chicken-5-head/include/chicken/chicken.h:2411:37: warning: expression result unused [-Wunused-value]
    C_CHECKp(x,C_bignump(C_VAL1(x)),0);
                                    ^
/home/mario/local/chicken-5-head/include/chicken/chicken.h:858:37: note: expanded from macro 'C_CHECKp'
# define C_CHECKp(v,a,x)           (x)
                                    ^
1 warning generated.

$ ./assoc

$ echo $?
0

If I import srfi-1 twice, it works in the interpreter:

$ cat assoc.scm 
(import srfi-1)
(import srfi-1)
(define e2 '((2 3) (5 7) (11 13)))
(assoc 5 e2 <)

$ ~/local/chicken-5-head/bin/csi -s assoc.scm

$ echo $?
0

I suspect the same problem is affecting srfi-71: https://salmonella-linux-x86-64.call-cc.org/master/gcc/linux/x86-64/2018/01/02/salmonella-report/test/srfi-71.html

I'm using

$ ~/local/chicken-5-head/bin/csi -version
(c) 2008-2017, The CHICKEN Team
(c) 2000-2007, Felix L. Winkelmann
Version 5.0.0 (rev baf6363)
linux-unix-clang-x86-64 [ 64bit dload ptables ]

I cannot reproduce this problem with 5.0.0pre4.

Attachments (1)

toplevel-evals.diff (2.9 KB) - added by sjamaan 6 years ago.
Attempt that fails

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Mario Domenech Goulart

Description: modified (diff)
Summary: csi needs double imports shadowed procedurescsi needs double imports

comment:2 Changed 6 years ago by felix winkelmann

Component: unknownexpander
Milestone: someday5.0

comment:3 Changed 6 years ago by sjamaan

It looks like this is happening because (import srfi-1) causes the import library to be loaded and imported into the environment, but then also causes srfi-1.so to be loaded, which contains (eval '(scheme chicken.base chicken.syntax)) since 24325e3a6e1e829e9e9be8d22e28d1711a796bd2.

This eval clobbers the bindings we just imported with those from scheme. Because calling (import srfi-1) again at the REPL won't cause the srfi-1 library's toplevel to be evaluated again, the bindings are clobbered yet again, but this time with those from srfi-1, causing us to actually see the intended bindings.

You can see it clearly in a DEBUGBUILD:

#;1> (import srfi-1)
; loading /home/sjamaan/chickens/chicken-5-dbg/lib/chicken/9/srfi-1.import.so ...
; loading /home/sjamaan/chickens/chicken-5-dbg/lib/chicken/9/chicken.import.so ...

Note: re-importing already imported identifier: assoc  [this is the srfi-1 binding we asked for, srfi-1#assoc]

Note: re-importing already imported identifier: member [same]
; loading /home/sjamaan/chickens/chicken-5-dbg/lib/chicken/9/srfi-1.so ...

Note: re-importing already imported identifier: member [srfi-1 toplevel executing, importing scheme#assoc as assoc]

Note: re-importing already imported identifier: assoc [same]
#;2> (import srfi-1)

Note: re-importing already imported identifier: assoc [and now we ask for srfi-1#assoc again, and we get it because no new code is run]

Note: re-importing already imported identifier: member

I'm not sure about the best fix for this. We could change the ordering of what import does, so that the import of the requested library is done after all the dependency loading, but that's not a complete fix. Perhaps only proper programs (not libraries) should get these eval expressions injected at the toplevel?

comment:4 in reply to:  3 Changed 6 years ago by sjamaan

Replying to sjamaan:

We could change the ordering of what import does, so that the import of the requested library is done after all the dependency loading, but that's not a complete fix.

Nah, it's not a fix at all. If you do something like:

(import srfi-1)
(import some-other-library)

the toplevel of some-other-library will cause a re-import of scheme at the toplevel, clobbering the srfi-1 imports.

Changed 6 years ago by sjamaan

Attachment: toplevel-evals.diff added

Attempt that fails

comment:5 Changed 6 years ago by sjamaan

The above attachment should fix the situation, but for some reason this causes make check to fail on the test-finalizers.scm interpreted test.

comment:6 in reply to:  5 Changed 6 years ago by felix winkelmann

Replying to sjamaan:

The above attachment should fix the situation, but for some reason this causes make check to fail on the test-finalizers.scm interpreted test.

I don't think this is related. The test works sometimes, depending on cmdline-arguments or how it is run. It's a different issue that causes finalizers not to run in certain situations.

comment:7 Changed 6 years ago by felix winkelmann

Note that this problem still exists in the recent change for allowing import of base modules in static executables (commit 81e400fc2516b64d2c768fcd5e97772ecc8e5327). I assume this is caused by a wrong initialization order of imports. To reproduce this problem, try this:

;; compile: bin/csc -static x.scm && ./x -:rd
(import srfi-1)
(print (assoc 'a '((a . 1)) eq?))
(print (eval '(begin (import srfi-1) (assoc 'a '((a . 1)) eq?))))

The first call to assoc succeeds, the second fails with an error message.

comment:8 Changed 6 years ago by sjamaan

I just noticed if you split up into two evals, it works correctly!

(import srfi-1)
(print (assoc 'a '((a . 1)) eq?))
(print (eval '(import srfi-1)))
(print (eval '(assoc 'a '((a . 1)) eq?)))

This probably is something going wrong in the evaluator's lookup or in how begin is handled, not the imports as such.

comment:9 Changed 6 years ago by sjamaan

Looks like this will also fail when using the compiler:

(begin
  (import srfi-1)
  (print (assoc 'a '((a . 1)) eq?)))

When compiling this:

Warning: at toplevel:
  (test.scm:3) in procedure call to `scheme#assoc', expected 2 arguments but was given 3 arguments

And when run, it will fail too.

The reason is that in the compiler, we walk the toplevel using map, which calls canonicalize-expression on each toplevel expression. This in turn accesses (##sys#current-environment) once at the start and then passes this on as the syntax environment (se), which gets passed along. When expanding an import, we update ##sys#current-environment, but this won't affect se because that's another list (at best, it's the tail of the new ##sys#current-environment).

In the evaluator, the same happens. So you'd have to map eval over the "toplevel" expressions, instead.

comment:10 Changed 6 years ago by sjamaan

Resolution: fixed
Status: newclosed

Fixed by bf374b022bc40c3cc24d4e7e7267ea300f8be987

Note: See TracTickets for help on using tickets.