Tcl Source Code

View Ticket
Login
Ticket UUID: 3598385
Title: [dict exists] doesn't produce an error for non-dict values
Type: Bug Version: current: 8.6.0
Submitter: pooryorick Created on: 2012-12-24 18:37:58
Subsystem: 15. Dict Object Assigned To: dkf
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2018-05-21 00:23:35
Resolution: Wont Fix Closed By: aspect
    Closed on: 2018-05-21 00:23:35
Description:
set data {one 1 two}; dict exists $data one ;# -> 0
User Comments: aspect added on 2018-05-21 00:23:35:

+1 error.

I don't think the cases should be distinguished: [dict get {a b} a b] is exactly the same category of error. Considering it a bug for [3475264] was too hasty. Dictionaries with non-dict leaves are only recursive structures until you hit a leaf - applying too many keys with [dict exists] is just as wrong as doing so with [dict set] or [dict get].

Let me emphasise: an arbitrarily-shaped tree with values at the leaves is not simply represented in a dict, since those values may accidentally look like dicts. We all know this, but I've seen myself and others stumble on it many times. I think [3475264] came from the same mistaken place, and I struggle to imagine correct code that could be rendered incorrect by an error being raised in that case.

In [3475264] ticket dkf observed:

> based on the current documentation it should not be throwing errors

which is false: the manual prescribes only when [dict exists] returns true.

The current situation is asymmetric and surprising on at least two counts:

1. Every other dict subcommand raises an error when asked to treat a non-dict value as a dict.

2. the (implicit but obvious) identity is broken:

[dict exists $d $k ...] == [dict exists $d $k] && [dict exists [dict get $d $k] ...]

as a user I expect this because:

[dict get $d $k ...] == [dict get [dict get $d $k] ...]

3. {[dict exists $d {*}$k] == 0} doesn't tell me that [dict set $d {*}$k] will succeed


dkf added on 2018-05-20 15:07:58:

The issue is that the case of a malformed dictionary at one level is awkward to distinguish from the other cases. The "obvious" thing to do isn't obvious, at least to me, given that I've had people ask for things to be both ways round and the two ways appear incompatible interpretations to me. Personally, I'd rather have the "that is not a dictionary at all" case be an error (and I believe I wrote it that way originally) but I got convinced to change it. I refuse to thrash back and forth; sort out between yourselves what it should be and I'll willingly make it work that way if it doesn't already.

Until then, this issue stays closed.


apnadkarni added on 2018-05-20 08:00:14:
+1 that this should generate an error if the value cannot be interpreted as a dictionary. What I think folks were grumbling about earlier was ambiguity related to nested keys but in that case the value was still interpretable as a valid dictionary.

Consider also for consistency how lsearch (for example) behaves with badly formed lists.

% lsearch  {a b "} c
unmatched open quote in list

ferrieux added on 2013-01-01 01:53:09:
obviouslt, I meant "exists" instead of "get"

ferrieux added on 2013-01-01 01:51:36:
To add to pooryorick's arguments, a natural source of odd-length lists is the typo

  [dict get d $k]

(should be "$d"). This typo is likely to occur because the dict API mixes both styles (for good reasons).
But the net result of the Jan-2012 commit is that this case will NO LONGER raise an error. Instead, it will just silently behave as though the dict had been emptied (ie return 0 for all keys).

For this frequent case at least, it looks to me like an obvious regression.

pooryorick added on 2012-12-31 23:12:22:
Donal says he hasn't seen a compelling reason to revert.  In the case that led to this report, a function wanted to assume that its argument was a dict, and when it wasn't, it took some development time to trace back to the fact that the "type" of the argument was invalid in the first place.  Perhaps Donal could explain how "will this [dict get] work" is actually the operation of greatest practical utility.  It seems to me that any programmer asking about the existence of a key in a dict would be surprised if that dict wasn't actually a dict, and would want to know about it via an error message in order to fix the problem in the program logic.  Also, doesn't [dict exists], like the other [dict] commands, have to convert the value to a dict first, and at that point, wouldn't there be an opportunity to raise the standard "missing value to go with key" (I'm now used to that verbiage as meaning the dict is invalid) error?  Regarding [string is...], maybe it should be promoted to [is], and given some mechanism for extension.  But I digress.

mistachkin added on 2012-12-27 06:53:27:
How about an alternative suggestion.  Let's add 'string is dict' and leave the current dict exists behavior alone?

ferrieux added on 2012-12-27 02:09:46:
OK, here's my reasoning:
Turning a tristate (bool + error) into a boolean loses information: what's gained by this intentional loss ?
Given a tristate, a simple [catch] restores the simple yes/no behavior that seems to be so important.
While with the current state, the accidental un-dictness of a value is silently disguised as an empty dict.
Would you advocate for [lsearch] to just return false when SetListFromAny fails ?

dkf added on 2012-12-26 07:15:20:
I've also yet to see a compelling reason to revert. Not closing yet, but tending to a "won't fix" as I'm not convinced that thrashing back and forth between these two behaviors is a good idea at all.

dkf added on 2012-12-26 07:12:31:
When we had it the other way, people also grumbled. Right now, [dict exists] answers the question "will this [dict get] work?" and that is the operation of greatest practical utility.

The problem is that when you get into nested dictionaries, the cases become extremely difficult to tell apart. At the C level, I can tell that the lookup fails. I can't tell the deep reason why this is the case: did someone get the path wrong, or did they get the value wrong in the first place? Two cases (according to whose mistake it was) that you might possibly want distinguished, but the information to do that isn't there with the type of type system that we have. All we have is that it didn't work (with potentially some information in the error that *might* track it down, if it is from a useful perspective; that's the info that is dropped in [dict exists]).

I suppose the error message "missing value to go with key" could be improved.

ferrieux added on 2012-12-25 08:28:09:
For the record, the commit "fixing" the bug you mentioned is 22ec97b057 .
Donal, I really don't buy that move. It does comply to an unclear documentation, but in the most surprising way, by freezing into law what is only the most counterintuitive of the possible interpretations :}
Please please consider reverting: as pooryorick rightfully points out, this leads to very hard-to-track bugs, for a dubious benefit... or am I missing a big thing ?

ferrieux added on 2012-12-25 07:58:21:
Hmm, I suddenly realize the meaning of this error message "missing value to go with key": it does not refer to the key arg passed to [dict get], but to the odd list element... So it is just a cryptic form of "value is not a dict" ;)

So, back to your suggestion, I agree that letting [dict exists] ignore the ill-formedness of the dict is just plain wrong.
I'd vote for a fix with a marked POTENTIAL INCOMPATIBILITY as soon as 8.0.1.
But that's Donal's realm, let him decide.

ferrieux added on 2012-12-25 07:30:58:
I agree with your intuitions. However, I wonder whether there was a change between 8.5 and 8.6 in this area: both tips from 8.5 and 8.6 branches yield:

% set x {1 2 3}
1 2 3
% dict get $x 1
missing value to go with key

Clearly, as you suggest, something like "value is not a dict" would seem better.
Definitely for novem; unsure about 8.x. Donal ?

pooryorick added on 2012-12-25 02:02:05:
 set data {one 1 two}; dict exists $data one ;# -> 0

pooryorick added on 2012-12-25 02:01:31:
doc for [dict exits] states "This returns a true value exactly when dict get on
that path will succeed", so I think the fix in 3475264 was to make behaviour
conform, to the documentation.

But even as currently written, the documentation could allow for an error if a
if a key is checked in a value which isn't a dict