rusty_v8 icon indicating copy to clipboard operation
rusty_v8 copied to clipboard

`impl_try_from!` for `Integer` is misleading / too strict

Open aapoalas opened this issue 3 years ago • 0 comments

The TryFrom macros for the Integer class are defined with a conditional on Value::IsUint32() || Value::IsInt32().

This is correct in the strictest sense that Uint32 and Int32 are the only SMI integer classes and the Integer class internally has separate paths for SMIs and other numbers. However, this isn't really quite how V8 seems to view to class itself. To V8, Integer is more like a view to a Number value. As is somewhat evident from the Integer::Value() method's return type i64, the class can represent more than just 32 bit numbers. There just seems to be no external API to really check if a number is Integer or not. Which I guess is just because there really is no way to tell so we can't really do anything about that one way or another.

In any case, it's not strictly correct to only allow the Integer class to be upcasted if it's internally an SMI. That being said, the alternative would be to allow any number to be upcast into an integer which will probably lead into "happy" accidents.

Still, it can be a bit surprising that this function will only ever return values up to 32 bit integers, and likewise the main function will panic:

fn get_maybe_int64(value: v8::Local<v8::Value>) -> Result<i64, AnyError> {
  v8::Local::<v8::Integer>::try_from(value)?.value()
}

fn main() {
  let integer = v8::Number::new(scope, MAX_SAFE_INTEGER + 1);
  get_maybe_int64(integer).unwrap(); // This will panic
}

Not sure what exactly should be done about this, though.

aapoalas avatar Jul 12 '22 20:07 aapoalas