Bug - Fixed ASH does not create final slice in multi-maps

Veracity

Developer
Staff member
That's obscure.

int[int] map1 => a map from int -> int
int[int][int] map2 => a map from int -> (map from int -> int)
int[int, int] map3 => (the same)

In Java, when you fetch from a map, if the element is not present, you get a null.
In ASH, when you fetch from a map, if the element is not present, you get the default for the value type.
If you have a 2-dimensional map, as above:

map3[1, 2} => 0 (default for int)
map3[1] => an anonymous int[int] map (default for int[int])

The issue is that ASH will create intermediate "slices", when fetching, but will not save the final slice.

Here's an example script:

Code:
int[int] map1;
int[int][int] map2;
int[int,int] map3;

print(map1);
print(map2);
print(map3);
yields
Code:
aggregate int [int]
aggregate int [int, int]
aggregate int [int, int]
(A map from a key to another map is the same as a multi-dimensional map)
Code:
print("map1 contains 1 = " + (map1 contains 1) +
      " map1[1] = " + map1[1] +
      " map1 contains 1 = " + (map1 contains 1));
yields
Code:
map1 contains 1 = false map1[1] = 0 map1 contains 1 = false
(Fetching from a single-dimension map with a non-existent key does not add the key)
Code:
print("count(map2)  = " + count(map2) +
      " map2[1] = " + map2[1] +
      " count(map2) = " + count(map2));
yields
Code:
count(map2) = 0 map2[1] = aggregate int [int] count(map2) = 0
(fetching from a multi-dimensional map without using all the indices gives you a "slice" - but does not put it in the map.
(This is the issue)
Code:
print("count(map2)  = " + count(map2) +
      " count(map2[1])  = " + count(map2[1]) +
      " map2[1][2] = " + map2[1][2] +
      " count(map2)  = " + count(map2) +
      " count(map2[1]) = " + count(map2[1]));
yields
Code:
count(map2) = 0 count(map2[1]) = 0 map2[1][2] = 0 count(map2) = 1 count(map2[1]) = 0
(Fetching using all the indices when the first one does not exist will create the first slice, but not the second.)
(The same issue)

Here is an example of how this is a pitfall for the ASH coder:

Code:
int[int][int] map4;
print("map4 contains 1 = " + (map4 contains 1));
int[int] map5 = map4[1];
print("(after fetch of map4[1]) map4 contains 1 = " + (map4 contains 1) +
      " map5 = " + map5 +
      " map5 contains 2 = " + (map4 contains 2));
map5[2] = 10;
print("(after store of map5[2]) map5 contains 2 = " + (map5 contains 2) +
      " map4[1] contains 2 = " + (map4[1] contains 2))
yields
Code:
map4 contains 1 = false
(after fetch of map4[1]) map4 contains 1 = false map5 = aggregate int [int] map5 contains 2 = false
(after store of map5[2]) map5 contains 2 = true map4[1] contains 2 = false

The ASH program intentionally fetched an intermediate slice.
It modified it.
It might have hoped that it was modifying the original map, but no.
If the original key was not present, it must store it back in the original map.

Perhaps like this:

Code:
int[int][int] map;
int[int] data = map[1];
if (!(map contains 1)) {
   map[1] = data;
}
data[2] = 10;

or perhaps like this:

Code:
int[int][int] map;
int[int] data = map[1];
data[2] = 10;
map[1] = data

In Java, it would be obvious you have to do this; fetching a value from a map with a non-existent key returns null - and if it is a mutable value which you intend to change, you have to create a new one and store it back in the map.

ASH does not have null. It would simplify things for even experienced coders if fetching a mutable element using a non-existent key not only returned the default value, but also stored it in the map with that key.
 

heeheehee

Developer
Staff member
This logic is at the end of CompositeReference.getSlice(), per the if (it.hasNext()) block. (You could also put it into CompositeReference.getValue(), which probably makes more sense.)

I personally would like if we could make all such accesses mutating (as is the case in C++ -- if you want a non-mutating lookup, you use .find()), but that ship has sailed, as I'm sure all sorts of scripts rely on the current behavior.
 

Veracity

Developer
Staff member
This logic is at the end of CompositeReference.getSlice(), per the if (it.hasNext()) block. (You could also put it into CompositeReference.getValue(), which probably makes more sense.)
Yeah, I don't think this will be too difficult.

Note that it is not just multi maps: it is any map whose value is a Composite - a map or a record.

I am an "experienced coder" who was fooled by the current behavior. Code from my cheese analysis program:

Code:
record data_set
{
    string bbstat;    // Which stat is used
    int[int] xvals;    // Map from stat to count
    Point[int] points;    // Array of points
};
...
data_set[string] stat_encounters_data;
...
    data_set data = stat_encounters_data[encounter];
    data.bbstat = bbstat;
    data.xvals[x]++;
    data.points[count(data.points)] = point;
   // *** I initially did not have the following line
    stat_encounters_data[encounter] = data;

And I was confused, later on in the program, when stat_encounters_data was empty. I put in some debug prints, saw where the failure occurred, thought about how ASH was implemented, and then went "duh". I mean, I knew that the record would be empty the first time - which is why I set the stat - but also expected to get the same record back in subsequent calls - which is why I updated xvals and point.

I personally would like if we could make all such accesses mutating (as is the case in C++ -- if you want a non-mutating lookup, you use .find()), but that ship has sailed, as I'm sure all sorts of scripts rely on the current behavior.
Not an expert at C++ (one 6 month project of new code, various modifications of existing code) - as opposed to C (20+ years), Lisp (5 years), and Java (15+ years) - and that C++ project was back in the 90's.

So, not understanding what you are getting at here. Googling map::find says it returns an iterator pointing to the element - and that is the usual way to look up an element by key - and you get an iterator to the end of the map (i.e., not pointing to an actual value?) if it is not present. A "mutating" access would create the element if it were not present? How do you do that in C++?

What I am proposing is to make all "composite" - i.e. mutable - data types be entered into the map if you fetch them and they are not already present. We could do the same thing for non-mutable data types, but since you can't change those without storing a new value in the map, the current behavior - repeated fetching will continue to give you the type-default value but not change the map - would be indistinguishable - if all you did was fetch values.

Huh. You are suggesting:

Code:
int[int] map;
print(count(map));      // 0
print(map[10]));        // 0
print(count(map));      // 1
print(map contains 10); // true
print(map[10]));        // 0

Yeah, I think that would be an unexpected change.
 

heeheehee

Developer
Staff member
A "mutating" access would create the element if it were not present? How do you do that in C++?
I meant via map[key]. In C++, std::map::operator[] performs an insert_or_lookup.

But yeah, I'm sure that would break all sorts of things if we made that change now.

edit: no, I did not mean to type std:🗺️:eek:perator, forum
 

xKiv

Active member
Huh, weird that kolmafia is doing perl's autovivification now differently from perl ;-)
One point of tangentially related note that I just read about - what about deleting an element from a nonexistent intermediate slice?
IOW
Code:
int[int][int] map;

delete map[0][0]; # is this an error? was it an error before this fix? should it be an error?

print count(map); # 0 or 1? Did map[0] spring to life only to have one element created and immediately removed?
# In Perl it apparently does, which is considered a wontfix bug ...
 

heeheehee

Developer
Staff member
I don't think this fix changed the behavior of `remove`.

r26419 (prior to the fix) prints 1 in your example (after fixing up syntax).
 

Veracity

Developer
Staff member
map[0] did and still does “spring to life”.

count(map) == 1
(map contains 0) == true

However, since map[0] is a int[int] (and fetching a key in it gives an int, which is immutable)

count(map[0]) == 0

We discussed actually doing a set of the default value, which would have made it 1 - which presumably would make it like Perl (which I have not coded in for 20 years. 🙂).

The behavior of remove has not changed.

remove map[0][0] will probably create map[0], not create key 0 in it (since the default is in 0), and then look to remove, and discover that it does not contain the (default) next key of 0, and do nothing further.
 
Top