Fix Term's "get_type()" implementation

[philss]

It was not returning the correct type for integers, probably due to a "garbage" arriving because of the wrong type.
This affects the latest v0.30.

[philss]

Looks like this is not working well on Windows. I'm going to investigate tomorrow.

[filmor]

Hah, even found out who introduced the bug: #212 🕵️‍♂️

I can't debug the Windows issue for the next week or so since I won't have a Windows machine available.

[philss]

I can't debug the Windows issue for the next week or so since I won't have a Windows machine available.

Thanks! I'm trying to setup a Windows VM so I can debug this as well :)

[philss]

Just to give an update: I've tested and was able to reproduce the build problem on Windows.
Unfortunately I couldn't yet figure out what is going on - it just return "unknown" for any term there :/
I should go back to this next week.

[filmor]

Yeah, I'll be able to look into it this weekend as well. My initial guess would be a mismatched base type for the enum, Windows longs are always 32 bit.

[filmor]

@philss Did you get any further? I couldn't reproduce it so far, but I'll try again tomorrow.

[philss]

@filmor unfortunately I couldn't :/

I tried to figure out where the int type is wrong, but there is no explicit declaration of it.
For a comparison, I did write a small NIF in C and it works good on Windows. So the problem seems to be on our side.
I will give another try this week as well.

[philss]

@filmor I tried a bunch of things to fix this problem with the Windows build, but I couldn't find a solution.
I suspect the problem is in the bindings we create in the rustler_sys create for the NIF API.

I also tried to setup rust-bindgen on my windows VM, but without success. The idea was to generate a file that would follow more closely what is expected from the erl_nif.h file.

I think, given that we cannot make it work on Windows easily, we could drop this feature for that OS, documenting this behaviour. WDYT?

[josevalim]

Given it never worked, perhaps we should just nuke it altogether. :)

[filmor]

Well, it works fine on Unix and is IMO the preferable approach (single function call vs. potentially ~10). I'll try to get the Windows interface in line (I think this is mostly about distinguishing floats from integers).

/edit: Also, this is clearly a potentially nasty bug (e.g. a type confusion) that I would really like to clear up.

[josevalim]

It is a single function call but internally it is 10 bitmap comparisons. If you want to know if something is a float or integer, asking specifically for float and integer would be more efficient.

Also note that nor Erlang nor Elixir have a similar function and we have been just fine for decades. Given it is broken in main, odds are that we really don’t need it.

that said, if you can make it work, then that’s great. I am just saying that, if it doesn’t work, it is not the end of the world. :)

[filmor]

@philss @evnu This finally works and passes all tests. It's really not pretty to essentially "copy" this gnarly code for is_float. We could still do as Jose suggests and drop the whole new code path, but since it works fine on non-Windows (which is likely the primary userbase anyhow) and we now have a workaround.

Notes:

• I will still have to test this manually on 32bit Windows as that is not part of our CI at all and the is_float codepath is totally different (maybe enif_term_type works there as well?)
• There is apparently a bug in OTP24, in that is_fun doesn't work for remote funs (fun module:function/0 or in Elixir &Function.identity/1). I don't intend to debug this further, it works for "function objects".

[josevalim]

For what is worth, I don't think we should export a function that does not work on Windows as it may become an additional pitfall for both developers and Windows users. When a package using Rustler misbehaves on Windows, it may lead to both users and authors spent time debugging only to land here.

If you really want to go down that route, can it be an opt-in feature? So it is not available by default, reducing the chance of accidental use. Or, make sure that it is not available on Windows (so it is not even compiled) or that it raises a clear error message?

[filmor]

Which export are you now talking about? enif_term_type is in rustler-sys, which is like all-sys crates "there be dragons".

What exactly would you make optional here?

My intention is to eventually get rid of the custom is_float again and use enif_term_type everywhere. The way it's now implemented, everything works the same way everywhere, apart from checking remote funs on OTP24, but that is a bug in a version of OTP that we are going to stop supporting in a few months (when 27 comes out) anyhow.

[josevalim]

My comment was to make enif_term_type optional. :) However, I was assuming that it still does not work on Windows (while the C NIF does). Given you said:

The way it's now implemented, everything works the same way everywhere, apart from checking remote funs on OTP24

If it all works on Windows too, then just ignore me. :) But it may be that using enif_get_float and attempting to get a float may be more efficient than the several masks from enif_term_type.

[filmor]

• On all non-Windows systems with OTP>=22 we use enif_term_type for the get_type function
• On Windows, we use the old way with an additional hacky-copied version of the non-exposed is_float (which I will probably replace with an enif_get_float)
• The behavior is now identical, the implementation is not (yet)
• The bug with remote funs on OTP24 is preexisting ant not something I intend to follow up on apart from writing it down
• I think the get_type will be useful for serializations, if benchmarks show that that's slower than chaining the enif_is_*, I'd consider that a bug in OTP since that is accoding to the docs why enif_term_type was introduced

[philss]

Awesome! For me this is good enough for now 🚢

@filmor thank you for the efforts here!