| 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,
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:
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 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:
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 I modified On the "front end", I relieved 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:
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 .. | |||