Opened 10 years ago

Last modified 11 months ago

#1131 new defect

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

Reported by: sjamaan Owned by:
Priority: major Milestone: someday
Component: unknown Version: 4.9.x
Keywords: Cc:
Estimated difficulty: insane

Description

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

Attachments (1)

0001-Resolve-variable-in-set-using-lookup-when-variable-i.patch (2.6 KB) - added by sjamaan 4 years ago.
Initial fix for compiler (interpreter still broken)

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 years ago by sjamaan

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.

(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.

comment:2 Changed 10 years ago by sjamaan

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).

comment:3 Changed 10 years ago by sjamaan

Another inconsistency:

(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.

comment:4 Changed 10 years ago by sjamaan

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

comment:5 Changed 9 years ago by sjamaan

Milestone: 4.10.05.0

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.

comment:6 Changed 8 years ago by sjamaan

Estimated difficulty: insane

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.

comment:7 Changed 8 years ago by sjamaan

Milestone: 5.05.1

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

comment:8 Changed 7 years ago by sjamaan

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))

comment:9 Changed 6 years ago by sjamaan

Milestone: 5.15.2

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

comment:10 Changed 5 years ago by felix winkelmann

Milestone: 5.25.3
Priority: criticalmajor

comment:11 Changed 4 years ago by sjamaan

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.

Changed 4 years ago by sjamaan

Initial fix for compiler (interpreter still broken)

comment:12 Changed 4 years ago by sjamaan

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...

comment:13 Changed 4 years ago by sjamaan

Milestone: 5.35.4

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

comment:14 Changed 3 years ago by sjamaan

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...

comment:15 Changed 3 years ago by sjamaan

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.

comment:16 Changed 11 months ago by felix winkelmann

Milestone: 5.4someday
Note: See TracTickets for help on using tickets.