Calculation engine executes statements on every branches

[frantzmiccoli]

This is:

- [ ] a bug report
- [x] a feature request
- [x] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

To be honest I am not sure whether it is a feature request or a bug report.

What is the expected behavior?

If we take the example of an IF statement like IF(A1="YES";B12;C12) we could expect that A1 contains the value YES we should only resolve the value in the cell B12.

What is the current behavior?

Currently B12 is resolved and reutrned correctly, though C12 is evaluated too which causes extra, not required, computation

What are the steps to reproduce?

1. Using 1.5.1 and the following file: repro.xlsx

2. Alter the Calculation::calculateCellValue method to just log the name of the resolved cell. Like:

public function calculateCellValue(Cell $pCell = null, $resetLog = true)
    {
        var_dump($pCell->getCoordinate());
     ...

3. Run the following test case:

<?php
class RandomTest extends \PHPUnit\Framework\TestCase {

    public function testIdentifiedProblem() {
        $path = 'model/excel/repro.xlsx';
        $spreadsheet = \PhpOffice\PhpSpreadsheet\IOFactory::load($path);

        $targetSheetName = 'Sheet1';

        $spreadsheet->setActiveSheetIndexByName($targetSheetName);

        $cell = 'B2';
        $value = 'YES';

        $spreadsheet->getActiveSheet()->setCellValue($cell, $value);

        $targetCell = 'B13';
        $targetValue = $spreadsheet->getActiveSheet()->getCell($targetCell)
            ->getCalculatedValue(true);
        $expectedValue = 8;
        $this->assertEquals($expectedValue, $targetValue);
    }

}

4. Observe that both `D11` and `B11` are processed.

Which versions of PhpSpreadsheet and PHP are affected?

PHP 7.2.3-1ubuntu1 (cli) (built: Mar 14 2018 22:03:58) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.3-1ubuntu1, Copyright (c) 1999-2018, by Zend Technologies

PhpSpreadsheet 1.5.1

[MarkBaker]

This is most definitely not a bug report, a bug report would be when the calculation engine returns an incorrect result for a formula (eg if C12 was returned in your example rather than B12).

PHPSPreadsheet uses a simplistic stack-based parser for the calculation engine, which evaluates all the data necessary for a function (all the arguments) onto the stack before executing the function itself. For most functions, this is more efficient than alternative approaches, because the arguments for most functions are pre-determined and required; and it is only a few functions like the IF() functions where it isn't efficient.

I have done some prototyping work on alternative approaches to evaluating formulae and that would make the calculation engine more manageable, it is a non-trivial piece of work, although it would have other benefits such as better handling for errors, the ability to handle array formulae (even 3d arrays), and support for row and column references (not simply cell and range references). However, this is still very much an experimental prototype, and will take a lot of time and effort before it's stable enough to start integrating into PHPSpreadsheet itself.

[frantzmiccoli]

@MarkBaker I have implemented something keeping the stack based parser and tagging tokens to enable pruning. #818

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[frantzmiccoli]

Not stale! ;)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[frantzmiccoli]

Nope, mean bot! Not stale!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[frantzmiccoli]

Not stale

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[frantzmiccoli]

Not stale, pending review dear bot!

[frantzmiccoli]

Resolved through 0b387e7