Tcl Source Code

A problem was found with lappend using read traces on arrays inconsistently in the bytecompiled and direct evaluation code paths. Attached code demonstrating the issue.

This issue doesn't block progress on TIP 673.

That said, it's a mess worthy of some renewed attention, especially if
Tcl continues to show divergent behavior between direct evaluation and
bytecode execution of the same commands.

TIP 673 brings attention back to this issue.

Priority lowered: not worth blocking anything for this one.

How about lowering prio, given the risk assessment and available manpower ?

+1.

Note that INSTEAD (which is proposed by TIP #385) subsumes all the rest ;-)

The code is currently inconsistent - different behaviour between compiled and evaled code. This needs fixing irrespective of anything else.

As for minimizing the risk of code incompats: the min is not to change behaviours at all, leave append doing one thing and lappend a different one as it was for 10 years. If the issue is instead one of reasonability, my sense tells me that read/modify ops should trigger read traces, that lappend did it right and append did it wrong.

The best solution IMO is to extend the trace command so that traces can be defined to operate BEFORE or AFTER the requested operatio, maybe also INSTEAD. 

The original issue with ::env should be fixed differently IMO

The issue is that there were tests to the contrary for append.  The effect of traces is that I suspect a strong risk of existing code compatibility by changing 'append', whereas the lappend change obviously was noticed, but the direction it shifted (to lose the bytecode read trace) was considered the less offensive direction.

The patch explicitly removed setting the TCL_TRACE_READS flag into storeFlags in all lappend paths in TEBC.

The comments say "lappend now behaves like append". That change would require a TIP in my opinion, and I would rather vote for "append now behaves like lappend" myself: a read-modify clearly involves a read, both append and lappend should trigger read traces.

Both results were {1 2} before this patch (commit 552822d3bb) - read traces were triggered. After this patch (commit e6f97d1e4a) the divergence appeared: the BC path stopped invoking the read trace.

allow_comments - 0

lappend's behaviour is still inconsistent between BC and eval'ed code. On today's core-8-5-branch tip and trunk I get (reported by mbecroft in 3477581):


% apply {{} {trace add variable foo read {apply {{name1 name2 op} {upvar $name1 var ; set var 1}}} ; lappend foo 2}}
2
% apply {{} {trace add variable foo read {apply {{name1 name2 op} {upvar $name1 var ; set var 1}}} ; eval lappend foo 2}}
1 2
% apply {{} {trace add variable foo read {apply {{name1 name2 op} {upvar $name1 var ; set var 1}}} ; eval [list lappend foo 2]}}
1 2

allow_comments - 1

Done for 8.6 and 8.4 as well.

Committed to 8.5 branch, with tests.
Working on the 8.6 and 8.4 branches now.

In this case, ::env has had that behavior for numerous versions, and it is lappend that changed behavior, but oddly only for byte-compiled cases.  If I can figure out my poor reasoning of 10 years ago, it may have been that I believed the lappend direct eval would be a rarity, only in toplevel code.  I didn't consider the many other ways that wouldn't end up ever actually bytecompiled before running (another reason to kill off direct eval path totally).

In this case, lappend is also different than append, in that append doesn't trigger the read trace, so it again becomes "more consistent" with this change.

File Added - 385269: readtrace.diff

Can't we instead define the trace on env so that this doesn't fail?

My worry is that all commands with read/modify semantics (append, lappend, incr, others?) should properly trigger read traces. If they don't, we can't easily define "volatile variables" via read traces - a shame, I'd say.

Continued notes ...
===============

[HL] Note that YYYY is ZZZZ and we can probably fix/make changes to it to get it to work if necessary. But we are more concerned about user scripts compatibility and how will they be affected -- we have no way of modifying user scripts if they run into this situation. So we would like to get to the bottom of the issue -- primarily what is the intended behavior when read traces are set on non-existent variables and how lappend operates on it. 

===============

[JH] In the 2 switch cases below, you are seeing the subtle variation between a switch that can be bytecompiled (with --) and without (because $oper may be another option to switch, it checks at runtime instead of bytecompiling).

As to other variables, this does have the same behavior, if you follow the same trace proc that ::env has set by default:

proc test {oper var elem value}  {
    upvar #0 $var lvar
    switch -- $oper { add { lappend lvar($elem) $value } }
}
proc nonull {var key val} {
    upvar 1 $var lvar
    if {![info exists lvar($key)]} { return -code error "no such variable" }
}
trace add variable myvar read nonull
test add myvar key "new value"

The 'nonull' style of trace is what ::env has had set for a long time.  The difference is that byte-coded lappend changed to do read traces.  This was done in 8.4, but done incompletely such that only _bytecompiled_ lappend was affected.  Thus, you would have encountered this in 8.4 already had you written the autoeasy code slightly differently.

The answer may be to remove these read traces on variables, but do we do that for 8.4 and 8.5, or just 8.5+?  Or just for arrays, and leave for scalars?  That would be very odd.  I think that might be the best way to go, because otherwise lappend is simply a trap that cannot be used for currently empty ::env vars.  The question that needs to be first answered is what other impact this might have for those that expected to have the read traces executed.

What do you think of that?

===============

[HL] Is nonull trace only set for env variable or for other variables too? If this behavior is changed, someone else out there might be relying on this feature -- will very difficult to find out.

The other question is: should lappend be considered to have a read trace. How do append, lset or any other command which modifies existing variable behave. Do they invoke read traces whenever the variable value is updated or do they also depend on whether it is byte-compiled or not.

I think that the read-traces should be consistent in whether they are invoked or not for all scenarios and not be dependent on byte-compiled or not. Otherwise we would be waiting for another bug/fix cycle needed in the future.

Having said all this, [...], I would like it to not throw any errors on upgrade so that no modifications are required on our end to upgrade. 

===============

[JH] The nonull behavior is particular to the ::env array in Tcl in that it is the default behavior on that array defined by Tcl.  No other arrays have traces on them by default, but users could set up such themselves.

Only lappend has this read trace behavior turned on at the bytecode level, append does not.  I'm beginning to think this is just not good that lappend does this read trace in this way.  While technically correct, the other setter functions don't mirror this behavior, and lappend is only doing it when byte-compiled, not otherwise.

I'll prepare a bug report for this, as it will be good to have it documented.  I will probably have this behavior changed in both 8.4 and 8.5 branches as well as head, so it will again be consistent if you are using the latest.  Again, be warned though that lappend can cause this error in the current 8.4 if you have a fully byte-compiled code segment.

File Added - 385265: lappend_bug_hl.txt

Notes from the discussion of the problem ...
==================================

[JH] There is a related set of comments in append.test around this behavior change:

    # The behavior of read triggers on lappend changed in 8.0 to
    # not trigger them, and was changed back in 8.4.

This of course adversely effects ::env, which has a built-in read trace that errors on unknown vars.  So why does the first case not error in 8.4?  Apparently the fix to lappend only kicked in for the fully byte-compiled case. That means placing it straight in a proc, or in an if/else you would see the change, but 'switch' wasn't byte-compiled until 8.5.

At this point, I don't know what the best resolution is.  The intention of the change was to allow read traces to trigger with lappend.  ::env has a read trigger that errors on unknown variable reads.  This was new in 8.4, but again masked by code that didn't use the byte-compiled version.

==================================

[HL] Hi Jeff,

I have modified your example bit more ... The test1 passes in tcl8.5 but test2 fails. It seems that the behavior changes whether -- switch is specified after -exact or not. If I do not specify the -exact switch, then it fails for both in tcl8.5. So the "--" option seems to be changing its behavior.

Ok, assuming that this is the intended behavior -- what about regular array variables? Shouldn't it behave the same if I define a read trace on global array x with no elements defined for it but use lappend to add value to it.

proc test {var oper elem value}  {
  switch -exact -- $oper { add { lappend [set var]($elem) $value } }
}
proc foo args {puts $args}
trace add variable ::x read foo
test ::x add key "new value"
test ::env add key "new value"

It doesn't seem to throw any error for other tcl variables. So why is only ::env variable treated specially.

File Added - 385264: lappend_bug.txt