Summary

Kill ##sys#alias-global-hook with fire

Metadata

Attachments

Description

The whole concept is broken. Just by studying the code it's easy to come up with bug after bug.

Changes and comments

[2014-06-09 21:04:36 UTC] sjamaan wrote:

Here's the first:

The following code will compile (or run, in csi). If the module definition for another is removed, it doesn't. Why? Because when another imports foo, ##sys#mark-imported-symbols will mark it as ##core#aliased, which means ##sys#alias-global-hook will simply pass it through whenever it's subsequently referenced.

 #!scm
 (module foo (bar)
   (import scheme chicken)
   (define (bar x) (+ x 1)))
 
 (module another ()
   (import chicken scheme foo)
   (define lala bar))
 
 (module whoops (lala2)
   (import chicken scheme)
   (define lala2 foo#bar))
 
 (import whoops)
 (print (lala2 1))

The question is whether a fully qualified module identifier should work within another module regardless of whether it's been imported. This part should be the first to clean up, because ##core#aliased is only used in a handful of places. Unfortunately it's a bit of a mindfuck, because it is really used all over the place, **through** ##sys#global-alias-hook.

[2014-06-09 21:06:59 UTC] sjamaan wrote:

Ideally, we'd also remove ##sys#mark-imported-symbols while we're at it: it's totally unneccessary, because whether a symbol is imported is an aspect of a particular module, not a global status (if we ignore the toplevel).

[2014-06-15 18:32:49 UTC] sjamaan wrote:

Another inconsistency:

 #!scm
 (module bar (x)
   (import scheme)
   (define x 1))
 
 (module foo ()
   (import chicken)
   (print (#%+ bar#x 1)))

The above program fails with Warning: reference to possibly unbound identifier `bar#x'. If we add bar to the import list (or remove the reference) it will start working, even though we didn't import the primitive module which includes a binding for + (ie, scheme).

This is due to the (sometimes?) different treatment of ##core#aliased and ##core#primitive.

[2014-06-15 20:51:22 UTC] sjamaan wrote:

Looks like the first order of business should be to try and eliminate the special handling of primitives.

[2015-08-27 08:43:38 UTC] sjamaan changed milestone from 4.10.0 to 5.0

[2015-08-27 08:43:38 UTC] sjamaan wrote:

We're not going to fix this in the CHICKEN 4 branch, but hopefully in the 5 cycle we'll be able to eventually fix it. I'll assign 5.0 as a milestone even though I'm doubtful we'll make it as soon as that.

[2016-08-25 21:32:45 UTC] sjamaan set difficulty to insane

[2016-08-25 21:32:45 UTC] sjamaan wrote:

This is the kind of thing that has to die a death by a thousand cuts. That will make it seem "easy", but it will take a long time before we have everything covered.

[2017-04-07 20:14:40 UTC] sjamaan changed milestone from 5.0 to 5.1

[2017-04-07 20:14:40 UTC] sjamaan wrote:

Still needs to die, but we won't manage to do that before 5.0

[2018-02-18 12:39:06 UTC] sjamaan wrote:

Another catch, from #1441:

 (module undef (undefined)
   (import scheme)
   (define undefined 1))
 
 (module expando (do-it)
   (import scheme)
   (define-syntax do-it
     (syntax-rules ()
       ((_)
        (let-syntax ((final
                      (syntax-rules ()
                        ((_ ?x) ?x))))
          (final undefined))))))
 
 (import undef expando)
 ;; Should fail, not print 1, because the let-syntax expansion should rename introduced 
 ;; identifiers from its SE (but input should be looked up here)
 (print (do-it))

[2019-04-07 12:23:24 UTC] sjamaan changed milestone from 5.1 to 5.2

[2019-04-07 12:23:24 UTC] sjamaan wrote:

Getting ready for 5.1, moving tickets which won't make it in to 5.2.

[2019-08-25 17:33:53 UTC] felix changed priority from critical to major

[2019-08-25 17:33:53 UTC] felix changed milestone from 5.2 to 5.3

[2021-04-11 12:48:14 UTC] sjamaan wrote:

Note to self: It looks like there is some inconsistency between `set!` which doesn't resolve variables using `lookup` and variable dereference which does.

Also, the interpreter seems to have a problem regarding duplicate lookup in `##sys#current-environment`.

Once these are fixed, we can drop the `(assq sym (##sys#current-environment))` from `##sys#alias-global-hook`.

In other words, there is an inconsistent number of times in which variables are looked up in `##sys#current-environment` which means sometimes it will resolve a certain variable **too often**, resulting in mismatches. One reason is that `se` in the compiler generally already refers to `##sys#current-environment`, and in the interpreter the current environment is also resolved.

[2021-04-11 13:02:25 UTC] sjamaan attached 0001-Resolve-variable-in-set-using-lookup-when-variable-i.patch (description=Initial fix for compiler (interpreter still broken))

[2021-04-11 13:03:45 UTC] sjamaan wrote:

Attachment above includes a test and removing the call to `##sys#current-environment` as a proof of concept. The interpreted module-tests still break because there's still some confusion regarding `##sys#current-environment` in there...

[2021-04-12 13:22:54 UTC] sjamaan changed milestone from 5.3 to 5.4

[2021-04-12 13:22:54 UTC] sjamaan wrote:

More work has been done to whittle it down. Maybe we can get it done in 5.4 (gotta stay optimistic :P)

[2022-03-10 08:47:53 UTC] sjamaan wrote:

That patch is actually incorrect - it actually *introduces* a duplicate lookup in the compiler to correct for the removed lookup in `##sys#alias-global-hook`. The interpreter does *not* have a duplicate lookup.

It's a bit confusing that the names in the compiler and interpreter differ; `lookup` in the compiler is called `rename` in the interpreter. And the path that leads to the `##sys#alias-global-hook` call is put at the start in the compiler and somewhere nested down in the interpreter...

[2022-03-10 10:49:39 UTC] sjamaan wrote:

I'm not so sure anymore that this is really a bug, and if it is, I'm not sure it's caused by `##sys#alias-global-hook` per se (more that we don't keep info about which module an identifier originated from, which would be tough to do)

But I think the code can be refactored to be more obviously correct.

[2023-11-06 21:53:43 UTC] felix changed milestone from 5.4 to someday