Typing violation in `lib/xlsx_reader/conversion.ex` when using `Decimal.parse/1`

[tosie]

The Elixir 1.18 compiler outputs the following warning when using xlsx_reader 0.8.8:

     warning: the following clause will never match:

         {:ok, decimal}

     because it attempts to match on the result of:

         Decimal.parse(string)

     which has type:

         dynamic(
           :error or {%Decimal{coef: :NaN or :inf, exp: integer(), sign: integer()}, binary()} or
             {%Decimal{coef: :NaN or :inf or integer(), exp: integer(), sign: integer()}, term()}
         )

     typing violation found at:
     │
 155 │       {:ok, decimal} ->
     │       ~~~~~~~~~~~~~~~~~
     │
     └─ (xlsx_reader 0.8.8) lib/xlsx_reader/conversion.ex:155: XlsxReader.Conversion.to_decimal/1

Here is the affected code:

xlsx_reader/lib/xlsx_reader/conversion.ex

Lines 153 to 164 in 411a48e

	def to_decimal(string) do
	case Decimal.parse(string) do
	{:ok, decimal} ->
	{:ok, decimal}
	{decimal, ""} ->
	{:ok, decimal}
	_ ->
	:error
	end
	end

The (documentation of Decimal.parse/1)[https://hexdocs.pm/decimal/Decimal.html#parse/1]) describes the possible outputs as:

If successful, returns a tuple in the form of {decimal, remainder_of_binary}, otherwise :error.

It seems like the typing change was introduced with Decimal 2.0 in September of 2020 (https://github.com/ericmj/decimal/releases/tag/v2.0.0).

Not sure how to solve this (= not showing a warning) without breaking backwards compatibility to Decimal 1.x ... have you thought about how to handle this, yet? Maybe drop support for older versions of Decimal? Or would you just ignore the compiler warning?

[xavier]

It might be possible to inspect which version of Decimal is being used at runtime and add a guard but I'm not sure the type checker would be able to take that into account…

[xavier]

According to the hex.pm page for Decimal 1.9.0, the version is still being downloaded several thousand times per day so I'm not sure that we can drop support for 1.x even though it hasn't seen an update in nearly 5 years.

[tosie]

Oh wow, did not realize that. From my point of view somebody who is using an old version of Decimal will also be using an old version of other deps like xlsx_reader. So changing the requirements in a newer version of xlsx_reader should be okay. But that is just my opinion :-)