Fix evaluation of #elif <expr> inside false context

[puetzk]

See discussion at https://groups.google.com/g/innosetup/c/lhlU3LgB0IY/m/gFRmoPG9BAAJ

This PR is untested (not even compiled) because I do not have access to Embarcadero Delphi (and my employer does not qualify for the Community license). I apologize for that, but I think I understand what's wrong, and sharing what I think is the right fix is hopefully more helpful than sitting on it. Some examples from that thread of code that currently gives an "Undefined identifier" preprocessing error that this should fix, are:

#if 0
  #if 1
  #elif NOEXIST
  #endif
#endif

#if 0
  #ifndef BAZ
    #define BAZ "bar"
  #elif BAZ == "foo"
  #endif
#endif

or the original example

#define FOO = "foo"
#if FOO == "foo"
#elif FOO == "baz"
  #ifndef BAZ
    #define BAZ = NULL
  #elif BAZ == "foo"
  #endif
#endif

All share the symptom that it's an #elif that unexpectedly evaluates, and as of https://groups.google.com/g/innosetup/c/lhlU3LgB0IY/m/y7nrMArABAAJI think I've figured out why; it's the or not FStack.Last.BlockState at

https://github.com/jrsoftware/issrc/blob/bd96b87f5edc0317134e3d5c0b9ed321e3f01015/Projects/ISPP/IsppPreprocessor.pas#L360-L366

I think this is here because FStack.Last.BlockState=False will make FStack.Include false, and yet we want to evaluate #elif when it's the preceding #if was false. But combining in this way does not distinguish whether the False came from the Last.BlockState (which should still consider the #elif alternative) or a parent scope (which should skip all of the alternatives, without even evaluating them).

What we really want to ask here is whether all but the last FStack.Include[*].BlockState are true, ignoring FStack.Last.BlockState (if it was true we'll have FStack.Last.Fired, if false we should evalute this alternative). Which is not quite the same thing.

So if I remembered my pascal syntax correctly from 30 years ago (classic MacOS days), this might be a correct fix :-).

[CLAassistant]

All committers have signed the CLA.

[martijnlaan]

Thanks. I'll look into this a bit later and try to confirm this is a correct fix. (I'm not the original author of ISPP).

[puetzk]

Thanks. The CLA is new since the last PR I did (#286), so I have to submit the new wording to our corporate OSS contributions review. I don't expect it will to be any problem: I see nothing in there out of the ordinary and they've approved similar things for many projects. But it'll probably be next week before I hear back on it.

Sorry I didn't have that lined up in advance, I try to have that stuff ready to go before I open PRs. But the CLA wasn't mentioned in CONTRIBUTING.md (hint, hint), so I didn't know I needed to do new paperwork.

[martijnlaan]

... the CLA wasn't mentioned in CONTRIBUTING.md (hint, hint), so I didn't know I needed to do new paperwork.

Sorry. It's mentioned now.

[puetzk]

CLA is now signed. Sorry about the delay.

[martijnlaan]

I'm closing this because it's untested and dont have the time to properly test it myself, nor did I write the original ISPP code so it's hard to do a proper code review. Sorry.

[puetzk]

Understood. Would you accept a resubmission if I was able to purchase a Delphi license, test the fix against the problematic cases (above, and in the thread) myself, and tell you it worked? This bug bites us often enough I suspect I can get that approved, though I admit to selfishly trying the "what can I do by myself?" way first before engaging with corporate politics :-)

[martijnlaan]

If all cases are tested (using some kind of test script which I could run myself?) and not just the problematic ones that would certainly help. I can also create a test build of this PR if you want so you could do these tests without having to purchse a Delphi license.

[puetzk]

Compiling a test build would be quite helpful if you're willing (and hopefully I didn't make too many typos...). If you could do that I'm willing to spend some more time on creating test-cases for various combinations of nesting/short-circuiting #if expressions, probably involving some degree of (ab)using #define/#sub so expansion leaves a clear trace of whether a given expressions got evaluated or not.

As far as a test script you could run yourself, probably just as a bunch of small .iss files which should preprocess successfully (some of which won't in the current release), and corresponding expected results (e.g. using SaveToFile)? Or at least that's my current best idea on how to do a test script for ISPP. As I know iscc.exe has no way to script "preprocess only", so this will presumably make a bunch of installers too. Kind of heavy, but whatever. If you'd prefer some other format for ISPP test cases, feel free to point me at something (I don't see anything that looks like unit tests in issrc to follow the pattern of).

[puetzk]

I should add - no hurry at all, it's a chronic pain point but one where we've been working around it for months already. And I probably won't be able to get this until to it for at least a week myself.

[martijnlaan]

It didn't compile since the Include prototype change was only done in the implementation section but I did it in the interface section as well and this is the resulting (unsigned) build: https://files.jrsoftware.org/is/dev/innosetup-6.2.2-puetzk-elif.exe

[puetzk]

Well, I have a fair number of test cases written, and this did fix the issue it was trying to, but I also found a regression. I missed the cache logic https://github.com/jrsoftware/issrc/blob/2c7ac38ab42297569dced13ea22a31104ac69ecc/Projects/ISPP/IsppPreprocessor.pas#L1253-L1254

which doesn't account for SkipLastBlock, and so can give cached answers that disregard it. I have an proposal to fix this that I think actually makes the caching both simpler and more effective (less invalidation), if you would reopen the PR so I can push updates (and would be willing to then make a second test build...).

[puetzk]

Update pushed to https://github.com/puetzk/issrc/compare/elif...elif2 for now

[KngStr]

I can't access google group. But if you guys are looking for this #pragma parseroption -u+ ? u means Allow undeclared identifiers. If an undefined identifier is encountered, ISPP will raise an error unless this option is turned on, in which case a standalone identifier (the one that does not look like a function call) will be considered void value. Default state: off.

All Parser options

Parser options
b Short-circuit boolean evaluation. Default state: on. 
m Short-circuit multiplication evaluation. (In "0 * A", A will not be evaluated, since the result of expression is known to be zero.) Default state: off. 
p Pascal-style string literals. In off state, uses C-style string literals (with escape sequences). Default state: on. 
u Allow undeclared identifiers. If an undefined identifier is encountered, ISPP will raise an error unless this option is turned on, in which case a standalone identifier (the one that does not look like a function call) will be considered void value. Default state: off.