GenieClient / Genie4

.NET 6 Update of Genie
GNU General Public License v3.0
21 stars 14 forks source link

4.0.2.6 - Conditional Statement: not taking true path when 'contains' function is in multi-variant evaluation #145

Open umbravi opened 1 year ago

umbravi commented 1 year ago

I just picked up Genie4 for the first time after being away from the game for a few years. A chunk of my scripts aren't working correctly because the 'if' conditional isn't evaluating correctly when there are more than one evaluation in a statement AND the contains function is one of the evaluations.

When 'contains' (or '!contains') function is part of a multi-variant evaluation, the 'if' conditional is not correctly evaluating.

Given script names "test.cmd":

debug 10

if (true && true) then echo true
if contains("put|Put|get|Get", "%1") then echo 1 is put or get
if "%2" == "" then echo 2 is empty
if (contains("put|Put|get|Get", "%1") || "%2" == "") then echo 1 should be put or get or 2 should be empty
if (contains("put", "%1") && "%2" == "") then echo 1 should be put and 2 should be empty
if (!contains("inverse", "%1")) then echo 1 is not inverse
if (!contains("inverse multi", "%1") && "%2" == "") then echo 1 should not be inverse multi and 2 should be empty

Exit:
exit

And command: .test put

output is:

test.cmd(5): [if (true && true) then]
test.cmd(5): [echo true]
true
test.cmd(6): [if contains("put|Put|get|Get", "put") then]
test.cmd(6): [echo 1 is put or get]
1 is put or get
test.cmd(7): [if "" == "" then]
test.cmd(7): [echo 2 is empty]
2 is empty
test.cmd(8): [if (contains("put|Put|get|Get", "put") || "" == "") then]
test.cmd(9): [if (contains("put", "put") && "" == "") then]
test.cmd(10): [if (!contains("inverse", "put")) then]
test.cmd(10): [echo 1 is not inverse]
1 is not inverse
test.cmd(11): [if (!contains("inverse multi", "put") && "" == "") then]
test.cmd(13): [Exit:]
test.cmd(14): [exit]
mj-colonel-panic commented 12 months ago

Sorry for the lack of discussion on this. I looked into this shortly after you opened the issue and was unable to spot what was going on. I've continued looking into it and today I believe I've found the problem. I'm documenting what I've found here.

Relevant code is in the class Genie.Script.Eval

The process that occurs when evaluating statements is to break them into sections and parse each section This is done by putting each segment of the statement into a queue and processing it in sections For example, contains("put|Put|get|Get", "put") breaks into 4 segments, an array like ["(", "put|Put|get|Get", "put", ")"] The segments are objects though, not strings, and contains flags of their own. This is a boon, as each has a flag to indicate whether it has been parsed. That will come into play for my proposed fix. As each segment is parsed, it flags that segment as having been parsed.

The function ParseQueue is used recursively to parse each section. It starts at the base of the array and iterates through. The cause of this bug seems to be in the logic here.

One way a section is identified is by using parentheses. In the enum these are ParseType.SectionStartType and ParseType.SectionEndType.

When the parser reaches a SectionEndType, it returns. When the Section contains a Function with its own Section, this can cause it to return before fully evaluating the entire section.

The fix I am currently testing - and so far it looks good - is to break instead of return, and depend on it having marked the segment as parsed to prevent it from parsing a second time. I am going to run a script on this for the next hour, as the change is in a significantly highly trafficked area of code. If it maintains the stability that it has as I've written this, and after I've had another programmer take a look and give their thoughts, if they agree with my assessment I will commit this change and put it in the test build.

umbravi commented 11 months ago

I'm glad you found the issue. :-D