From 5bf90653c4c2742ce084dce9eed87463993db134 Mon Sep 17 00:00:00 2001 From: David Wendt Date: Wed, 30 Oct 2019 20:25:52 -0400 Subject: [PATCH] Add implicit coercions to remove most instances of manually constructing a `ReturnValue`. --- core/src/avm1.rs | 3 +-- core/src/avm1/activation.rs | 6 ++---- core/src/avm1/function.rs | 2 +- core/src/avm1/globals.rs | 28 ++++++++++++--------------- core/src/avm1/globals/math.rs | 30 ++++++++++++----------------- core/src/avm1/movie_clip.rs | 36 ++++++++++++++++------------------- core/src/avm1/object.rs | 16 +++++++--------- core/src/avm1/return_value.rs | 30 +++++++++++++++++++++++++++-- 8 files changed, 79 insertions(+), 72 deletions(-) diff --git a/core/src/avm1.rs b/core/src/avm1.rs index 56190854c..52944bc71 100644 --- a/core/src/avm1.rs +++ b/core/src/avm1.rs @@ -1,7 +1,6 @@ use crate::avm1::function::Avm1Function; use crate::avm1::globals::create_globals; use crate::avm1::object::Object; -use crate::avm1::return_value::ReturnValue; use crate::backend::navigator::NavigationMethod; use crate::context::UpdateContext; use crate::prelude::*; @@ -110,7 +109,7 @@ impl<'gc> Avm1<'gc> { form_values.insert( k, v.ok() - .unwrap_or(ReturnValue::Immediate(Value::Undefined)) + .unwrap_or_else(|| Value::Undefined.into()) .resolve(self, context) .ok() .unwrap_or(Value::Undefined) diff --git a/core/src/avm1/activation.rs b/core/src/avm1/activation.rs index 025d9e169..f206ca091 100644 --- a/core/src/avm1/activation.rs +++ b/core/src/avm1/activation.rs @@ -268,13 +268,11 @@ impl<'gc> Activation<'gc> { context: &mut UpdateContext<'_, 'gc, '_>, ) -> Result, Error> { if name == "this" { - return Ok(ReturnValue::Immediate(Value::Object(self.this))); + return Ok(Value::Object(self.this).into()); } if name == "arguments" && self.arguments.is_some() { - return Ok(ReturnValue::Immediate(Value::Object( - self.arguments.unwrap(), - ))); + return Ok(Value::Object(self.arguments.unwrap()).into()); } self.scope().resolve(name, avm, context, self.this) diff --git a/core/src/avm1/function.rs b/core/src/avm1/function.rs index b86168713..12b3412d7 100644 --- a/core/src/avm1/function.rs +++ b/core/src/avm1/function.rs @@ -296,7 +296,7 @@ impl<'gc> Executable<'gc> { } avm.insert_stack_frame(frame_cell); - Ok(ReturnValue::ResultOf(frame_cell)) + Ok(frame_cell.into()) } } } diff --git a/core/src/avm1/globals.rs b/core/src/avm1/globals.rs index 243332bc1..aa25dc663 100644 --- a/core/src/avm1/globals.rs +++ b/core/src/avm1/globals.rs @@ -20,7 +20,7 @@ pub fn getURL<'a, 'gc>( let url = url_val.clone().into_string(); if let Some(fscommand) = fscommand::parse(&url) { fscommand::handle(fscommand, avm, context); - return Ok(ReturnValue::Immediate(Value::Undefined)); + return Ok(Value::Undefined.into()); } let window = args.get(1).map(|v| v.clone().into_string()); @@ -34,7 +34,7 @@ pub fn getURL<'a, 'gc>( context.navigator.navigate_to_url(url, window, vars_method); } - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } pub fn random<'gc>( @@ -44,10 +44,10 @@ pub fn random<'gc>( args: &[Value<'gc>], ) -> Result, Error> { match args.get(0) { - Some(Value::Number(max)) => Ok(ReturnValue::Immediate(Value::Number( - action_context.rng.gen_range(0.0f64, max).floor(), - ))), - _ => Ok(ReturnValue::Immediate(Value::Undefined)), //TODO: Shouldn't this be an error condition? + Some(Value::Number(max)) => { + Ok(Value::Number(action_context.rng.gen_range(0.0f64, max).floor()).into()) + } + _ => Ok(Value::Undefined.into()), //TODO: Shouldn't this be an error condition? } } @@ -58,11 +58,9 @@ pub fn boolean<'gc>( args: &[Value<'gc>], ) -> Result, Error> { if let Some(val) = args.get(0) { - Ok(ReturnValue::Immediate(Value::Bool( - val.as_bool(avm.current_swf_version()), - ))) + Ok(Value::Bool(val.as_bool(avm.current_swf_version())).into()) } else { - Ok(ReturnValue::Immediate(Value::Bool(false))) + Ok(Value::Bool(false).into()) } } @@ -73,9 +71,9 @@ pub fn number<'gc>( args: &[Value<'gc>], ) -> Result, Error> { if let Some(val) = args.get(0) { - Ok(ReturnValue::Immediate(Value::Number(val.as_number()))) + Ok(Value::Number(val.as_number()).into()) } else { - Ok(ReturnValue::Immediate(Value::Number(0.0))) + Ok(Value::Number(0.0).into()) } } @@ -86,11 +84,9 @@ pub fn is_nan<'gc>( args: &[Value<'gc>], ) -> Result, Error> { if let Some(val) = args.get(0) { - Ok(ReturnValue::Immediate(Value::Bool( - val.as_number().is_nan(), - ))) + Ok(Value::Bool(val.as_number().is_nan()).into()) } else { - Ok(ReturnValue::Immediate(Value::Bool(true))) + Ok(Value::Bool(true).into()) } } diff --git a/core/src/avm1/globals/math.rs b/core/src/avm1/globals/math.rs index 90df82f5b..ca9bc326b 100644 --- a/core/src/avm1/globals/math.rs +++ b/core/src/avm1/globals/math.rs @@ -12,9 +12,9 @@ macro_rules! wrap_std { $name, |_avm, _context, _this, args| -> Result, Error> { if let Some(input) = args.get(0) { - Ok(ReturnValue::Immediate(Value::Number($std(input.as_number())))) + Ok(Value::Number($std(input.as_number())).into()) } else { - Ok(ReturnValue::Immediate(Value::Number(NAN))) + Ok(Value::Number(NAN).into()) } }, $gc_context, @@ -32,16 +32,12 @@ fn atan2<'gc>( ) -> Result, Error> { if let Some(y) = args.get(0) { if let Some(x) = args.get(1) { - return Ok(ReturnValue::Immediate(Value::Number( - y.as_number().atan2(x.as_number()), - ))); + return Ok(Value::Number(y.as_number().atan2(x.as_number())).into()); } else { - return Ok(ReturnValue::Immediate(Value::Number( - y.as_number().atan2(0.0), - ))); + return Ok(Value::Number(y.as_number().atan2(0.0)).into()); } } - Ok(ReturnValue::Immediate(Value::Number(NAN))) + Ok(Value::Number(NAN).into()) } pub fn random<'gc>( @@ -50,9 +46,7 @@ pub fn random<'gc>( _this: GcCell<'gc, Object<'gc>>, _args: &[Value<'gc>], ) -> Result, Error> { - Ok(ReturnValue::Immediate(Value::Number( - action_context.rng.gen_range(0.0f64, 1.0f64), - ))) + Ok(Value::Number(action_context.rng.gen_range(0.0f64, 1.0f64)).into()) } pub fn create<'gc>(gc_context: MutationContext<'gc, '_>) -> GcCell<'gc, Object<'gc>> { @@ -245,7 +239,7 @@ mod tests { let math = GcCell::allocate(context.gc_context, create(context.gc_context)); assert_eq!( atan2(avm, context, *math.read(), &[]).unwrap(), - ReturnValue::Immediate(Value::Number(NAN)) + Value::Number(NAN).into() ); assert_eq!( atan2( @@ -255,7 +249,7 @@ mod tests { &[Value::Number(1.0), Value::Null] ) .unwrap(), - ReturnValue::Immediate(Value::Number(NAN)) + Value::Number(NAN).into() ); assert_eq!( atan2( @@ -265,7 +259,7 @@ mod tests { &[Value::Number(1.0), Value::Undefined] ) .unwrap(), - ReturnValue::Immediate(Value::Number(NAN)) + Value::Number(NAN).into() ); assert_eq!( atan2( @@ -275,7 +269,7 @@ mod tests { &[Value::Undefined, Value::Number(1.0)] ) .unwrap(), - ReturnValue::Immediate(Value::Number(NAN)) + Value::Number(NAN).into() ); }); } @@ -286,7 +280,7 @@ mod tests { let math = GcCell::allocate(context.gc_context, create(context.gc_context)); assert_eq!( atan2(avm, context, *math.read(), &[Value::Number(10.0)]).unwrap(), - ReturnValue::Immediate(Value::Number(std::f64::consts::FRAC_PI_2)) + Value::Number(std::f64::consts::FRAC_PI_2).into() ); assert_eq!( atan2( @@ -296,7 +290,7 @@ mod tests { &[Value::Number(1.0), Value::Number(2.0)] ) .unwrap(), - ReturnValue::Immediate(Value::Number(f64::atan2(1.0, 2.0))) + Value::Number(f64::atan2(1.0, 2.0)).into() ); }); } diff --git a/core/src/avm1/movie_clip.rs b/core/src/avm1/movie_clip.rs index 6fcda578c..847abb010 100644 --- a/core/src/avm1/movie_clip.rs +++ b/core/src/avm1/movie_clip.rs @@ -14,10 +14,10 @@ macro_rules! with_movie_clip { |_avm, _context, this, args| -> Result, Error> { if let Some(display_object) = this.read().display_node() { if let Some(movie_clip) = display_object.read().as_movie_clip() { - return Ok(ReturnValue::Immediate($fn(movie_clip, args))); + return Ok($fn(movie_clip, args).into()); } } - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) }, $gc_context, DontDelete | ReadOnly | DontEnum, @@ -34,10 +34,10 @@ macro_rules! with_movie_clip_mut { |_avm, context: &mut UpdateContext<'_, 'gc, '_>, this, args| -> Result, Error> { if let Some(display_object) = this.read().display_node() { if let Some(movie_clip) = display_object.write(context.gc_context).as_movie_clip_mut() { - return Ok(ReturnValue::Immediate($fn(movie_clip, context, display_object, args))); + return Ok($fn(movie_clip, context, display_object, args).into()); } } - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } as crate::avm1::function::NativeFunction<'gc>, $gc_context, DontDelete | ReadOnly | DontEnum, @@ -59,7 +59,7 @@ pub fn overwrite_root<'gc>( this.write(ac.gc_context) .force_set("_root", new_val, EnumSet::new()); - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } pub fn overwrite_global<'gc>( @@ -75,7 +75,7 @@ pub fn overwrite_global<'gc>( this.write(ac.gc_context) .force_set("_global", new_val, EnumSet::new()); - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } pub fn create_movie_object<'gc>(gc_context: MutationContext<'gc, '_>) -> Object<'gc> { @@ -120,18 +120,14 @@ pub fn create_movie_object<'gc>(gc_context: MutationContext<'gc, '_>) -> Object< object.force_set_virtual( "_global", - Executable::Native(|avm, context, _this, _args| { - Ok(ReturnValue::Immediate(avm.global_object(context))) - }), + Executable::Native(|avm, context, _this, _args| Ok(avm.global_object(context).into())), Some(Executable::Native(overwrite_global)), EnumSet::new(), ); object.force_set_virtual( "_root", - Executable::Native(|avm, context, _this, _args| { - Ok(ReturnValue::Immediate(avm.root_object(context))) - }), + Executable::Native(|avm, context, _this, _args| Ok(avm.root_object(context).into())), Some(Executable::Native(overwrite_root)), EnumSet::new(), ); @@ -139,14 +135,14 @@ pub fn create_movie_object<'gc>(gc_context: MutationContext<'gc, '_>) -> Object< object.force_set_virtual( "_parent", Executable::Native(|_avm, _context, this, _args| { - Ok(ReturnValue::Immediate( - this.read() - .display_node() - .and_then(|mc| mc.read().parent()) - .and_then(|dn| dn.read().object().as_object().ok()) - .map(|o| Value::Object(o.to_owned())) - .unwrap_or(Value::Undefined), - )) + Ok(this + .read() + .display_node() + .and_then(|mc| mc.read().parent()) + .and_then(|dn| dn.read().object().as_object().ok()) + .map(|o| Value::Object(o.to_owned())) + .unwrap_or(Value::Undefined) + .into()) }), None, EnumSet::new(), diff --git a/core/src/avm1/object.rs b/core/src/avm1/object.rs index da43f2bf8..c9ca1842e 100644 --- a/core/src/avm1/object.rs +++ b/core/src/avm1/object.rs @@ -57,7 +57,7 @@ impl<'gc> Property<'gc> { ) -> Result, Error> { match self { Property::Virtual { get, .. } => get.exec(avm, context, this, &[]), - Property::Stored { value, .. } => Ok(ReturnValue::Immediate(value.to_owned())), + Property::Stored { value, .. } => Ok(value.to_owned().into()), } } @@ -318,7 +318,7 @@ impl<'gc> Object<'gc> { return value.get(avm, context, this); } - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } /// Delete a given value off the object. @@ -364,7 +364,7 @@ impl<'gc> Object<'gc> { if let Some(function) = &self.function { function.exec(avm, context, this, args) } else { - Ok(ReturnValue::Immediate(Value::Undefined)) + Ok(Value::Undefined.into()) } } @@ -458,7 +458,7 @@ mod tests { .read() .get("not_defined", avm, context, object) .unwrap(), - ReturnValue::Immediate(Value::Undefined) + Value::Undefined.into() ); }) } @@ -609,7 +609,7 @@ mod tests { assert_eq!( object.read().get("virtual", avm, context, object).unwrap(), - ReturnValue::Immediate(Value::Undefined) + Value::Undefined.into() ); assert_eq!( object @@ -620,7 +620,7 @@ mod tests { ); assert_eq!( object.read().get("stored", avm, context, object).unwrap(), - ReturnValue::Immediate(Value::Undefined) + Value::Undefined.into() ); assert_eq!( object @@ -635,9 +635,7 @@ mod tests { #[test] fn test_iter_values() { with_object(0, |_avm, context, object| { - let getter = Executable::Native(|_avm, _context, _this, _args| { - Ok(ReturnValue::Immediate(Value::Null)) - }); + let getter = Executable::Native(|_avm, _context, _this, _args| Ok(Value::Null.into())); object .write(context.gc_context) diff --git a/core/src/avm1/return_value.rs b/core/src/avm1/return_value.rs index 2d3b56aae..e337a7f88 100644 --- a/core/src/avm1/return_value.rs +++ b/core/src/avm1/return_value.rs @@ -6,14 +6,28 @@ use crate::context::UpdateContext; use gc_arena::{Collect, GcCell}; use std::fmt; -/// Represents a value which can be returned immediately or at a later time. +/// Represents the return value of a function call. +/// +/// Since function calls can result in invoking native code or adding a new +/// activation onto the AVM stack, you need another type to represent how the +/// return value will be delivered to you. +/// +/// This function contains a handful of utility methods for deciding what to do +/// with a given value regardless of how it is delivered to the calling +/// function. +/// +/// It is `must_use` - failing to use a return value is a compiler warning. We +/// provide a helper function specifically to indicate that you aren't +/// interested in the result of a call. #[must_use = "Return values must be used"] #[derive(Clone)] pub enum ReturnValue<'gc> { /// Indicates that the return value is available immediately. Immediate(Value<'gc>), - /// Indicates that the return value will be calculated on the stack. + /// Indicates that the return value is the result of a given user-defined + /// function call. The activation record returned is the frame that needs + /// to return to get your value. ResultOf(GcCell<'gc, Activation<'gc>>), /// Indicates that there is no value to return. @@ -125,3 +139,15 @@ impl<'gc> ReturnValue<'gc> { } } } + +impl<'gc> From> for ReturnValue<'gc> { + fn from(val: Value<'gc>) -> Self { + ReturnValue::Immediate(val) + } +} + +impl<'gc> From>> for ReturnValue<'gc> { + fn from(frame: GcCell<'gc, Activation<'gc>>) -> Self { + ReturnValue::ResultOf(frame) + } +}