Bug - Fixed Foreach Anomalous Behavior

[Roger Ramjet]

The following code demonstrates what appears to be inconsistencies in the treatment of the foreach statement in ASH. The code was verified and run in r10016 and r10058, and shows no errors.

(Note that this code is designed to demonstrate certain specific issues, not to actually perform any work, although it is derived from functional code).

// When running this code change the three if statements
// "if (!have_skill($skill[pulverize]))" to be evaluate to true for the
// skill set of the character (i.e., leave or remove the "!").

void A()
{
// If the line "item x;" is present, a verify error occurs.
// Key variable 'x' is already defined (foreach test.ash, line 7)
//	item x;
	foreach x in $items[heavy D, original G]
	{	print("A item x = " + x);
	}
	return;
}

void B()
{
// Note the presence of "item y;" here but there is no verify error for
// the foreach statement.
	item y;
	if (!have_skill($skill[pulverize]))
	{	foreach y in $items[heavy D, original G]
		{	print("B item y = " + y);
		}
	}
	return;
}

void C()
{
// Note no declaration of "item z".
	if (!have_skill($skill[pulverize]))
	{	foreach z in $items[heavy D, original G]
		{	print("C item h = " + z);
		}
	}
	return;
}

void D()
{
	item i;
	int  j;
	i = $item[maiden wig];
	j = 5;
	print("D before foreach item i = " + i);
	print("D before foreach int j = " + j);
	if (!have_skill($skill[pulverize]))
	{	foreach i in $items[heavy D, original G]
		{	print("D in foreach item i = " + i);
			print("D in foreach int j = " + j);
		}
	}
	print("D after foreach item i = " + i);
	print("D after foreach int j = " + j);
	return;
}

void main()
{
	A();
	B();
	C();
	D();
}

There are three issues demonstrated by this code ...

Issue 1:

First, look at routine A. If the line "item x;" is commented-out (as shown), this verifies fine. If that line is not commented-out, a verify produces an error; "Key variable 'x' is already defined (foreach test.ash, line 7)", apparently due to the redeclaration of "x" in the foreach statement.

Next look at routine B. It is effectively the same as routine A, with the line "item y;" included, but with the foreach statement within the bounds of the if statement. This verifies as correct, despite the declaration of "y".

Why does routine A produce an error but routine B does not produce one? If the foreach in routine B self-declares its key, should not this cause a error? If the foreach is not self-declaring its key, then why does routine A produce an error if "item x;" is included? This is not consistent behavior in the compiler. The situation appears to depend on whether or not the foreach is at the top level of a routine or not.

Issue 2:

Look at routines B and C. They are effectively the same, except routine B includes a declaration for the key in the foreach statement (the line "item y;"). As noted in Issue 1, this would, if consistent with routine A, produce an error; but it does not.

It would appear, at first glance, that ASH does not care if a foreach key is also declared within a routine, as long as the foreach is not in the highest scope of the routine with the variable declaration.

This, however, allows the anomalous behavior shown in routine D. There, "item i" is declared at the routine level and used as the foreach key. Executing the script produces the following output for routine D:
D before foreach item i = maiden wig
D before foreach int j = 5
D in foreach item i = heavy D
D in foreach int j = 5
D in foreach item i = original G
D in foreach int j = 5
D after foreach item i = maiden wig
D after foreach int j = 5

Clearly there are two separate variables "i" active within routine D. One in most of the routine, and one which is limited to the scope of the foreach statement. Further, based on the results of routine A, this changing scope only appears to happen when foreach statements are within the scope of other control statements, giving the foreach statement different behaviors at different points in code. This is not the way that most programmers would expect code to work, and is something which will produce hard to debug errors.

Issue 3:

To those who write the ASH documentation and tutorials ...

No where that I have seen (and I read everything I could find) did anyone mention that the ASH foreach statement self-declares its key variable and that the key is defined exclusively within the scope of its foreach statement. To C# programmers this may seem normal (except that C# requires a redundant type identifier within its foreach key); however, this action is not typical of many other programming languages. It should be specifically mentioned in the documentation.

 

[slyz]

I haven't spend much time looking at the ASH interpreter source, but it seems like this is a scope issue. If the variable used by foreach already exists in foreach's "parent scope", you get the "variable already defined" error.

The situation appears to depend on whether or not the foreach is at the top level of a routine or not.

Not exaclty. This:

void B()
{	
	if ( true )
	{
		item y;
		foreach y in $items[ heavy D, original G ]
		{
			print( "B item y = " + y );
		}
	}
	return;
}

also causes a "variable already defined" error.

Again, I don't understand everything in the source (Parser.java), but it looks like a new scope is created when "if{}" is used, and that's the scope in which foreach will check if its variable has already been defined.

On the other hand, this doesn't happen when using simply "if". You can check it by defining B() like this:

void B()
{
// Note the presence of "item y;" here but there is no verify error for
// the foreach statement.
	item y;
	if ( true )
		foreach y in $items[ heavy D, original G ]
		{
			print( "B item y = " + y );
		}
	return;
}

which causes a "Key variable 'y' is already defined" error.

It looks like the problem is actually if{} versus if:

void E()
{
	item a = $item[ heavy D ];
	print( "E before if item a = " + a );
	if ( true )
	{
		item a = $item[ original G ];
		print( "E during if item a = " + a );
	}
	print( "E after if item a = " + a );
}

results in

E before if item a = heavy D
E during if item a = original G
E after if item a = heavy D

If I had to solve this, I would probably try to see if it is possible to initialize the scope of if{} as being its parent scope instead of creating a fresh new scope, but I have no idea what this would break.

 

[StDoodle]

It should be specifically mentioned in the documentation.

The documentation's a wiki, so....

 

[Veracity]

the ASH foreach statement self-declares its key variable and that the key is defined exclusively within the scope of its foreach statement. To C# programmers this may seem normal (except that C# requires a redundant type identifier within its foreach key); however, this action is not typical of many other programming languages.

Think Java.

If I had to solve this, I would probably try to see if it is possible to initialize the scope of if{} as being its parent scope instead of creating a fresh new scope, but I have no idea what this would break.

It would make ASH be unlike every other language in which each block is a scope in which you can define variables.

if (x)
    statement;

is SUPPOSED to be identical to

if (x)
{
    statement;
}

In other words, for things like if that take a scope, the scope can be either a block of statements inside curly braces or a single statement. 'if' takes a scope. So does foreach.

I believe that foreach should define its iterator variables as if they were declared inside its scope. Declaring variables with the same name as an iterator variable, either within the same scope as the foreach itself is at or in an enclosing scope should be allowed; you will mask the outer variables, but, tough.

I will look in detail at this later. I am pretty busy right this second, but it is worth investigating.

 

[Roger Ramjet]

The documentation's a wiki, so....

Some of the documentation is a wiki.

The final issue is just a note for the various authors not to assume that the way ASH does its foreach statement is obvious. I expect that the authors know that is how the ASH foreach works, but simply forgot to clarify it. A strict-type, self-declaring key variable is not that common a way for a foreach statement to act (although is a good idea).

 

[slyz]

I really should look up what a scope is in this context. I thought it was simply the existing variables and functions (the namespace?), but it looks like that's not exactly it.

In any case, the same issue applies to function definitions too:

void test()
{
	print( "testing" );
}

if ( true )
{
	void test()
	{
		print( "testing in if" );
	}

	test();
}

test();

results in

> call test.ash

testing in if
testing

instead of warning about the multiple definitions of test().

This is very low priority, in my opinion, but I find the problem interesting.

 

[jasonharper]

That is not at all a "problem", slyz. Two identical definitions (functions, variables, or whatever) in the same scope does indicate a bug in the code, and therefore generates an error, since one of the definitions would become completely inaccessible if that was allowed. Identical definitions in different scopes are explicitly not a bug, it simply means that the identifier being defined has different meanings in different contexts. One of the scopes being nested inside the other does not change that fact.

 

[slyz]

if (x)
    statement;

is SUPPOSED to be identical to

if (x)
{
    statement;
}

I was referring to the fact that the above wasn't true. I agree it's not really a problem, it's more of an inconsistency that few scripters are going to stumble upon.

 

[Veracity]

I was referring to the fact that the above wasn't true.

The above IS true. Note that a variable declaration is not a "statement".

This bug is an error in checking for duplicate key variables while parsing a foreach statement. It is fixed in revision 100101.