Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IXLCell.Value returns incorrect value when using formula that references variable that was not yet added. #1536

Open
2 of 5 tasks
yar-shukan opened this issue Sep 30, 2020 · 3 comments · May be fixed by #1539
Open
2 of 5 tasks

IXLCell.Value returns incorrect value when using formula that references variable that was not yet added. #1536

yar-shukan opened this issue Sep 30, 2020 · 3 comments · May be fixed by #1539
Assignees
@Pankraty
Labels
bug

Comments

@yar-shukan
Copy link

yar-shukan commented Sep 30, 2020 •
edited

NOTE: This same code throws ClosedXML.Excel.CalcEngine.Exceptions.NameNotRecognizedException: 'The identifier variableA was not recognised.' on build from 'develop' branch. I think this was changed (is that now as designed?) in this commit:
f2063a7

Do you want to request a feature or report a bug?

  • Bug
  • Feature
  • Question

Did you test against the latest CI build?

  • Yes
  • No

I used the latest officially released Nuget package v = 0.95.3

If you answered No, please test with the latest development build first.

Version of ClosedXML

Nuget package v = 0.95.3

What is the current behavior?

In the current released 0.95.3 version we have an issue that if formula has usage of named variable and we access .Value before this named variable is added to the worksheet and we try to access formulaCell.Value after we added named variable to worksheet we get .Value incorrectly as old value. Example

var wb = new XLWorkbook();
var ws = wb.Worksheets.Add("Formulas");

IXLCell formulaCell = ws.Cell(1, 1);
formulaCell.FormulaA1 = $"=variableA*5";

Console.WriteLine($"Expected = 0, Actual = {formulaCell.Value}"); //gets 0

ws.Cell(1, 2).Value = "5";
ws.Cell(1, 2).AddToNamed("variableA");

formulaCell.InvalidateFormula();
ws.RecalculateAllFormulas();

Console.WriteLine($"Expected = 25, Actual = {formulaCell.Value}"); //gets 0

Note that this works fine if I comment out first formulaCell.Value access:

var wb = new XLWorkbook();
var ws = wb.Worksheets.Add("Formulas");

IXLCell formulaCell = ws.Cell(1, 1);
formulaCell.FormulaA1 = $"=variableA*5";

//Console.WriteLine($"Expected = 0, Actual = {formulaCell.Value}"); // don't make access here

ws.Cell(1, 2).Value = "5";
ws.Cell(1, 2).AddToNamed("variableA");

formulaCell.InvalidateFormula();
ws.RecalculateAllFormulas();

Console.WriteLine($"Expected = 25, Actual = {formulaCell.Value}"); //gets 25

What is the expected behavior or new feature?

formulaCell.Value is correctly invalidated and results to 25 at the last line.

Is this a regression from the previous version?

No

Reproducibility

Code to reproduce problem:

using ClosedXML.Excel;
using System;

class Program
{
    static void Main(string[] args)
    {
        var wb = new XLWorkbook();
        var ws = wb.Worksheets.Add("Formulas");

        IXLCell formulaCell = ws.Cell(1, 1);
        formulaCell.FormulaA1 = $"=variableA*5";

        Console.WriteLine($"Expected = 0, Actual = {formulaCell.Value}");

        ws.Cell(1, 2).Value = $"5";
        ws.Cell(1, 2).AddToNamed("variableA");

        formulaCell.InvalidateFormula();
        ws.RecalculateAllFormulas();

        Console.WriteLine($"Expected = 25, Actual = {formulaCell.Value}");
    }
}

cc: @igitur
@Pankraty Pankraty added the bug label Oct 5, 2020
@Pankraty Pankraty self-assigned this Oct 6, 2020
@Pankraty
Copy link
Member

Pankraty commented Oct 6, 2020

@igitur, this turned out to be trickier than I expected. First, we have to consider named ranges too when analyzing precedent cells. But this is not enough - our ExpressionCache caches evaluated expression and refuses to notice that the named range point to a different location than it used to. Do you think we can turn the expression caching off? Parsing has never been a performance bottleneck, as I recall.

@yar-shukan
Copy link
Author

@Pankraty , @igitur I will close this issue as with latest version on develop you can't use this case as when using variable that is not yet added code now throws NameNotRecognizedException and I think this should be expected behavior.

@Pankraty Pankraty reopened this Oct 6, 2020
@Pankraty
Copy link
Member

Pankraty commented Oct 6, 2020

The issue exists nonetheless


        [Test]
        public void ChangingNamedRangeInvalidatesCache()
        {
            using var wb = new XLWorkbook();
            var ws = wb.AddWorksheet("Sheet1");
            var cell1 = ws.FirstCell();
            cell1.FormulaA1 = "NAMED * 10";
            var cell2 = ws.Cell("B1");
            cell2.Value = 5;

            Assert.Throws<NameNotRecognizedException>(() => _ = cell1.Value);

            cell2.AddToNamed("NAMED");
            Assert.AreEqual(50, cell1.Value);

            wb.NamedRanges.NamedRange("NAMED").SetRefersTo("Sheet1!D1");
            Assert.AreEqual(0, cell1.Value);

            wb.NamedRanges.DeleteAll();
            Assert.Throws<NameNotRecognizedException>(() => _ = cell1.Value);

            cell2.AddToNamed("NAMED", XLScope.Worksheet);
            Assert.AreEqual(50, cell1.Value);

            ws.NamedRanges.NamedRange("NAMED").SetRefersTo("Sheet1!D1");
            Assert.AreEqual(0, cell1.Value);

            ws.NamedRanges.DeleteAll();
            Assert.Throws<NameNotRecognizedException>(() => _ = cell1.Value);
        }

@Pankraty Pankraty linked a pull request Oct 6, 2020 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Pankraty Pankraty

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix call calculated value invalidation if named ranges are changed (#1536) Pankraty/ClosedXML
2 participants
@yar-shukan @Pankraty