Re: [Chicken-hackers] [PATCH] [5] Aggressively reject definitions in expression contexts (#1309)

[Peter Bex]

On Sat, Feb 25, 2017 at 06:13:48PM +0100, Peter Bex wrote:
> However, I figured out that the real cause of #1309 is much, much deeper
> and has to do with a nasty bug in ##sys#canonicalize-body: it invokes
> "fini" as soon as it sees a non-pair.  However, fini doesn't handle
> expansions the same way as the body.  For example:
> 
>   (let () 1 (begin (define blabla 3)) blabla)
> 
> This will define blabla globally (or trigger an error in CHICKEN 5 with
> the patch), because the "1" stops the begin from being expanded properly.
> I fear that this requires a pretty invasive rewrite of
> ##sys#canonicalize-body.  I'll get back to y'all on that.

Attached is an updated and hopefully final patch set to tackle this bug.

The fix for the nested "begin" mentioned above is pretty simple: The list
of special cases in "fini" needs to be extended with ##core#begin; if that
is encountered, we need to call "expand" again, which will flatten all the
begins.

But the trickier bit here is that the forms are all macro-expanded by
##sys#canonicalize-body.  This results in three problems:

1) A form that expands to something that eventually expands to "define"
    was expanded just once and then processed by the compiler as a
    regular form, resulting in a toplevel define, which is incorrect.
2) After fixing the above by expanding macros completely in
    ##sys#canonicalize-body, line numbers are lost in the compiler because
    ##sys#canonicalize-body does not have access to the line number
    database maintained by the compiler.
3) "import" and "module" need special consideration with macro expansion.

Here, number 1) was fixed by simplifying macro expansion a bit: the
"expand" procedure no longer tries to do macro-expansion or detection
of calls to any locally defined procedures or macros.  Instead, "fini"
now does this.  It will expand until it finds a define, "begin", "import"
or "module", and then either hand back control to the compiler or
re-invoke "expand" on the remaining body to grab all the defines it
can find.

Number 2) was easily fixed by adding yet another hook.  It's ugly, but
I've discussed this on IRC with Felix and he sess no other way to do
it either.  The advantage is that this actually _improves_ precision
of line numbers, as you can see in the final patch's hunk that changes
the scrutiny-2.expected; it now correctly knows that the (pair?) call
is on line 20 instead of on line 14 (which is the start of the LET).

Number 3) deserves some more attention.  It turns out that a (module)
form which instantiates a functor will expand to two (module ...)
forms, one for an "internal" module and one for another module that
imports this internal one.  However, the macro will look up the
other module at expansion time, in a table that's only populated once
the ##core#module form is processed by the compiler or interpreter core.
Thus, if you expand both module forms one by one, this will fail.

The other issue is with "import".  In functor-tests, there is a test
that creates three different functors, which are then used in an
inner define:

(define output
  (with-output-to-string
   (lambda ()
     (import (2x print))
     (print-twice #\a)
     (import (2x noop))
     (print-twice #\a)
     (import (2x write))
     (print-twice #\a))))

This is supposed to import (2x print) and use print-twice from that
module.  Then it will import module (2x noop) and use print-twice from
there, then  import module (2x write) and use its print-twice.  This
_requires_ that expansion of import is delayed.

Normally, one would see an import as a "global action", affecting all
subsequent uses of the identifiers from the imported module.  This is
exactly what happens if you expand the import, then process the rest
of the body.  So what needs to happen is to process all the forms,
except for any imports.  Then, the compiler can process the import,
process the following form, process the import, process the form, etc
to get the interleaving that was intended.

This works in the above code in CHICKEN 4, but only "by accident".
A simple change can already break this behaviour:

(define output
  (with-output-to-string
   (lambda ()
     (import (2x print))
     (print-twice #\a)
     (import (2x noop))
     (print-twice #\a)
     (define whatever 1) ;; This interferes with the final print
     (import (2x write))
     (print-twice #\a))))

This causes the imports to be expanded incorrectly.  All this is
now handled correctly with the attached patches, including the
r7rs test case from the original bugreport; the r7rs version of
define-record-type is now correctly seen as an internal define.

And truly incorrectly placed defines are now rejected with a syntax
error, of course :)

Cheers,
Peter

0001-Reject-toplevel-definitions-in-non-toplevel-contexts.patch
Description: Text Data

0002-Fix-invalid-definition-caught-by-previous-commit.patch
Description: Text Data

0003-Export-internal-define-like-definitions-from-chicken.patch
Description: Text Data

0004-Change-the-way-LET-bodies-are-macro-expanded.patch
Description: Text Data

0005-Add-expander-hook-so-compiler-can-track-line-numbers.patch
Description: Text Data

signature.asc
Description: Digital signature