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)
Change History (17)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
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
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
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
Milestone: | 4.10.0 → 5.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
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
Milestone: | 5.0 → 5.1 |
---|
Still needs to die, but we won't manage to do that before 5.0
comment:8 Changed 7 years ago by
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
Milestone: | 5.1 → 5.2 |
---|
Getting ready for 5.1, moving tickets which won't make it in to 5.2.
comment:10 Changed 5 years ago by
Milestone: | 5.2 → 5.3 |
---|---|
Priority: | critical → major |
comment:11 Changed 4 years ago by
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
Attachment: | 0001-Resolve-variable-in-set-using-lookup-when-variable-i.patch added |
---|
Initial fix for compiler (interpreter still broken)
comment:12 Changed 4 years ago by
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
Milestone: | 5.3 → 5.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
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
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
Milestone: | 5.4 → someday |
---|
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 whenanother
importsfoo
,##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.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
.