Thanks for the work.

I think you need to test it under 32-bit as well as feature flags like only_i32 etc. Some of the redundant stuff are there to handle different combinations of 32/64-bit.

Thanks for the work.

I think you need to test it under 32-bit as well as feature flags like only_i32 etc. Some of the redundant stuff are there to handle different combinations of 32/64-bit.

Fixed. It doesn't really help that many unittests fail when compiled with only_i32. I did check that the same unittests fail before and after the change, though.

Fixed. It doesn't really help that many unittests fail when compiled with only_i32. I did check that the same unittests fail before and after the change, though.

You'd also need to back out the tests with MAX_USIZE_INT etc. because they are required for 32-bit.

Fixed. It doesn't really help that many unittests fail when compiled with only_i32. I did check that the same unittests fail before and after the change, though.

You'd also need to back out the tests with MAX_USIZE_INT etc. because they are required for 32-bit.

I don't understand what you mean I'm afraid.

cargo test --features only_i32 has the exact same failures on main as it does with this branch:

failures:
    src/api/call_fn.rs - api::call_fn::Engine::call_fn (line 88)
    src/api/call_fn.rs - api::call_fn::Engine::call_fn_with_options (line 135)
    src/api/compile.rs - api::compile::Engine::compile (line 13)
    src/api/compile.rs - api::compile::Engine::compile_expression (line 246)
    src/api/compile.rs - api::compile::Engine::compile_expression_with_scope (line 270)
    src/api/compile.rs - api::compile::Engine::compile_scripts_with_scope (line 168)
    src/api/compile.rs - api::compile::Engine::compile_with_scope (line 42)
    src/api/eval.rs - api::eval::Engine::eval (line 21)
    src/api/eval.rs - api::eval::Engine::eval_ast (line 143)
    src/api/eval.rs - api::eval::Engine::eval_ast_with_scope (line 165)
    src/api/eval.rs - api::eval::Engine::eval_binary_op (line 269)
    src/api/eval.rs - api::eval::Engine::eval_expression (line 82)
    src/api/eval.rs - api::eval::Engine::eval_expression_with_scope (line 100)
    src/api/eval.rs - api::eval::Engine::eval_with_scope (line 46)
    src/api/eval.rs - api::eval::eval (line 336)
    src/api/events.rs - api::events::Engine::on_var (line 39)
    src/api/mod.rs - api::Engine::register_custom_operator (line 176)
    src/api/register.rs - api::register::Engine::register_fn (line 42)
    src/api/register.rs - api::register::Engine::register_get_set (line 372)
    src/api/register.rs - api::register::Engine::register_indexer_get (line 439)
    src/api/register.rs - api::register::Engine::register_indexer_get_set (line 563)
    src/api/register.rs - api::register::Engine::register_indexer_set (line 500)
    src/api/register.rs - api::register::Engine::register_set (line 319)
    src/api/register.rs - api::register::Engine::register_static_module (line 647)
    src/api/register.rs - api::register::Engine::register_type (line 138)
    src/api/run.rs - api::run::Engine::run_ast_with_scope (line 100)
    src/api/run.rs - api::run::Engine::run_with_scope (line 39)
    src/engine.rs - engine::Engine (line 76)
    src/func/func_args.rs - func::func_args::FuncArgs::parse (line 19)
    src/func/func_trait.rs - func::func_trait::Func::create_from_ast (line 25)
    src/func/func_trait.rs - func::func_trait::Func::create_from_script (line 58)
    src/module/mod.rs - module::Module::eval_ast_as_new (line 2263)
    src/types/dynamic.rs - types::dynamic::Dynamic::from (line 1330)
    src/types/fn_ptr.rs - types::fn_ptr::FnPtr::call (line 220)
    src/types/scope.rs - types::scope::Scope (line 31)
    src/types/scope.rs - types::scope::Scope<'_>::get (line 693)
    src/types/scope.rs - types::scope::Scope<'_>::get_mut (line 773)

test result: FAILED. 97 passed; 37 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.07s

Sorry, I made a mistake. It will cause problem when not using only_i32 feature flag on a 32-bit CPU.

// Original
#[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)]
let len = len.min(MAX_USIZE_INT) as usize;

// New
let len = len as usize;

The new version is problematic.

Consider a 32-bit CPU where usize is 32-bit. len is i64 which is 64-bit.

Casting len as usize truncates. However, the clippy lint is turned off here so you don't see it.

It currently does not cause a CI failure simply because our CI does not fully test edge cases. The tests are actually extremely minimal coverage... Also not many people have fully tested Rhai on 32-bit (although I sometimes run CI on 32-bit target once in a blue moon). Adding 32-bit tests into CI doesn't help unless the edge-case tests are added.

It'd not even prop up during fuzzing unless fuzzing on a 32-bit machine.

Edit: Changes

Actually, this is old code. It should be much more idiomatic to use usize::try_from(...) in order to catch overflow errors, which should work for all architectures. I think I can go in and replace all usage of MAX_USIZE_INT with usize::try_from.

try_from is certainly a good idea, if error handling is done right (rather than e.g. simply unwrapping).

try_from is certainly a good idea, if error handling is done right (rather than e.g. simply unwrapping).

Yup, and this is what I'm doing. I'm almost done, but still need to comb through carefully just in case I make a mistake.

Maybe it'd be better for you to wait for my commit and then rebase off that. There will be quite a number of changes to change all implicit castings to try_from.

The goal is to remove as many as XXX casts as possible.

@therealprof Please try to rebase off the latest drop. I have replaced most non-trivial castings with try_from which should make Rhai much safer.

Done... Not entirely sure I agree with the gobs of new unwrapping making Rhai safer than the hard casting but well.

Well, with hard casting I always have to check it very carefully for edge cases, and a lot of times I screw up only for it to come back via fuzzing.

Not that fuzzing can catch all...

try_from().unwrap() is essentially a hard cast so there isn't anything less safe. On the other hand the hard cast will silently fail in release builds making it even more dangerous.