Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No implicit type conversions #360

Closed
SWW13 opened this issue Feb 23, 2021 · 21 comments
Closed

No implicit type conversions #360

SWW13 opened this issue Feb 23, 2021 · 21 comments

Comments

@SWW13
Copy link

SWW13 commented Feb 23, 2021

Not sure how to describe the problem further than "function arguments are broken" as I can reduce the problem with just a simple compare and no optimizations enabled.

Minimal Reproducer:

const SCRIPT: &'static str = r###"
fn function(arg0) {
    print("arg0 = " + arg0);
    
    if arg0 == 42 {
        print("the answer to life the universe and everything");
    } else {
        print("the cake is a lie");
    }
}
"###;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut engine = rhai::Engine::new();
    engine.set_optimization_level(rhai::OptimizationLevel::None);

    let ast = engine.compile(SCRIPT)?;
    let mut scope = rhai::Scope::new();
    let _: () = engine.call_fn(&mut scope, &ast, "function", (42u32,))?;

    Ok(())
}

Output:

arg0 = 42
the cake is a lie
@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

Rhai integers default to i64. You're using u32. They are different types, so they don't compare. Therefore the == operator always returns false - that's the default for comparisons.

Try:

let _: () = engine.call_fn(&mut scope, &ast, "function", (42i64,))?;

@SWW13
Copy link
Author

SWW13 commented Feb 24, 2021

So I can only use i64 if I want to do anything useful?
Why are there even other incompatible types?
Why doesn't the documentation mention this issue?
It feels fundamental broken in this state.

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

There are no types in Rhai, so you can use any type. It doesn't matter at all to the engine.

By default, all Rust integer types are supported in Rhai.

Your problem is trying to mix them (for example, comparing a u32 with a i64). Even Rust won't allow you to do that. So Rhai is the same as Rust here, and not the same as C-like languages where there are implicit type conversions for integers.

You can use to_int to turn almost all integer types into i64. For example, the following will work:

if age0.to_int() == 42 {

Or you can directly compare types of u32 with u32...

// here assume 'age_target' is another 'u32' variable
if age0 == age_target {

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

Why doesn't the documentation mention this issue?

You're right, it doesn't make it clear. I'll add a section into the Book about no implicit type conversions for integers.

EDIT: Warning section added - https://rhai.rs/book/language/numbers.html

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

So I can only use i64 if I want to do anything useful?

You can use i64 or i32 (if you turn on the only_i32 feature flag). Using i32` has the added benefit of being the same as Rust's default integer type.

But if you want to use u32 by default, then unfortunately yes it'll be harder as number literals do not parse to u32.

I'd suggest you simply use i64 as it is a superset of u32.

This is better than many scripting languages where you don't even have the choice of integers - many of them only have one f64 number type.

@SWW13
Copy link
Author

SWW13 commented Feb 24, 2021

Your problem is trying to mix them (for example, comparing a u32 with a i64). Even Rust won't allow you to do that. So Rhai is the same as Rust here, and not the same as C-like languages where there are implicit type conversions for integers.

That sounds like a really bad design decision when combined with the fact that rhai is dynamically typed.
Would be fine if the engine doesn't allow compares of different types, but also having strong typing this will result in many unseen problems.

So this is likely the real issue I have:

  • Disallowing compares with different types or at least a warning in case this happens.
  • Adding integer literals with types like in Rust (42u32 or similar).

Edit: I don't want to sound rude, but this was a really painful experience so far.

@SWW13 SWW13 changed the title Function argument are broken when called from Rust No implicit type conversions Feb 24, 2021
@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

Well, it is probably more a design choice.

Option 1: Allow only specific types. So you'll get an error if you try to put in a function with u32 parameters. The user has to go through fire hoops to allow a new type in scripting.

Option 2: Allow all types. Anything works. There are no restrictions. But that also means that the user can put anything in, and it is probably easy to mess up data types.

Rhai takes Option 2.

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

A bit more on Option 1: in order to restrict types, the user no longer has the freedom to work directly with Dynamic values.

For example:

let foo = SomeType::new();

let d = Dynamic::try_from(foo)?;    // is `SomeType` an allowed type?  How should `Dynamic` know?  Should this fail?

let mut scope = Scope::new();

scope.push("x", 42_u8);       // is `u8` allowed?  How should `Scope` know?

// The only alternative is this:

engine.allow::<u8>();
engine.set_scope_value(&mut scope, "x", 42_u8)?;

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

Would be fine if the engine doesn't allow compares of different types, but also having strong typing this will result in many unseen problems.

You're probably right... I have been struggling for quite a while whether to build in automatic type conversions for integer values... Even had a prototype a long time ago.

However, it opens up to yet another set of problems, especially when Rhai doesn't have typed literals. It is easy to have an expression and lose track of the actual number type; then things start failing somewhere down the road.

So far, the only reasonable route is to always use i64 and avoid other integer types. This is the cost we must pay to have integers in the first place... Many scripting languages simply avoid it by forcing everything to f64 (not even f32 because they probably use NaN tagging).

@SWW13
Copy link
Author

SWW13 commented Feb 24, 2021

I agree on that Option 1 isn't a good choice for a scripting language.

But I don't think these are the only options and Option 2 can be further divided.
"Allow all types" doesn't have to include "There are no restrictions".

I image implementing compare just like the other operands (if I understand them correctly), so that there will be an error for 0u32 == 0i64 like error: function not found for == (u32, i64).

I feel like rhai picks concepts from other language and ends up with bad collection while there are good variants/compromises near by. In my experience having strict types is a very good thing and other languages struggle so hard without, same goes for implicit conversions. But then rhai does dynamic typing like Python which will break during execution (or produce unexpected behavior) and doesn't allow enforcing types. While I see why a scripting language wants this, there is still the mentioned problem with compares. Without that dynamic typing is okay-ish.

I think a great compromise would be a TypeScript like approach:

  • You don't have to give a type, it defaults to dynamic.
  • You can optionally add a type, then usage of that variable will be type-checked during "compilation".

Then compares would work if any operand is dynamic or both are the same.

I see how this could be huge implementation cost. So maybe the stricter compare function and other integer literals are the more practical way to go.
But without further improvement the compare implementation and strict typing in its current state is more a burden then anything else.

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

The comparison operators default to false in order to simplify a large majority of use cases when comparing dissimilar data types:

let x = "hello";

if x == 42 {
    print("It is The Answer!");
} else {
    print("Well... " + x);
}

Without the default, x == 42 will cause a runtime error, or the following is needed:

if type_of(x) == "i64" && x == 42 {

But maybe the solution is to turn off defaults when comparing between different integer types?

@SWW13
Copy link
Author

SWW13 commented Feb 24, 2021

Without the default, x == 42 will cause a runtime error, or the following is needed:

I see, for these cases it's a sensible default.

But maybe the solution is to turn off defaults when comparing between different integer types?

That sounds like a low hanging fruit we want to grab.

An alternative could be an additional compare operator like ===, but these usually open the door for a bunch of other problems.

I'm also thinking about a linting mode where warnings are emitted if the types differ, but that could be a bit noisy.

@schungx
Copy link
Collaborator

schungx commented Feb 24, 2021

I'm also thinking about a linting mode where warnings are emitted if the types differ, but that could be a bit noisy.

Something like a use strict; that turns on strict mode? That's interesting.

@schungx
Copy link
Collaborator

schungx commented Feb 25, 2021

@SWW13
Copy link
Author

SWW13 commented Feb 25, 2021

Something like a use strict; that turns on strict mode? That's interesting.

I thought more of something like an engine option for that. But letting scripts define strict mode also sounds nice as it won't affect everything.

@schungx
Copy link
Collaborator

schungx commented Feb 26, 2021

It can easily be an engine option together with a script option to turn it on.

@schungx
Copy link
Collaborator

schungx commented Feb 26, 2021

The new versions will now disallow by default:

  1. Comparisons between the same types without operators defined

  2. Comparisons between any numeric types (even different) without operators defined

Only defaults will be comparing between two different types.

@SWW13
Copy link
Author

SWW13 commented Feb 26, 2021

It can easily be an engine option together with a script option to turn it on.

Yeah, both sounds nice.

The new versions will now disallow by default: [...]

Maybe I'm reading this wrong, but are all comparisons disabled by default then? Or only ones for "unknown" types?
I think there should be implementations for comparisons for all types that also implement e.g. add by default, except for raw engine.

@schungx
Copy link
Collaborator

schungx commented Feb 26, 2021

Maybe I'm reading this wrong, but are all comparisons disabled by default then? Or only ones for "unknown" types?
I think there should be implementations for comparisons for all types that also implement e.g. add by default, except for raw engine.

Yes, It is more of a refinement. For example, for x op y:

  • If x and y are the same type (or even the same unknown type), then it fails.

  • If x and y are numeric, then it fails, even if x and y are the same type.

  • If x and y are different types, we know that == is always false etc. So it defaults.

@SWW13
Copy link
Author

SWW13 commented Feb 26, 2021

That sounds fine. Should prevent a lot of silent coding errors without the burden of type checking everywhere.

@schungx
Copy link
Collaborator

schungx commented Feb 27, 2021

Closing this for now as the new refinement will come to the next version.

@schungx schungx closed this as completed Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants