Bug - Fixed remove doesn't seem to work on maps with records

[QVamp]

Hey all. I just ran into this. I don't *think* I'm doing anything wrong here, but let me know if I did.

I modified the existing wiki example of removing an element of a map, to show the problem. If a map contains records, then the 'remove' function gives flaky results. The item shows as removed, unless you try to access it, in which case it is there.

void main() {
 record my_type {
  	int num;
	string str;
 };
 my_type rvar;
 rvar.num=3; rvar.str='foo';

 my_type rvar2;
 rvar2.num=8;rvar2.str='bar';

 my_type [int] map1;
 item key;
 map1[2] = rvar;
 map1[5] = rvar2;
 print( count( map1 ) + " " + map1 contains 2 + " " + map1[2] );
 print( "remove: " + remove map1[2] );
 print( count( map1 ) + " " + map1 contains 2 + " "  + map1[2] );
 foreach key in map1 {
	 print(key + " (" + map1[key] + ")");
 }
 print( count( map1 ) + " " + map1 contains 2 + " "  + map1[2] );
}

produces output:

2 true record my_type
remove: record my_type
1 false record my_type
2 (record my_type)
5 (record my_type)
2 true record my_type

As you can see... when I loop through the entries, it is there. And the same print statement, after the loop, provides different results.

 

[Veracity]

Hey all. I just ran into this. I don't *think* I'm doing anything wrong here, but let me know if I did.

I'll explain what you did wrong.

The issue is that ASH does not have the concept of a "null pointer". When you ask to retrieve a particular element from a map, it cannot return a null to tell you that the element was not there. It has to return an object of the type you asked for. It returns the default for that type, whatever it is. And, the process of retrieving that element will insert the default into the map.

I'll have to consider whether that is correct. Perhaps not - in which case we do have a bug. Just not the one you thought you were demonstrating.

Your program:
- inserts elements in a map
- removes an element
- references a removed element - which reinserts the default

I modified your script to be this:

record my_type {
    int num;
    string str;
};

void print_my_type( string tag, my_type rec )
{
    print( tag + ": num = " + rec.num + " str = " + rec.str );
}

void main()
{
    my_type rvar;
    rvar.num=3; rvar.str='foo';

    my_type rvar2;
    rvar2.num=8;rvar2.str='bar';

    my_type [int] map1;
    map1[2] = rvar;
    map1[5] = rvar2;

    print( count( map1 ) + " " + map1 contains 2 + " " + map1[2] );
    foreach key in map1 {
	print_my_type( "key = " + key, map1[key] );
    }

    print( "remove: " + remove map1[2] );
    foreach key in map1 {
	print_my_type( "key = " + key, map1[key] );
    }

    print( count( map1 ) + " " + map1 contains 2 + " " + map1[2] );
    foreach key in map1 {
	print_my_type( "key = " + key, map1[key] );
    }
}

When I run it, I get this:

> mmm.ash

2 true record my_type
key = 2: num = 3 str = foo
key = 5: num = 8 str = bar
remove: record my_type
key = 5: num = 8 str = bar
1 false record my_type
key = 2: num = 0 str =
key = 5: num = 8 str = bar

Do you see what happened?

 

[QVamp]

The explanation makes sense, but it does seem dangerous to insert an key/value into the map without the programmer issuing an actual assignment. I'll add some of your explanation to the wiki, but I'm feeling like the example itself at: http://wiki.kolmafia.us/index.php?title=Data_Structures#Remove is dangerous, since it actually does this (though it uses primitives) and does NOT indicate that this is a bad thing to do. Do you agree, and if so, I'll modify that part too.

 

[Veracity]

I found the code that stores the record back in the map.

CompositeReference.java:

			Value result = this.slice.aref( this.index, interpreter );

			if ( result == null )
			{
				result = this.slice.initialValue( this.index );

				// If the result is a composite, store it back,
				// since if the user modifies it, we'll want to
				// get the modified version back again.
				if ( result instanceof CompositeValue )
				{
					this.slice.aset( this.index, result, interpreter );
				}
			}

That comment says this is intentional behavior. Now I need to remember what I was thinking.
Maybe I was wrong!

 

[Veracity]

I think I was thinking unclearly. Perhaps I wrote that before we had a "contains" operator to let you know if an element was in a map. In any case, if you fetch an index that is not present, modify the resulting record, and then expect the modified record to be present in the map without you having inserted it, I think your program is buggy.

Revision 10493 no longer inserts an empty record into a map if you fetch an index that is not present.
We will find out, soon enough, how many "buggy programs" depended on the old behavior.

This script:

record my_type {
    int num;
    string str;
};

string pmt( string tag, my_type rec )
{
    return tag + ": num = " + rec.num + " str = " + rec.str;
}

void main()
{
    my_type rvar;
    rvar.num=3; rvar.str='foo';

    my_type rvar2;
    rvar2.num=8;rvar2.str='bar';

    my_type [int] map1;
    map1[2] = rvar;
    map1[5] = rvar2;

    print( count( map1 ) + " " + map1 contains 2 + " " + pmt( "", map1[2] ) );
    foreach key in map1 {
	print( pmt( "key = " + key, map1[key] ) );
    }

    print( "remove" + pmt( "", remove map1[2] ) );
    foreach key in map1 {
	print( pmt( "key = " + key, map1[key] ) );
    }

    print( count( map1 ) + " " + map1 contains 2 + " " + pmt( "", map1[2] ) );
    foreach key in map1 {
	print( pmt( "key = " + key, map1[key] ) );
    }
}

yields the following result:

> mmm

2 true : num = 3 str = foo
key = 2: num = 3 str = foo
key = 5: num = 8 str = bar
remove: num = 3 str = foo
key = 5: num = 8 str = bar
1 false : num = 0 str =
key = 5: num = 8 str = bar

 

[QVamp]

Groovy. Thanks for doing this.