Tcl Source Code

This is perhaps much more of an [incr Tcl] bug.  I
submitted it  in that project first; see bug #631735. 
However, I thought I'd raise the issue here too, in
case it was important in some other context.  (If not,
my apologies.)

The essence of the issue is that the change in tclVar.c
to make the new ObjMakeUpvar() function call
TclLookupSimpleVar(), which in turn always calls any
registered variable resolver functions, make it very
difficult (maybe impossible?) for the "variable"
command to create a new local variable and link it to a
namespace variable, if a registered package like [incr
Tcl]'s variable resolver normally makes the namespace
variable visible in a local procedure scope.

When using the latest (post-3.2.1) CVS version of [incr
Tcl] with Tcl 8.4.1, the 5.6a test from the basic.test
file fails due to a new, deep change to the tclVar.c
file.  Specifically, the test fails with the message
"can't upvar from variable to itself".

(Note also that there are two 5.6a tests; the first one
is the one that fails.  In the suggested patch below,
the second one is renamed 5.6b.)

A simple test case is the following:

package require Itcl
itcl::class test_globals {
        common foo goo
        proc getval {name} {
            variable $name
            return [set [namespace tail $name]]
        }
}
test_globals::getval foo

Removing the "variable $name" command causes this test
to work.

A suggested patch for the [incr Tcl] test suite is
shown in bug #631735.  After some examination of the
problem, this seems to me to be the preferred route,
because I can't see any way to restore the old
behaviour of the "variable" command with 8.4.1 without
altering the tclVar.c file's functions considerably.

The reason for the failure is that in tclVar.c, the
MakeUpvar() function has been replaced in 8.4.1 with
the ObjMakeUpvar() function.  Further, this function no
longer duplicates some code from TclLookupVar(), but
instead, both functions now use the common
TclLookupSimpleVar() function.  TclLookupSimpleVar() is
also used by TclObjLookupVar().

The TclLookupSimpleVar() function always calls the
namespace's variable resolver(s) -- in this case,
Itcl_ClassVarResolver() from [incr Tcl]'s itcl_class.c.
 Previously, MakeUpvar() did not call the variable
resolver(s), but simply looked for local variables
using similar code as appeared in TclLookupVar() --
except for the calls to the variable resolver(s).

This is important because ObjMakeUpvar() -- previously
MakeUpvar() -- is called by the Tcl_VariableObjCmd()
function that handles the "variable" command, to create
a link between a local variable and a namespace variable.

Formerly, because MakeUpvar() did not call the
Itcl_ClassVarResolver(), it would find no local
variable when handling the test case shown above, and
could proceed to create a local variable as a link to
the namespace variable "foo", created by the "common"
command.

In Tcl 8.4.1, though, ObjMakeUpvar() calls the
TclLookupSimpleVar() function to locate and create or
replace the local variable that ObjMakeUpvar() wants to
turn into a link to the namespace variable.  The
TclLookupSimpleVar() function calls the
Itcl_ClassVarResolver() function, which of course finds
no local variable named "foo" and proceeds to locate
and return the namespace variable named "foo" that was
created with the "common" command.

ObjMakeUpvar() has also recieved a pointer to this same
variable from Tcl_VariableObjCmd(), which used the
TclObjLookupVar() function to locate the namespace
variable to be linked to.  And TclObjLookupVar() also
called TclLookupSimpleVar(), which in turn called
Itcl_ClassVarResolver().

So the ObjMakeUpvar() function now has two identical
pointers for the namespace and local variables, and
refuses to try to make a link.

The only way I can see to resolve this is to alter the
functions in tclVar.c to pass down some kind of flag
that would allow Itcl_ClassVarResolver() to determine
this specific case.  It can't rely on !(flags &
TCL_NAMESPACE_ONLY) because that may or may not be set
in various combinations, depending on the caller --
specifically, Tcl_FindNamespaceVar() in tclNamesp.c
calls  the variable resolver function(s) too.  Nor can
it rely on varFramePtr->isProcCallFrame because that
doesn't clarify which chain of functions is calling
TclLookupSimpleVar() or Tcl_FindNamespaceVar() -- these
are the two functions that can call the variable
resolver(s), but they are in turn called in various
contexts.

We'd have to specifically catch the case of
Tcl_VariableObjCmd() -> ObjMakeUpvar() ->
TclLookupSimpleVar() -> Itcl_ClassVarResolver(), and
specifically not catch the case of Tcl_VariableObjCmd()
-> TclObjLookupVar() -> TclLookupSimpleVar() ->
Itcl_ClassVarResolver().

All of this does raise one other small questions, which
is whether the code in Itcl_ClassVarResolver() that
looks for local variables, and is duplicated from
portions of the old TclLookupVar() function (now that
code has been moved into TclLookupSimpleVar() and
altered), should be updated.  For example,
TclLookupSimpleVar() doesn't perform this logic if
TCL_NAMESPACE_ONLY is passed.  However, when
Tcl_FindNamespaceVar() is calling
Itcl_ClassVarResolver(), TCL_NAMESPACE_ONLY could be
true or false -- so I don't see an obvious way to check
it within Itcl_ClassVarResolver(), without having some
additional context flags -- a similar issue as to the
one discussed above.

Well, hope that all helps someone.

Logged In: YES 
user_id=637999

Well, I don't think Itcl has changed at all -- I'm not a
developer of Itcl, I was just trying to use it -- but I
finally got around to pulling down the Itcl CVS sources and
compiling them against Tcl 8.4.5, and their "make test" does
work again, so I think you have indeed fixed the problem (if
it was a problem to begin with ... I never saw any action
from the Itcl folks on my matching bug report in their
database).  Any at rate, many thanks for persevering with
the fix!

File Added - 45846: diff

Logged In: YES 
user_id=148712

This should be fixed by the enclosed patch (already
committed to HEAD and core-8-4-branch).

Chris: could you please confirm that the issue is solved to
Itcl's satisfaction?

Logged In: YES 
user_id=79902

(SFBug: Summaries with double quotes in are marked as 
changed every time someone updates a bug at all...)

Logged In: YES 
user_id=637999

Yes, that's it in a nutshell, Miguel.  I've been away from
this problem for a long time, so I hope I'm not leading you
astray.  If memory serves, Itcl allowed one to use
"variable" in a class method (sort of like a namespace proc)
to refer to a "common" var (like a namespace var).  But you
could also just leave out the "variable" and the common
class var was automatically resolved within the method's
(proc's) scope.  (I think.)  It seemed to me when I was deep
into the problem that while cool, this wasn't maybe
something Tcl really had to support, and hence my proposed
"fix" for the Itcl test suite is simply to say that that old
behaviour is now reversed; "variable" doesn't find the
common var.  But I don't know how many people rely on  this,
or use/maintain Itcl these days, etc., and would have
problems with such a "fix".

Logged In: YES 
user_id=72656

Assuming miguel added a call to 'test' before the 'set a', I 
think that may be a correct assumption, and the way that itcl 
uses common.  The basic var resolver still works, as shown 
in this case:

itcl::class resolve {
  common foo 0
  proc getval {name} {
    return "$name: [set $name]"
  }
}
resolve::getval foo

but the handling of 'variable' seems to have been tripped up.

Logged In: YES 
user_id=148712

Doh ... that script should be 

  namespace eval ns { 
      set a 0
      proc test {} {set a 5}
      test; # FORGOT TO CALL THIS
      set a
  }

Logged In: YES 
user_id=148712

That is my understanding: if an extension defines a resolver
for namespace ns, the script

  namespace eval ns { 
      set a 0
      proc test {} {set a 5}
      set a
  }

*could well* return 5, not 0. Ie, if I understand correctly
the way the resolvers work (see TclLookupSimpleVar,
tclVar.c:640), the resolver has priority over the tcl rule
that would make this a local var. 

This seems to be what is tripping itcl here: in the example
above (I'm more or less guessing what itcl's resolvers do)
   common foo
causes that, in any proc in the test_globals class, the name
"foo" refers to the namespace var and not to a local var.
Thus, the new ObjMakeUpvar code finds the namespace variable
"foo" when trying to create a local var "foo", and errors
out with "can't upvar from variable to itself". 

The previous MakeUpvar would sidestep calling the resolvers
- it implemented a special case of variable lookup. 

Chris: is my understanding correct so far?

Logged In: YES 
user_id=80530

That's clearer, but isn't it wrong?

In plain ordinary Tcl, with

proc test {} {
    set a 5;
}

the variable "a" would always
be a local variable.  If I wanted
it to refer to the namespace-level
variable named "a", I'd expect to
have to call [variable a] first.

Or is this discussion (and the
notion of "resolver scope") only
relevant when some 
extension-provided variable
resolver is in control ?

Logged In: YES 
user_id=148712

That wasn't too clear, was it? My problem is with something like

    proc test {} {
        set a 5;
        return $a
    }

If there is a variable named "a" within resolver scope, it
will be set to 5 (side-effect). Otherwise, "a" will be local
to the proc, and there are no side-effects. IIUC.

Logged In: YES 
user_id=148712

1) *wonderful* bug report, with so much detail. Thanks!

2) I'm not sure I understand why Itcl (or any other
extension) would want to "create a new local variable and
link it to a namespace variable, if a registered package
like [incr Tcl]'s variable resolver normally makes the
namespace variable visible in a local procedure scope." The
semantics are somewhat mysterious to me.

The point being: the local name already refers to the
namespace variable per the resolution rules; why do we need
to create a local var that also refers to the same namespace
variable? Performance?
 
This misunderstanding goes deeper into the problems I have
with the name resolution rules: the whole resolver mechanism
implies that a variable name within a proc defines a
variable if the resolver cannot find a pre-existing
same-named variable in the resolution path; otherwise, it
refers to the resolved variable. State-dependence of (name
resolution rules and appearance of side-effects) sounds
pretty dangerous to me.

This being said, I'm working on patch that should solve this
issue.