Issue 128585 - Make the "IF" function compatible with ODF 1.3
Summary: Make the "IF" function compatible with ODF 1.3
Status: CONFIRMED
Alias: None
Product: Calc
Classification: Application
Component: code (show other issues)
Version: 4.2.0-dev
Hardware: All All
: P5 (lowest) Normal (vote)
Target Milestone: ---
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 128282
  Show dependency tree
 
Reported: 2024-01-18 18:55 UTC by damjan
Modified: 2024-02-02 06:35 UTC (History)
0 users

See Also:
Issue Type: DEFECT
Latest Confirmation in: 4.2.0-dev
Developer Difficulty: ---


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description damjan 2024-01-18 18:55:21 UTC
I am creating this issue to track a harder kind of change to our OpenFormula code, needed for ODF 1.3 compatibility.

On AOO:

                                |          Condition
IF formula                      |TRUE           | FALSE
--------------------------------+---------------+------
IF(condition)			|TRUE		|FALSE
IF(condition;)			|Err:511	|Err:511
IF(condition;IfTrue)		|IfTrue		|FALSE
IF(condition;;)			|Err:511	|Err:511
IF(condition;;IfFalse)		|Err:518	|IfFalse
IF(condition;IfTrue;)		|Err:511	|Err:511
IF(condition;IfTrue;IfFalse)	|IfTrue		|IfFalse


On LO, and as per the ODF 1.3 requirements:

                                |          Condition
IF formula                      |TRUE           | FALSE
--------------------------------+---------------+------
IF(condition)			|TRUE		|FALSE
IF(condition;)			|0		|FALSE
IF(condition;IfTrue)		|IfTrue		|FALSE
IF(condition;;)			|0		|0
IF(condition;;IfFalse)		|0		|IfFalse
IF(condition;IfTrue;)		|IfTrue		|0
IF(condition;IfTrue;IfFalse)	|IfTrue		|IfFalse
Comment 1 damjan 2024-01-18 18:59:10 UTC
The calc method implementing the "IF" function is in ScInterpreter::ScIfJump() in file main/sc/source/core/tool/interpr1.cxx.

However setting a breakpoint on this method, and then trying to type an "=IF(...)" formula, results in the breakpoint getting triggered when the formula is something AOO currently sees as valid, such as =IF(condition) or =IF(condition;IfTrue). When typing something that AOO currently doesn't see as valid, such as =IF(condition;) or =IF(condition;;), the breakpoint is NOT triggered.

This means we need to change the "IF" function somewhere else, perhaps earlier on in the code, when the formula is getting parsed.
Comment 2 damjan 2024-01-19 15:35:55 UTC
Formula parsing seems to work in stages:

In main/sc/source/ui/view/viewfunc.cxx, method ScViewFunc::EnterData() calls ScCompiler::CompileString(), which lexes "=IF(TRUE;)" (the 2nd line in my original post) into:
[ocIf, ocOpen, ocPush, ocSep, ocClose]

This is then passed to ScCompiler::CompileTokenArray() which is implemented in its parent class, FormulaCompiler::CompileTokenArray(), in file main/formula/source/core/api/FormulaCompiler.cxx. This then parses the lexed tokens into the correct structure for the formulas.

Eventually that somehow arrives in ScInterpreter::ScIfJump() from file main/sc/source/core/tool/interpr1.cxx for actual execution.

The problem happens in FormulaCompiler::Factor(),
in the "else if (eOp == ocIf || eOp == ocChose)" case,
where it expects to find an expression for the 2nd parameter, but gets ocSep (";") instead. This generates an error during the call to Expression().

So the lexing is definitely correct, but we need to patch the parsing and possibly the execution.
Comment 3 damjan 2024-02-01 17:12:03 UTC
Some further notes on how the lexing happens:

IF(c)
[ocIf, ocOpen, ocPush, ocClose]

IF(c;)
[ocIf, ocOpen, ocPush, ocSep, ocClose]

IF(c;IfTrue)
[ocIf, ocOpen, ocPush, ocSep, ocPush, ocClose]

IF(c;;)
[ocIf, ocOpen, ocPush, ocSep, ocMissing, ocSep, ocClose]

IF(c;;IfFalse)
[ocIf, ocOpen, ocPush, ocSep, ocMissing, ocSep, ocPush, ocClose]

IF(c;IfTrue;)
[ocIf, ocOpen, ocPush, ocSep, ocPush, ocSep, ocClose]

IF(c;IfTrue;IfFalse)
[ocIf, ocOpen, ocPush, ocSep, ocPush, ocSep, ocPush, ocClose]


So it appears that:
IF => ocIf
(  => ocOpen
values => ocPush
;  => ocSep
)  => ocClose
2 x adjacent ocSep also get ocMissing between them
Comment 4 damjan 2024-02-02 06:35:20 UTC
In light of the fact that it's Expression() that fails, we can avoid calling it for ocClose with this kind of patch:

---snip---
diff --git a/main/formula/source/core/api/FormulaCompiler.cxx b/main/formula/source/core/api/FormulaCompiler.cxx
index d5554e30c0..b1d7384d2c 100644
--- a/main/formula/source/core/api/FormulaCompiler.cxx
+++ b/main/formula/source/core/api/FormulaCompiler.cxx
@@ -1096,8 +1096,9 @@ void FormulaCompiler::Factor()
             {
                 if ( ++nJumpCount <= nJumpMax )
                     pFacToken->GetJump()[nJumpCount] = pc-1;
-                NextToken();
-                eOp = Expression();
+                eOp = NextToken();
+                if (eOp != ocClose)
+                    eOp = Expression();
                 // ocSep or ocClose terminate the subexpression
                 PutCode( pToken );
             }
---snip---

and that gets these results:

                                |          Condition
IF formula                      |TRUE           | FALSE
--------------------------------+---------------+------
IF(condition)			|TRUE		|FALSE
IF(condition;)			|#NULL!         |FALSE
IF(condition;IfTrue)		|IfTrue		|FALSE
IF(condition;;)			|Err:518	|#NULL!
IF(condition;;IfFalse)		|Err:518	|IfFalse
IF(condition;IfTrue;)		|IfTrue 	|#NULL!
IF(condition;IfTrue;IfFalse)	|IfTrue		|IfFalse

which is slightly better, but still needs further fixes.

It also gets:
=CHOICE(1;2;)
to show "2" instead of "Err:511", as the same code is used to parse ocIf and ocChoice. Google Sheets and LO also display "2".

However I also note that for Google Sheets and LO:
=OR(TRUE;) returns TRUE
=AND(TRUE;) returns TRUE
but for us, those are Err:511.

Thus Expression() needs to be fixed for other tokens too, not just ocIf and ocChoice.