Decimal bug - please be careful with negative coefficients - fix is on pull request

I think the intended use here and for most structs is to not construct them yourself with raw internals but to use the library as the authority on validating/constructing… in this case, Decimal.new handles this by setting coef to an absolute value decimal/lib/decimal.ex at main · ericmj/decimal · GitHub

I’ll let @ericmj or other maintainers decide if they’re also interested in this additional safeguard, and I’ve been burned by constructing a struct incorrectly myself like this before, but, worth knowing as a general rule in elixir.

The coefficient type is clearly typed as non negative integer. Even though I’d not necessarily expect the library to crash on malformed data, I’d at least not expect it to behave correctly.

So any PR “fixing” this should do so by crashing because of function clause errors or badarg at least.

yes - the fix does exact that - anyway I was interested on your opinions and ideas. Thanks.

Maybe the Norm lib:

https://github.com/keathley/norm

Nitpick: that specific code

defmodule Derp do
  @spec bad_decimal() :: Decimal.t()
  def bad_decimal() do
    Decimal.new(%Decimal{sign: -1, coef: -12000000000})
  end
end

will be detected by Dialyzer:

lib/derp.ex:3:no_return
Function bad_decimal/0 has no local return.
________________________________________________________________________________
lib/derp.ex:4:call
The function call will not succeed.

Decimal.new(%Decimal{:coef => -12_000_000_000, :exp => 0, :sign => -1})

breaks the contract
(decimal()) :: t()

because Dialyzer can be 100% certain that -12_000_000_000 is not a non_neg_integer().

However, using a variable gets it to pass:

defmodule Derp do
  @spec bad_decimal(pos_integer()) :: Decimal.t()
  def bad_decimal(n) do
    Decimal.new(%Decimal{sign: -1, coef: -n})
  end
end

BUT I’m not sure how far library code should go to defend its invariants against intentional manipulation.

For instance, it’s possible to produce a MapSet with strange behavior at runtime:

iex(28)> x = MapSet.new([:a, :b])                        
#MapSet<[:a, :b]>

# NOTE: writing this will trigger a Dialyzer error because `MapSet` is opaque, but works at runtime
iex(29)> y = %MapSet{map: %{c: "wat"}}
#MapSet<[:c]>
iex(30)> z = MapSet.union(x, y)       
#MapSet<[:a, :b, :c]>
iex(31)> z2 = MapSet.new([:a, :b, :c])
#MapSet<[:a, :b, :c]>
iex(32)> MapSet.equal?(z, z2)         
false

but that code will always cause a Dialyzer error because of the @opaque setting on MapSet.

There are some gotchas with @opaque, notably in pattern matching and module attributes which may explain why it isn’t used in Decimal.

Yes exactly. Elixir is dynamically typed. If you abuse that to create malformed types they won’t work properly. This is true of literally every struct.

Thanks all for commenting.

@benwilson512 : Of course! I know. But does that mean we should not fix error edge cases? → I mean NO. And I think you agree with me and if we can we should make it easier to use and more safe for everybody.

As NobbZ wrote : So any PR “fixing” this should do so by crashing because of function clause errors or bad arg at least.

And thats exactly what it does. So back to the focus :

I think too the best bevavior for the existing Decimal lib and all the depending libs are: let it crash if wrong inputs:

  # wrong sign
  assert_raise Decimal.Error, fn ->
    Decimal.new(%Decimal{sign: 2, coef: 1, exp: 2})
  end

  # wrong coef
  assert_raise Decimal.Error, fn ->
    Decimal.new(%Decimal{sign: 1, coef: -1, exp: 2})
  end

iex(1)> Decimal.new(%Decimal{sign: 1, coef: 1, exp: 2})  
#Decimal<1E+2>
iex(2)> Decimal.new(%Decimal{sign: 2, coef: 1, exp: 2})
** (Decimal.Error) : wrong decimal number: (%Inspect.Error{message: "got ArgumentError with message \"argument error\" while inspecting %{__struct__: Decimal, coef: 1, exp: 2, sign: 2}"}) 
    (decimal 2.0.0-rc.0) lib/decimal.ex:1151: Decimal.new/1
iex(3)> Decimal.new(%Decimal{sign: 1, coef: -1, exp: 2})
** (Decimal.Error) : wrong decimal number: (%Inspect.Error{message: "got ArgumentError with message \"argument error\" while inspecting %{__struct__: Decimal, coef: -1, exp: 2, sign: 1}"})
    (decimal 2.0.0-rc.0) lib/decimal.ex:1151: Decimal.new/1 ```

all other wrong cases are handled by function clauses

So I think better then the current behavior with all the consequences of prolonging the error (see above):

iex(1)> Decimal.new(%Decimal{sign: 1, coef: 1, exp: 2})  
#Decimal<1E+2>
iex(2)> Decimal.new(%Decimal{sign: 2, coef: 1, exp: 2})
#Decimal<1E+2>
iex(3)> Decimal.new(%Decimal{sign: 1, coef: -1, exp: 2})
#Decimal<-.1E+3>

I think in a dynamically typed language, it is possible for a library to detect many misuses and edge cases at runtime, but there is a runtime cost to doing that. Documenting proper usage and data types allows to lower that runtime cost (assuming the documentation and type specifications are being followed)

yes something like this…

I think as much as possible without breaking the target. And it’s possible in this case.

Thanks for your thoughts!

yes you are right! There is a cost for it. I will try to measure it …