ruby-units icon indicating copy to clipboard operation
ruby-units copied to clipboard

Stack level too deep error

Open csm123 opened this issue 10 years ago • 2 comments

On this: Unit.new("1 to 1 1/4 lbs")

I understand that the provided value is not valid for ruby-units, but a more catchable error would be very helpful.

csm123 avatar Jul 07 '14 03:07 csm123

I just addressed a similar problem in PR #107 but based on this issue it seems the problem is a bit wider than just parsing currency (the particular stack level too deep problem that my PR addresses).

I just verified that I also get the stack level too deep problem doing Unit('5' 3/4"').

I traced the issue to the code in initialize :

      if options.first.instance_of?(String)
        opt_scalar, opt_units = RubyUnits::Unit.parse_into_numbers_and_units(options[0])
        unless @@cached_units.keys.include?(opt_units) || (opt_units =~ /(#{RubyUnits::Unit.temp_regex})|(pounds|lbs[ ,]\d+ ounces|oz)|('\d+")|(ft|feet[ ,]\d+ in|inch|inches)|%|(#{TIME_REGEX})|i\s?(.+)?|±|\+\/-/)
          @@cached_units[opt_units] = (self.scalar == 1 ? self : opt_units.unit) if opt_units && !opt_units.empty?
        end
      end

It seems that parse_into_numbers_and_units() does not cater for all of the same input string formats that parse does and inadvertently returns the input string into opt_units which then causes opt_units.unit to call String#to_unit and thus infinite recursion occurs as the string tries to be converted to a Unit and parsed again.

I am not sure what that code is supposed to do, but it doesn't make sense that the string should be parsed twice using different and incompatible approaches. I commented out that code and only a single test failed :

Failures:

  1) Unit Output formatting for a unit with a custom display_name 
     Failure/Error: subject { Unit("8 cups") }
     ArgumentError:
       'cupz' Unit not recognized
     # ./lib/ruby_units/unit.rb:1518:in `parse'
     # ./lib/ruby_units/unit.rb:368:in `initialize'
     # ./lib/ruby_units/string.rb:6:in `new'
     # ./lib/ruby_units/string.rb:6:in `to_unit'
     # ./lib/ruby_units/unit.rb:382:in `initialize'
     # ./lib/ruby_units/unit.rb:352:in `initialize'
     # ./lib/ruby_units/array.rb:7:in `new'
     # ./lib/ruby_units/array.rb:7:in `to_unit'
     # ./lib/ruby_units/object.rb:9:in `Unit'
     # ./spec/ruby-units/unit_spec.rb:1459:in `block (3 levels) in <top (required)>'
     # ./spec/ruby-units/unit_spec.rb:1461:in `block (3 levels) in <top (required)>'

Now whilst that is the only test that fails, removing the code above addresses an entire class of recursive string parsing stack level too deep errors because of the incompatible parsing in parse_into_numbers_and_units vs the fuller capability of parse . It seems parse_into_numbers_and_units should probably be retired and the need for double parsing using different approaches seriously questioned.

There still remains the problem that Unit('5' 3/4"') does not return the expected result, but with the code above commented out, it is certainly better than blowing up on invalid user input.

ahacking avatar Aug 21 '14 14:08 ahacking

Other examples that crash: Unit.new('m^2/1') and Unit.new('m^3/1').

However, Unit.new('m/1') and Unit.new('m^2')/Unit.new('1') do work.

kreintjes avatar Apr 30 '19 09:04 kreintjes