#602 APPLY doesn't copy its inputlist input when running a primitive -- list may be modified

[Vilim]

to abc :x
show apply "memberp fput :x [abc]
end

> abc "a
< true
> abc "b
< false
> abc "a
< false

I presume the modification was meant as an optimization for the case where the function is called multiple times with the same arguments, but it causes bugs when called with a different first argument and a persistent second argument.

[David Costanzo]

Nice bug report. This is not reproducible in UCBLogo, which means we just have to figure out how Brian fixed it and merge that change into FMSLogo.

Why do you think this is due to MEMBERP modifying its argument and not some interaction between APPLY and FPUT? Were you part of the bug fix in UCBLogo?

[David Costanzo]

When comparing the UCBLogo source code to FMSLogo's source code, I saw a difference at begin_apply in eval.cpp.

Changing

text assign(argl, cadr(val));

to

text assign(argl, append(cadr(val), NIL));

fixed the bug for the repro you gave.

The use of appendwith a NIL argument is mysterious and has no comment to justify it. cadr should be the parameters to MEMBERP. append with a NIL second argument will create a new list, but there are clearer ways to do that.

I don't know if this is the right fix, or if the real problem is as you wrote, that MEMBERP is modifying the argument list. I also don't know if FMSLogo should make different fixes from UCBLogo, even if they're better.

[Vilim]

Copying the arguments into a new list would nullify all the sound uses of the argument modification pattern, like preserving invisible modifications like string interning and conversions to numbers.

[David Costanzo]

Vilim writes:

Copying the arguments into a new list would nullify all the sound uses of the argument modification pattern, like preserving invisible modifications like string interning and conversions to numbers.

I don't think that happens. It looks like it happens if you read the source code, but I wrote a more straight-forward version of your repro steps:

to abc
  show memberp "a "abc
end

abc
abc

Then I set a break point in lmemberp. It's args parameter was a different pointer each time. That is, in the common case, it's given a different list each time, so whatever was cached in the list is thrown away before its next call. If you trace through eval.cpp (sourceforge.net) you can see why: argl is built new each time an expression is evaluated in the accumulate_args label.

I know don't why memberp_helper has code like:

setcar(cdr(args), cnv_node_to_strnode(obj2));
obj2 = cadr(args);

setcar(args, cnv_node_to_strnode(obj1));
obj1 = car(args);

But I think it's a cheap way to ensure that the old NODE gets freed and the new NODE doesn't with FMSLogo's reference counting memory management. UCBLogo, which has a proper garbage collector, does not set obj1 and obj2 back into the list.

The bug you reported can be seen more directly as

make "list [a abc]
ignore apply "memberp :list
show :list
[a a]

This is a good justification for why UCBLogo always copies arguments list input to APPLY; the list may be a variable and could be given to a different procedure with APPLY, where the subtle optimization may change the behavior. Or, the list could be drastically changed by :ERRACT, which violates the promise that lists are immutable.

make "list [1 hello]
make "erract [2]
show apply "sum :list
3

show :list
[1 2]

[David Costanzo]

I have committed a fix as [r6105]. This fix is similar to what UCBLogo does, but the argl list is only copied when the function is a primitive, as only primitives have the potential to modify the arguments.

[David Costanzo]

I forgot to mention...the fix will be available in FMSLogo 8.5.0.