Project

General

Profile

Feature #3006

Parsing "else if" as "elseif"

Added by Stanislav Zhukov about 2 years ago. Updated 15 days ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
Scripting
Target version:
Start date:
11/10/2015
% Done:

0%

Severity:
Normal

Description

Some mods using this "feature".

History

#1 Updated by Raphaël Lerbour about 2 years ago

I think you mean "else if" as "elseif"? Because "ifelse" makes no sense...

I got an error from OpenMW about this exact situation in a script of mine. I was surprised it could even compile with TESCS, and concluded it was a bug of Morrowind's parser and the script did not work as intended (it was a very minor one). However I did not actually test if it did, rather I just fixed my script right away :)

#2 Updated by Stanislav Zhukov about 2 years ago

  • Subject changed from Parsing "if else" as "ifelse" to Parsing "elseif" as "elseif"

yep you right :D.

#3 Updated by Stanislav Zhukov about 2 years ago

  • Subject changed from Parsing "elseif" as "elseif" to Parsing "else if" as "elseif"

#4 Updated by Marc Zinnschlag about 2 years ago

Could you give me an example where this causes problems? Is there an issue with the matching endif?

#5 Updated by Stanislav Zhukov about 2 years ago

Example plugin: http://mwmodders.com/farmermod.html - fm_slavescript.
Russian translate (original plugin may not contain this issue): http://www.fullrest.ru/files/farmer-mod/files?fid=190 - fm_slavescript.

#6 Updated by Marc Zinnschlag about 2 years ago

I see. The problem here is indeed a missing ENDIF. Need to think about. Implementing a workaround risks breaking a lot of other stuff.

#7 Updated by Marc Zinnschlag about 2 years ago

  • Assignee set to Marc Zinnschlag

#8 Updated by Marc Zinnschlag almost 2 years ago

  • Status changed from New to In Progress

#9 Updated by Marc Zinnschlag almost 2 years ago

Looks like a potential workaround might interfere with another workaround related to else.

For clarification: Are we absolutely sure that else if is the same as an elseif? From my understanding of the original compiler I would expect that a code like

If condition1
something1
Else if condition2
something2
Endif

would execute something1 if condition1 is true and something2 if condition1 is false, while condition2 is ignored.

In other words, we know that the else if construct compiles in vanilla, but does it work as intended?

#10 Updated by Marc Zinnschlag almost 2 years ago

  • Status changed from In Progress to Feedback needed

#11 Updated by Raphaël Lerbour almost 2 years ago

I just tested this with the old version of my script (using "else if").

You guessed right!

Actually if you do :

If condition1
something1
Else if condition2
something2
Else if condition3
something3
Endif

Then something3 will never execute. something2 will always execute as long as condition1 is false.

Compiled code looks weird too, condition2 and condition3 do not appear in it, although something1 and something2 do.

#12 Updated by Raphaël Lerbour almost 2 years ago

Sorry I meant "although something2 and something3 do". (something1 and condition1 appear first, obviously)

#13 Updated by Marc Zinnschlag almost 2 years ago

I didn't think of the possibility of multiple if else statements. That makes things even worse.

#14 Updated by Marc Zinnschlag almost 2 years ago

  • Status changed from Feedback needed to Confirmed

#15 Updated by Marc Zinnschlag almost 2 years ago

  • Target version changed from openmw-0.38 to openmw-0.39

#16 Updated by Anon Tahoe over 1 year ago

iirc openmw just stops executing the function in which "else if" occurs when it runs into that expression. the openmw-cs debugger might say "missing endif" but that's misleading.
as a temporary workaround mods can be fixed with openmw-cs or by turning "else if" into "elseif " in a hex editor.

#17 Updated by Marc Zinnschlag over 1 year ago

  • Target version changed from openmw-0.39 to openmw-0.40

#18 Updated by Marc Zinnschlag over 1 year ago

  • Target version changed from openmw-0.40 to openmw-0.41

#19 Updated by Marc Zinnschlag 12 months ago

  • Target version changed from openmw-0.41 to openmw-0.42

#20 Updated by Marc Zinnschlag 7 months ago

  • Target version changed from openmw-0.42 to openmw-0.43

#21 Updated by Anton Uramer 6 months ago

I've run some tests to make this thing more clear
1.

if ( 0 )
  MessageBox "if" 
else if(1)
    MessageBox "else if" 
  MessageBox "else?" 
endif

doesn't compile
2.
if ( 0 )
  MessageBox "if" 
else if(1)
    MessageBox "else if" 
  endif
  MessageBox "else?" 
endif

compiles but messages nothing
3.
if ( 0 )
  MessageBox "if" 
else
  if(1)
    MessageBox "else if" 
  endif
  MessageBox "else?" 
endif

messages "else?" and "else if"
4.
if ( 0 )
  MessageBox "if" 
elseif(1)
    MessageBox "else if" 
else
  MessageBox "else?" 
endif

messages "else if"

first goal is to make 2. message both "else if" and "else?", which can be done either by adding some kind of hack Scanner::scanSpecial or changing the way IfElseJunkState works (possibly just removing it and straight up using IfElseBodyState).
second goal is to handle 1: in this particular case endif should also end the parent if as well. This can be done either by changing how keywords are handled and making "else if" a keyword which "doubles" next endif. main problem is keeping both 1. and 2. functional. One way is to ignore up to 1 excessive endif for each "else if" keyword used. Also, it can be done by simply replacing every "else if" with "else2if" or any other string, really, before parsing the script, so that you don't have to implement multi-word keywords. Should I try to implement something along these lines, would such a pull request be accepted?

#22 Updated by Marc Zinnschlag 6 months ago

Difficult. I was about to reject this issue during the next cleanup, because it doesn't seem to be a common issue and any fix is seriously at risk of breaking other workarounds. We shouldn't go overboard with all these hacks anyway, since we need to disable them for post-1.0 scripts.

If you want to give it a go anyway, that is okay with me. But please avoid any restructuring or fundamental changes to the structure of the compiler.

#23 Updated by Anton Uramer 6 months ago

it breaks some rather significant scripts
So maybe it's better to go with a pre-compiler phase?
just straight up replace

else if ...
...
endif

with
else
  if...
...
  endif
endif

as a string, and not bother with doing all the fancy mState stuff?
I have a solution in works, but it already involves 4 states and some minor changes to how states operate overall, so not very nice

#24 Updated by Marc Zinnschlag 6 months ago

Not a fan of the precompiler idea. That's just too much overkill.

I just had a quick look at the code. At least the case 1 & 2 part doesn't look that complicated. Can't we just listen for an if when in IfElseJunkState and then continue with elseif?

#25 Updated by Anton Uramer 6 months ago

Ideally, we need both 2 and 3 working, else if = elseif approach will not do that, sadly

#26 Updated by Anton Uramer 6 months ago

also, 1 good thing about "pre-compiling" is that it's very easy to make it optional

#27 Updated by Marc Zinnschlag 5 months ago

3 is already working, right?

And a clear no to the precompiler. That will just create all kinds of problems. On the top of my head, what about debugging? The compiler will now see a code that is different from what the developers sees, which will cause issues with debugging tools or just plain error reporting.

Ideally, we need both 2 and 3 working

Actually, no. Ideally we would replicate the defective behaviour of vanilla. Because fixing this to work properly might actually break existing scripts. Since vanilla does not execute some of the erroneous else clauses these contain code that has never been tested and might cause further problems.
We had a similar problem in Morrowind.esm. By fixing a script problem (and therefore re-activating a piece of script that usually was not executed in vanilla) we broke the main quest.

#28 Updated by Anton Uramer 5 months ago

Ok, I've done the same tests in vanilla (using vanilla TESCS as well)
1.

if ( 0 )
MessageBox "if"
else if(1)
MessageBox "else if"
MessageBox "else?"
endif

messages "else?" and "else if"
2.
if ( 0 )
MessageBox "if"
else if(1)
MessageBox "else if"
endif
MessageBox "else?"
endif

messages "else?" and "else if"
3.
if ( 0 )
MessageBox "if"
else
if(1)
MessageBox "else if"
endif
MessageBox "else?"
endif

messages "else?" and "else if"
4.
if ( 0 )
MessageBox "if"
elseif(1)
MessageBox "else if"
else
MessageBox "else?"
endif

messages "else if"

so to support Vanilla behaviour, we need all 1,2 and 3 working, as I said.

#29 Updated by Marc Zinnschlag 5 months ago

Case 1 and 2 should be fixable in the way I described above.

And case 3 should be working already.

Out of curiosity, did you try case 2 with a 1 in the first if statement? I am wondering if that would make a change in vanilla.

#30 Updated by Anton Uramer 5 months ago

the problem is that we have to differentiate between 1 and 2 as well, and that can only be done by backtracking while in different states (after you found a second endif, and there isn't really any way to tell, which state you are in that point), which current compiler design doesn't support.

#31 Updated by Marc Zinnschlag 5 months ago

Sorry, I don't get it.

Case 1: Else and if are merged into an elseif. The 2nd if clause is executed and done

Case 2: Else and if are merged into an elseif. The 2nd if clause is executed then we hit an endif that ends the whole if-elseif statement. We execute the last message box and then we have a hanging endif, that we can ignore (printing a warning).

#32 Updated by Anton Uramer 5 months ago

look at the difference between 1 and 2
if we just merge "else if" into elseif, we can't parse stuff like 2 properly, as we would just stop at the first endif, and then throw an exception when the second one is met, moreover, just ignoring that exception means we also break the logic: everything between 2 endifs is not in any if at all, so always gets executed, and not when parent if is true

#33 Updated by Marc Zinnschlag 5 months ago

A hanging endif is not an error. It merely prints a warning. Plenty of cases where that happens (even in the 3 default esms), most of them unrelated to else if problems. So far that has broken nothing.

#34 Updated by Anton Uramer 5 months ago

Ive tested

if ( 1 )
MessageBox "if" 
else if(1)
MessageBox "else if" 
endif
MessageBox "else?" 
endif

in vanilla, it prints "if" and "else?"
it seems you are right, vanilla just ignores the second endif as well, and treats everything after first endif as if it was outside the parent if as well.
time to code this stuff then :)

#35 Updated by Anton Uramer 5 months ago

by the way, does anybody know if

        else if (code==Scanner::S_open && mState==IfElseJunkState)
        {
            SkipParser skip (getErrorHandler(), getContext());
            scanner.scan (skip);
            mState = IfElseBodyState;
            return true;
        }

this hack is necessary? what is it fixing?

#37 Updated by Marc Zinnschlag 5 months ago

Will review the PR as soon as I am not backed up that badly anymore (either later today or tomorrow).

Regarding the code you posted: This is the commit message:

ignore conditions after an else (only works if condition is put in parentheses)

I am not entirely sure why the special case is necessary, but it is probably best to just leave it alone.

#38 Updated by Alexei Dobrohotov 3 months ago

  • Tracker changed from Bug to Feature
  • Status changed from Confirmed to New

#39 Updated by scrawl . 2 months ago

  • Target version changed from openmw-0.43 to openmw-1.0

#40 Updated by Alexei Dobrohotov 15 days ago

  • Assignee deleted (Marc Zinnschlag)

Also available in: Atom PDF