Bug - Fixed ASH function return value enforcement is buggy.

[Veracity]

ASH is supposed to detect when you have a non-void function which doesn't return a value of the correct type and give you a compile error.

I see the following code in Parser.parseFunction().

		if ( !result.assertReturn() && !functionType.equals( DataTypes.TYPE_VOID )
		// The following clause can't be correct. I think it
		// depends on the various conditional & loop constructs
		// returning a boolean. Or something. But without it,
		// existing scripts break. Aargh!
		&& !functionType.equals( DataTypes.TYPE_BOOLEAN ) )
		{
			throw this.parseException( "Missing return value" );
		}

So, it intentionally fails to do that for boolean functions (for some unknown reason), but should make that check for non-boolean functions.

However, the following script shows that ASH's return value checking is busted for non-booleans:

int test()
{
	if ( true )
		return 1;
}

This fails validation with "missing return value".

If you declare that a function returns a value, ASH should enforce that you do that - for all data types, not just non-booleans. In order to do that, it will need to understand that certain control constructs can make following statements unreachable and not simply check that the last statement in the function is a "return".

 

[Winterbay]

Verification of functions in ASH-scripts fails in certain cases

As discussed over here (and a few posts on) ash functions with encapsulated retur-values fail to verify even though there is no way that the function would return without a return value.

The example given by Veracity was

int test( int a, int b )
{
	if ( a < b ) {
		return a;
	} else {
		return b;
	}
}

The same effect can be achieved with a switch-statement where this fails:

string poison_level(int toxic)
{
	switch (toxic)
	{
		case 1:	return "haseffect 436;";
				break;
		case 2:	return "haseffect 436 || haseffect 284;";
				break;
		case 3:	return "haseffect 436 || haseffect 284 || haseffect 283;";
				break;
		case 4:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282;";
				break;
		case 5:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282 || haseffect 264;";
				break;
		case 6:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282 || haseffect 264 || haseffect 8;";
				break;
		default: return "";
	}
	//return "";
}

while this doesn't

string poison_level(int toxic)
{
	switch (toxic)
	{
		case 1:	return "haseffect 436;";
				break;
		case 2:	return "haseffect 436 || haseffect 284;";
				break;
		case 3:	return "haseffect 436 || haseffect 284 || haseffect 283;";
				break;
		case 4:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282;";
				break;
		case 5:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282 || haseffect 264;";
				break;
		case 6:	return "haseffect 436 || haseffect 284 || haseffect 283 || haseffect 282 || haseffect 264 || haseffect 8;";
				break;
	}
	return "";
}

It is possible to program around this, but it would probably be better if ASH correctly analysed it.

 

[Winterbay]

Oh, you beat me to it
You can close my thread (or merge it I guess)...

 

[Veracity]

Thanks.

Duplicate. Merging.

 

[jasonharper]

r10727 fixes this (except that any truly broken boolean functions that were being allowed under the previous special exception are generating warnings for now, rather than errors).