STklos icon indicating copy to clipboard operation
STklos copied to clipboard

Rebinding car leads to unexpected behavior

Open jpellegrini opened this issue 2 years ago • 10 comments

Hi @egallesio !

I was working on SRFI 101, which redefines car, cdr etc, and found this:

stklos> 
(define-module x 
  (export (rename mycar car))
  (define (mycar . args)
    (print 'MYCAR)
    (apply car args))) ;; the binding of car used here should be the 
                       ;; one visible at this point, but it doesn't seem to be...

stklos> (import x)
stklos> car
#[closure mycar]
stklos> 

stklos> (car '(1 2 3))
MYCAR
MYCAR
MYCAR
MYCAR
.
.
.  ;; (infinite loop)

Why?

jpellegrini avatar May 03 '22 21:05 jpellegrini

Does the call to apply actually retrieve the binding of the procedure at runtime? That would explain this... And maybe also #367 .

jpellegrini avatar May 03 '22 21:05 jpellegrini

Aha! It's apply, indeed:

stklos>
(define-module x 
  (export (rename mycar car))
  (define (mycar x)
  (print 'MYCAR) 
  (car x)))

stklos> (import x)
stklos> car
#[closure mycar]
stklos> (car '(1 2 3))
MYCAR
1

jpellegrini avatar May 03 '22 21:05 jpellegrini

By the way... I am embarrassed -- I can't find where apply is defined in the source code! Where is it?

jpellegrini avatar May 03 '22 23:05 jpellegrini

Weird.....

I can't find where apply is defined in the source code! Where is it?

in vm.c

egallesio avatar May 04 '22 06:05 egallesio

in vm.c

Ok, I think I understand now... There's this stub:

DEFINE_PRIMITIVE("apply", scheme_apply, apply, (void))
{
  /* This function is never called. It is just here to declare the primitive
   * apply, as a primitive of type tc_apply
   */
  STk_panic("Inside apply. Should not occur");
  return STk_void;
}

But when apply is used in the source code, the VM doesn't call the primitive. Instead, there is this code, which I believe is where the real apply is performed (is this correct?):

    case tc_apply: {
      SCM l, func, *tmp, *argv;
      int len;

      if (nargs == 0) STk_error("no function given to apply");

      nargs -= 1;
      argv   = vm->sp + nargs;
      func   = *argv;
      .
      .
      .

It does func = *argv, which as I understand should take the procedure (and not its name)... But if this was the case, the behavior I showed above would not be possible, right?

jpellegrini avatar May 04 '22 09:05 jpellegrini

Hi @jpellegrini,

I think there is no problem here, in fact :wink: : module x implicitly import module STklos. That means that using car in module x results in using the car of STklos. A simple way to correct this consists in importing the SCHEME module in x.

stklos> (define-module x
  (import SCHEME)
  (export (rename mycar car))
  (define (mycar . args)
    (print 'MYCAR)
    (apply car args)))
;; x
stklos> (import x)
stklos> car
#[closure mycar]
stklos> (car '(1 2 3))
MYCAR
1
stklos> 

You can even use (import (only SCHEME car)), if you don't want to import all STklos.

egallesio avatar May 15 '22 16:05 egallesio

Hi @egallesio !

A simple way to correct this consists in importing the SCHEME module in x.

Thank you. I suppose it will also work with modules created in C. I'll check that later.

jpellegrini avatar May 17 '22 00:05 jpellegrini

Hi @egallesio !

It didn't seem to completely fix the issue -- see my comments on PR #404 . One way to reproduce the problem is this: Get the SRFI 125 implemention from PR #404, remove the s125: prefix from al functions, and set the deb variable in the beginning of the file to #t. Compile, then load SRFI 125 andtry to create hash tables. It seems that after make-hash-table finishes, STklos internal code runs, and tries to use its internal hash tables -- but it ends up using the hash tables from SRFI-125, and becomse confused by the different signature.

I also tried defining SRFI 125 as R7RS library (not module); and importing SCHEME, as you recommended, but that didn't help...

jpellegrini avatar Jun 24 '22 12:06 jpellegrini

Maybe the internal STklos code could run in a separate module? stklos-internal?

jpellegrini avatar Jun 24 '22 12:06 jpellegrini

And... Is this related to #367 also?

jpellegrini avatar Jun 24 '22 12:06 jpellegrini