View Ticket
Not logged in
Ticket UUID: 16fe1b5807820bb9705b460959f459989ec06404
Title: namespace ensemble command named ":" is mistakenly given the empty string as its name
Type: Bug Version: 8.6
Submitter: pooryorick Created on: 2017-11-10 11:48:07
Subsystem: 21. [namespace] Assigned To: dgp
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2024-11-21 13:45:53
Resolution: Duplicate Closed By: jan.nijtmans
    Closed on: 2024-11-21 13:45:53
Description:

This script,

namespace eval : {
    namespace ensemble create
    namespace export *
    proc p1 {} {
        puts {hello from p1}
    }
}

: p1
, results in the following error:

invalid command name ":"

User Comments: jan.nijtmans added on 2024-11-21 13:45:53:

Dup of [f14b33ec]. Closing.

The saga continues.


dgp added on 2019-06-17 18:05:25:
Re-opened. The change applied to fix this issue caused an 
unacceptable regression [8b9854c3d8ba]. This issue is still
captured as test namespcae-56.4 which is marked for now as
a "knownBug".

dgp added on 2019-03-11 14:19:59:
How in the world did the namespace facility ever permit any namespace to
have the name ":" ?

pooryorick added on 2019-02-07 16:17:37:

Disregard the previous comment. It was for [8b9854c3d8ba].


pooryorick added on 2019-02-07 15:47:09:

info level 0 simply returns all the words in the command associated with a level. It was not intended as a mechanism for resolving the command to a fully-qualified name, and it generally does not:

namespace eval n1 {} {
	proc p args {
		puts [info level 0]
	}

	namespace export *
	namespace ensemble create
	p n1
}

namespace eval n2 {} {
	namespace path ::n1
	p n2
}

The fact that the namespace ensemble command resolution routines ever rewrote the command name was incidental, and not intended to be part of the public interface. In order to fix [16fe1b5807] the ensemble subcommand lookup routines no longer make this artifact visible to info level. This is not a bug. Furthermore, info level 0 is now more consistent: It now always returns the command as provided by the caller, instead of just doing that in the common case.


pooryorick added on 2017-11-29 11:27:57:

Yes, see [93c437ef370480b3c80fb63726a3e460467df37|the fix] for the [the ticket you mentioned]. The original fix for the current issue was incomplete, but the Tcl test suite didn't detect it.


dgp added on 2017-11-28 19:50:29:
% oo::class create ns::foo
make: *** [shell] Segmentation fault

jan.nijtmans added on 2017-11-28 09:12:12:

It appears that this fix caused [6cf568a21bd8fc9b1f63543efe92cbedaa5a7160]. I'm not suggestiong that this fix is wrong, but it might be that this fix exposes a latent bug somewhere else. It's worth to have another look. Please! Any ideas?


aspect added on 2017-11-20 13:31:09:
Thanks for the detailed writeup!  Following along with that and the patches 
I have a much better idea, and like the look of what you've done:  it does 
make things more uniform and removes some string/dstring juggling which aids 
readability.

I was put off by the removal of error checking in Tcl_ProcObjCmd that prevents:

% namespace eval foo {proc : {} {}}

.. but with the unification you have performed, this can be restored in 
TclGetNamespaceForQualName to the benefit of all creative commands, if indeed 
that's what we want.

I'm still not too keen on enshrining creation of a namespace called ":" in 
the test suite (even though this was already possible, as you point out), 
but since it clearly identifies this bug the context is clear and the test 
can be adjusted in future.

One tiny nit:

> Tcl_Command TclCreateObjCommandInNs(
> Tcl_Command TclNRCreateCommandInNs(

should have newlines before the function name.

pooryorick added on 2017-11-20 08:12:09:

The TclOO segmentation fault had nothing to do with ":" characters, but rather with the act of replacing a command that happened to be a class with a command having the same name, causing the class to be deleted while a pointer to its structure was in use.


pooryorick added on 2017-11-19 16:26:55:

Regarding aspect's rationale:

  1. It's already possible to create commands like : and :foo. This fix does not affect that.
  1. The segfault is a separate issue, and is also now fixed.
  1. The fix *does* unify the name canonicalisation code referred to below.


pooryorick added on 2017-11-19 00:23:57:

Checkin [8251e62bab] additionally fixes the TclOO segmentation fault noted below, and updates coroutine and TclOO command creation routines to use TclCreateObjCommandInNs.


pooryorick added on 2017-11-18 21:04:04:

Fixed in [b433ae397cddec65].

The issue was that namespace.c/NsEnsembleImplementationCmdNR, which is responsible for resolving aned evaluating namespace ensemble commands, calls BuildEnsembleConfig , which slaps together a fully-qualified name for an exported command.

I modified BuildEnsembleConfig to stop building up fully-qualified names, and then modified NsEnsembleImplementationCmdNR to use the namespace for comand lookup rather than using a fully-qualified name.

On the "front end", I relieved TclProcObjCmd of the duty of slapping together a fully-qualified name, and created tclNRCreateCommandInNs for it to call instead of calling tclNRCreateCommand, which doesn't accept a namespace. This in turn required the creation of tclCreateObjectCommandInNs, and Tcl_CreateObjCommand becomes a thin wrapper over that.

TclEnsureNamespace is a new utility command that, given a namespace, either returns that namespace, or if that namespace is in a deleted state, returns a new namespace in its place.


aspect added on 2017-11-18 12:18:02:
I'm not comfortable with these changes being applied to a mainline branch, 
so I've moved them to [pyk-command-named-colon].

Rationale is that names like : or :foo reduce the utility of [namespace 
which] and break with documented behaviour ("two or more colons" are a 
namespace separator).  Further, as the TclOO segfault below illustrates, 
this is a can of worms likely to uncover implicit assumptions elsewhere 
in the core.

I suspect there's useful stuff in the patch, but I'd like to see a more 
considered approach .. preferably including unifying the name 
canonicalisation code referred to below.

pooryorick added on 2017-11-14 07:53:20:

Also:

namespace eval : {
    proc p1 {} {
        puts hello
    }
}

puts [namespace which p1] ;# -> ::p1


aspect added on 2017-11-11 01:54:52:
.. worms:

  [8.7a2] % namespace eval foo {proc : {} {namespace current}}
  can't create procedure ":" in non-global namespace with name starting with ":"
  [8.7a2] % namespace eval foo {proc x {} {namespace current}}
  [8.7a2] % namespace eval foo {rename x :}
  [8.7a2] % namespace eval foo :
  ::foo

  [8.7a2] % oo::class create {}
  object name must not be empty
  [8.7a2] % oo::class create :
  ::
  [8.7a2] % {} create {}
  object name must not be empty
  [8.7a2] % {} create :
  Segmentation fault (core dumped)

aspect added on 2017-11-11 01:47:20:
Simpler demo:

  [8.7a2] % namespace ensemble create -command :
  ::

The responsible code seems to be:

https://core.tcl.tk/tcl/artifact/9cf6780328312995?ln=672-681

which is distinct from how other commands (proc, coroutine) qualify names 
before calling Tcl_NRCreateCommand/Tcl_CreateObjCommand, eg (note the 
comment here):

https://core.tcl.tk/tcl/artifact/46a345d533c6582c?ln=8985-9015

seems like a great candidate for factoring into tclUtil.c and using 
uniformly across command-creating commands.

The first question is, is this the right approach?

The second is, what command-creating commands are there that should be 
brought into line?  This is a can of worms ..