rustler icon indicating copy to clipboard operation
rustler copied to clipboard

Odd difference between ErlNifTermType and TermType returned by `Term.get_type()`

Open Qqwy opened this issue 3 years ago • 2 comments

enif_get_type returns an enum value depending on the type of value a particular term is.

Rustler currently uses Term.get_type(), which returns a TermType.

There is a difference in the types returned by these two approaches. Under the hood, get_type is implemented as a bunch of is_* checks.

However, there are two downsides with this approach:

  1. They are slower;
  2. They do not disambiguate between integers and floats.

rustler_sys does seem to know about the ErlNifTermType, but it doesn't seem to be used anywhere else in the code right now.

What would be the best way to improve this situation? I would be willing to contribute with a PR or two if someone can tell me which approach to take.

Qqwy avatar Sep 08 '21 07:09 Qqwy

Note that enif_get_type is rather recent (OTP 22). Rustler currently supports OTP 20.3 as well (at least according to the github actions). To implement that in a backwards-compatible way, Term.get_type() could call into a feature-gated helper. For OTP >= 22, this would call into enif_get_type and match on the resulting enum, and for OTP < 22, the current logic would be used. See Term.map_from_arrays() for an example on functions feature-gated to the NIF version.

The feature-gate mechanism uses build.rs to determine the NIF version. The version check can be overridden with RUSTLER_NIF_VERSION as well.

They do not disambiguate between integers and floats.

As long as we need to maintain backwards compatibility, this would still be the case.

evnu avatar Sep 08 '21 09:09 evnu

TermType and ErlNifTermType don't match exactly. For example, TermType has Number, while ErlNifTermType does not. Maybe we should have another function then to implement enif_get_type? It is unfortunate that Term.get_type() is already taken. So maybe we should not implement this for OTP < 22 at all, but instead disambiguate between the two?

evnu avatar Sep 08 '21 13:09 evnu

See https://github.com/rusterlium/rustler/pull/538, which tackles this problem.

evnu avatar Aug 17 '23 21:08 evnu

#538 has been merged and released, closing this.

filmor avatar Nov 14 '23 13:11 filmor