fix lua null string

[Jampi0n]

replaces null strings with empty strings in lua

Null strings will cause errors when used in lua natives.
Fixes one of the issues reported in #885 .

[Frotty]

Hey, thanks for the lua PRs. Could you add unit tests for them?

[Jampi0n]

How would these unit tests look like?
Most of the unit tests I've seen are wurst programs which are expected to either be valid wurst code or not (testAssertOkLines, testAssertErrorsLines).
In this case the translation to lua needs to be tested.

[Frotty]

Yes, tests output the program states into files which you parse (test-output).
Some already do this, see optimizer correctness tests. Dunno if a lua one exists, but the test should generate a lua output.

[Jampi0n]

I have some test cases, but I had to change WurstScriptTest.java to get lua compilation to work. In line 246.

Is there a reason why lua is only tested if executeProg is true? executeProg is a parameter for translateAndTestLua, so it seems unnecessary to put it in the if. It would also be nice to have withStdLib for lua as well.

I removed executeProg from the if and the tests work now, but I don't want to commit it, because there are probably reasons why it was put like this.

[Frotty]

It seems fine to me since executeProg is handled in the lua compile method.
Stdlib is deactivated, because it didn't (doesn't ?) compile with lua I suppose.
Both of these seem like left overs from early development and can probably be removed, ideally add more tests for this as well.

[Jampi0n]

I changed the if to only check for lua.
Executing lua works fine, though it doesn't work with most natives from the standard library. E.g. InitHashtable or CreateTrigger doesn't work.
Standard library by itself also works.

I added tests to verify that both lua execution and using standard library work.
For the null string issue I added tests for strings and other nullable types to make sure only the string nulls are replaced by "".

Edit: the two tests that fail are the ones which execute lua, so they require a lua installation

[Frotty]

Nice, sounds goo so far.
Should make pipeline passable somehow, Perhaps can use some different docker image?
Or disable running for now.
What do you suggest regarding hashtables

[Jampi0n]

I don't know much about docker, so I guess just remove the tests for now. Should I leave the functions in and just remove the test annotation or remove the function as well?

Are you referring to the hashtable issue in #885 ?
The issue seems to be much more limited than I initially thought. It's not that hashtables return nil, but that toIndex and fromIndex is used in HashMap. The index 0 is never used, so using fromIndex on the 0 default return value of LoadInteger results in nil.
This is only an issue with primitive types, because other types are supposed to be nil if the key does not exist.

If the cast is moved outside from the HashMap.get function to its call, the desired return type is known and nil could be changed to the correct default value. In JASS the cast is also done at the call and different functions are used to cast the integer to the correct type.

[Frotty]

I don't know much about docker, so I guess just remove the tests for now. Should I leave the functions in and just remove the test annotation or remove the function as well?

Don't remove anything, you can mark tests as ignored. What exactly doesn't work tho regarding to lua?

Are you referring to the hashtable issue in #885 ?
The issue seems to be much more limited than I initially thought. It's not that hashtables return nil, but that toIndex and fromIndex is used in HashMap. The index 0 is never used, so using fromIndex on the 0 default return value of LoadInteger results in nil.
This is only an issue with primitive types, because other types are supposed to be nil if the key does not exist.

If the cast is moved outside from the HashMap.get function to its call, the desired return type is known and nil could be changed to the correct default value. In JASS the cast is also done at the call and different functions are used to cast the integer to the correct type.

I don't really get the issue, so dunno.

[Jampi0n]

The error is that it cannot find a lua installation:
java.lang.RuntimeException: java.io.IOException: Cannot run program "lua": error=2, No such file or directory
I had the same error locally until I installed lua.

The hashtable issue is more of a typecasting issue in lua and it affects some other parts as well.

As an example, take the JASS HashMap:
function HashMap_get takes integer this, integer key returns integer
It only deals with integers and when get is called, the arguments are converted to integer and the return type is converted to the correct type:
indexToValueType(HashMap_get(this, keyTypeToIndex(key),...)
The typecast functions depend on the type parameters of this HashMap instance.

In lua, the HashMap function takes and returns values of unkown type and they are cast to integer inside the function:

function HashMap_get(this, key) 
    return objectFromIndex(LoadInteger(this, objectToIndex(key)))
end

The JASS approach has the advantage, that different cast functions can be used depending on the type.
In lua this is not possible, because the casting is inside the function, so it is the same for any type.
In JASS you simply omit casting, if the type is an integer. In lua you always look up the object with objectFromIndex, which may result in nil even if the type is integer.

Currently there are other issues that are also related to typecasting in lua:

String concatenation may not be used, if one of the operands is returned by a function that returns not just strings:

function f()
	let strMap = new HashMap<int,string>()
	strMap.put(0, "some string")
	print("" + strMap.get(0))

The plus operator in the last line is not translated to string concatenation, because the return type of strMap.get is any.
In JASS, the type would be correctly identified by the compiler, because the return value of get would be wrapped inside a stringFromIndex.

Functions with type parameters are not translated correctly (the new commaList implementation does not work in lua because it uses LinkedList.map).

public interface ConversionFunc<T, Q>
	function run(T t) returns Q

class A<T>
	T elem

	construct(T elem)
		this.elem = elem

	function convert<Q>(ConversionFunc<T, Q> itr) returns Q
		return itr.run(elem)

@compiletime function foo()
	let a = new A<int>(4)
	let b = a.convert<string>() (integer t) ->
		return I2S(t)
	DisplayTimedTextToPlayer(GetLocalPlayer(), 0, 0, 45, b)

	let c = new A<string>("9")
	let d = c.convert() (string t) ->
		return S2I(t)
	DisplayTimedTextToPlayer(GetLocalPlayer(), 0, 0, 45, I2S(0 + d))

init
	foo()

In this example, the closure implementations are translated correctly, so the fist closure is just return I2S(t) and the second one is return S2I(t). However, they are wrapped by functions called run_wrapper2 and run_wrapper3, resulting in:
return stringToIndex(I2S(t) and return S2I(stringFromIndex(t)). I don't know why or where these functions were put in the mapscript. In ExprTranslation.java they are explicitly excluded for lua.
With these functions the types are wrong, resulting in an exception at compiletime and wrong values at runtime.

These errors can easily be avoided by changing the wurst code a little bit. So I don't know if it's worth trying to fix them, considering it will probably be a lot of work and the new generics should fix the problems anyway.

[Frotty]

The error is that it cannot find a lua installation:
java.lang.RuntimeException: java.io.IOException: Cannot run program "lua": error=2, No such file or directory
I had the same error locally until I installed lua.

Yeah so this could be fixed with a docker image that has a java and a lua installation.

Regarding the rest, I think this was one of the reasons for the new typeclasses feature and new generics, which wouldn't rely on toIndex/fromIndex hacks anymore.

[Frotty]

LGTM, thanks 👍