Context::to_ordinary_primitive panics when passed a function

[vgel]

Description
Trying to convert a function object (such as function test() {}) to a primitive (such as via the Number constructor) panics, because of a mismatch of assumptions between Value::to_primitive and Context::ordinary_to_primitive.

To Reproduce

$ RUST_BACKTRACE=1 cargo run --bin boa
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/boa`
>> function test() {}
undefined
>> Number(test)
thread 'main' panicked at 'assertion failed: o.get_type() == Type::Object', boa/src/context.rs:379:9
stack backtrace:
# ....... snipped panic machinery ......
  11: std::panicking::begin_panic
             at /home/vogel/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:456
  12: boa::context::Context::ordinary_to_primitive
             at boa/src/context.rs:379
  13: boa::value::Value::to_primitive
             at boa/src/value/mod.rs:600
  14: boa::value::Value::to_numeric_number
             at boa/src/value/mod.rs:914
  15: boa::builtins::number::Number::make_number
             at boa/src/builtins/number/mod.rs:134
  16: boa::object::gcobject::GcObject::call
             at boa/src/object/gcobject.rs:176
  17: boa::context::Context::call
             at boa/src/context.rs:141
  18: <boa::syntax::ast::node::expression::call::Call as boa::exec::Executable>::run
             at boa/src/syntax/ast/node/expression/call/mod.rs:95
  19: <boa::syntax::ast::node::Node as boa::exec::Executable>::run
             at boa/src/syntax/ast/node/mod.rs:262
  20: <boa::syntax::ast::node::statement_list::StatementList as boa::exec::Executable>::run
             at boa/src/syntax/ast/node/statement_list/mod.rs:70
  21: boa::context::Context::eval
             at boa/src/context.rs:499
  22: boa::main
             at boa_cli/src/main.rs:188
 # .... snipped rust boot ....
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Context
Value::to_primitive checks if its own variant is Object, and if so passes the value over to Context::ordinary_to_primitive:

boa/boa/src/value/mod.rs

Lines 595 to 609 in 3e60d2c

	pub fn to_primitive(&self, ctx: &mut Context, preferred_type: PreferredType) -> Result<Value> {
	// 1. Assert: input is an ECMAScript language value. (always a value not need to check)
	// 2. If Type(input) is Object, then
	if let Value::Object(_) = self {
	let mut hint = preferred_type;
	// Skip d, e we don't support Symbols yet
	// TODO: add when symbols are supported
	// TODO: Add other steps.
	if hint == PreferredType::Default {
	hint = PreferredType::Number;
	};
	// g. Return ? OrdinaryToPrimitive(input, hint).
	ctx.ordinary_to_primitive(self, hint)

But Context::to_ordinary_primitive checks if get_type() returns Type::Object:

boa/boa/src/context.rs

Lines 377 to 383 in 87d9e9c

	pub(crate) fn ordinary_to_primitive(
	&mut self,
	o: &Value,
	hint: PreferredType,
	) -> Result<Value> {
	// 1. Assert: Type(O) is Object.
	debug_assert!(o.get_type() == Type::Object);

These two don't match up, because Value::get_type checks if the object is callable, and if so returns function:

boa/boa/src/value/type.rs

Lines 49 to 54 in 59d328e

	Self::Object(ref object) => {
	if object.borrow().is_function() {
	Type::Function
	} else {
	Type::Object
	}

I believe the correct thing to do by the standard here is to treat functions the same as objects. https://tc39.es/ecma262/#sec-ordinarytoprimitive says Type of O should be Object, and in that section it only lists the Object type. Function as a type only comes up in https://tc39.es/ecma262/#sec-typeof-operator .

Build environment (please complete the following information):

• OS: Ubuntu
• Version: Linux marina 5.4.0-7634-generic #38~1592497129~20.04~9a1ea2e-Ubuntu SMP Fri Jun 19 22:43:37 UTC x86_64 x86_64 x86_64 GNU/Linux
• Target triple: x86_64-unknown-linux-gnu
• Rustc version: rustc 1.46.0 (04488afe3 2020-08-24)

[vgel]

Oh, also this doesn't panic in release mode -- it just does the wrong thing.

>> function test() {}
undefined
>> Number(test)
Uncaught: "TypeError": "cannot convert object to primitive value"

It's getting down here somehow:

boa/boa/src/context.rs

Lines 412 to 413 in 87d9e9c

	// 6. Throw a TypeError exception.
	self.throw_type_error("cannot convert object to primitive value")

Which I'm not sure why... the function should have a toString key.

[RageKnify]

@Razican and @Lan2u you were the ones who took care of #435 / #441 , looking at the codebase I don't think Type::Function is actually used, and it seems like the spec just sees functions as objects. Do you see any reason against removing Type::Function?

An object can still be identified as a function cince its ObjectData variant will be Function.

[Razican]

@Razican and @Lan2u you were the ones who took care of #435 / #441 , looking at the codebase I don't think Type::Function is actually used, and it seems like the spec just sees functions as objects. Do you see any reason against removing Type::Function?

An object can still be identified as a function cince its ObjectData variant will be Function.

I think it can be removed, there is no such thing as a "function" type in JavaScript, they are callable objects.

[RageKnify]

I did just check in chrome, and:

function x() {}
typeof x

Does return "function", so we need to keep that functionality...

[HalidOdat]

Does return "function", so we need to keep that functionality...

can't we check if its an object with Value::is_object, this gives true for any object including function

[vgel]

Should be incidentally fixed by #777, as ordinary_to_primitive is now associated with GcObject and no longer takes a Value, so it doesn't check the type.

[HalidOdat]

@vgel this has been fixed, right?

[RageKnify]

Yes, I just tested on master, function t() {}; Number(t) return NaN, just like Chrome does.