rubysolo/dentaku

Case sensitive BulkExpressionSolver#solve fails for capitalized keys declared in imperfect order

brandoncc opened this issue · 1 comments

Hello, and thanks for this awesome library!

I've built a system which uses primarily capitalized tokens in expressions. These expressions can reference other tokens with no limit to depth of references. To support this, I've created a graph generator that walks the formulas and has a way to output a list of all tokens found and their formulas, like this:

expressions = {
  FIRST: "SECOND * 2",
  SECOND: "THIRD * 2",
  THIRD: 2,
}

When my formulas are more than one reference level deep, as the example above is, Calculator#solve! raises an UnboundVariableError.

I found that this is because my expression hash is in an order where tokens are referenced before they are declared. The hash above is an example of this. Here is a failing test that I've written in spec/bulk_expression_solver_spec.rb to show the problem:

it 'resolves capitalized keys when they are declared out of order' do
  expressions = {
    FIRST: "SECOND * 2",
    SECOND: "THIRD * 2",
    THIRD: 2,
  }

  result = described_class.new(expressions, calculator).solve

  expect(result).to eq(
    FIRST: 8,
    SECOND: 4,
    THIRD: 2
  )
end

The failure message is:

|| Failures:
|| 
||   1) Dentaku::BulkExpressionSolver#solve resolves capitalized keys when they are declared out of order
||      Failure/Error:
||        expect(result).to eq(
||          FIRST: 8,
||          SECOND: 4,
||          THIRD: 2
||        )
||      
||        expected: {:FIRST=>8, :SECOND=>4, :THIRD=>2}
||             got: {:FIRST=>:undefined, :SECOND=>4, :THIRD=>2}
||      
||        (compared using ==)
||      
||        Diff:
||        @@ -1 +1 @@
||        -:FIRST => 8,
||        +:FIRST => :undefined,
||        
||      # ./spec/bulk_expression_solver_spec.rb:210:in `block (3 levels) in <top (required)>'

If I swap the order of FIRST and SECOND in expressions, though, the test passes.

I dug into BulkExpressionSolver#variables_in_resolve_order as a starting point. When the keys are in the order which allows the test to pass (SECOND, FIRST, THIRD), DependencyResolver.find_resolve_order(dependencies) returns ["third", "SECOND", "second", "FIRST", "THIRD"]. When "FIRST" is declared before "SECOND", this call returns ["second", "FIRST", "third", "SECOND", "THIRD"] and the test fails.

I tracked it down to this line. If I change [identifier] to [identifier.upcase], the test passes no matter what order the expressions are declared in the hash.

When I have this line changed to end with [identifier.upcase], DependencyResolver.find_resolve_order(dependencies) returns ["THIRD", "SECOND", "FIRST"].

I found that if I change the calculator initialization to include case_sensitive: false, then the test will also pass and the resolved dependencies are ["THIRD", "SECOND", "FIRST"].

I think the problem is that StringCasing is used to downcase the hash keys/identifiers when case sensitive is false, but the expressions are not downcased.