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

Fix Internal Object Methods #591

Closed
11 tasks
HalidOdat opened this issue Jul 23, 2020 · 2 comments · Fixed by #1354
Closed
11 tasks

Fix Internal Object Methods #591

HalidOdat opened this issue Jul 23, 2020 · 2 comments · Fixed by #1354
Assignees
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics discussion Issues needing more discussion E-Medium Medium difficulty problem execution Issues or PRs related to code execution help wanted Extra attention is needed
Milestone

Comments

@HalidOdat
Copy link
Member

HalidOdat commented Jul 23, 2020

What is wrong with the current implementation?
Currently are only set for ordinary objects, but not for array, string etc. Every object in the spec gets these internal methods an example is [[DefineOwnProperty]] for ordinary objects it calls OrdinaryDefineOwnProperty but for Array [[DefineOwnProperty]] is different when the property that is being defined is "length" it calls ArraySetLength this is what shrinks the array. This is what causes #557 bug.

We should avoid trait objects since it forces dynamic dispatch, and it also is not a viable solution.
With trait objects we are trying to solve an inheritance problem, rust does not have a way to describe a base class where we have base class (with the the properties) and all the object (Array, etc) inherit from it. Also this would force dynamic dispatch this would slow down the code and it would not allow some compiler optimizations, we know the exact number of object types we don`t have do do this.

My proposed changes:
This is how I intended to do it, we have the internal method for example [[set]], we also have the type definitions of what the object is Array, Map, etc. We have all we need to make this a static dispatch. So something like this:

impl Object {
	fn ordinary_set(&self /* , ... other parameters */) -> bool {
		// implementation
	}
	fn set(&mut self /* , ... other parameters */ ) -> bool {
		match self.data {
			ObjectData::Array => {
				// Arrays [[set]]
			}
			ObjectData::String(string) => {
				// Strings [[set]]
			}
			// This is for ordinay objects and for objects
			// that don't define a special implementation of [[set]]
			_ => self.ordinary_set(/* , ... args*/)
		}
	}
} 

This way of implementing allows the compiler to do some optimization that can't be done in dynamic dispatch.

What do you think? any other better ideas? or improvements?

@HalidOdat HalidOdat added bug Something isn't working help wanted Extra attention is needed technical debt discussion Issues needing more discussion blocked Waiting for another code change builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution and removed technical debt labels Jul 23, 2020
@HalidOdat HalidOdat self-assigned this Jul 23, 2020
@HalidOdat HalidOdat added E-Hard Hard difficulty problem E-Medium Medium difficulty problem and removed blocked Waiting for another code change E-Hard Hard difficulty problem labels Jul 24, 2020
@Razican
Copy link
Member

Razican commented Jan 11, 2021

Maybe it makes sense to have a trait with a default implementation of the methods, but override them when needed.

This was referenced Jan 15, 2021
@jedel1043
Copy link
Member

jedel1043 commented Jun 20, 2021

Another option would be to have a trait generalizing the behaviour of any object like:

trait CommonObject {
    fn ordinary_get(&self) -> usize { 
        64
    }
    
    fn get(&self) -> usize {
        self.ordinary_get()
    };
}

Then we would implement the internal function for every object directly in the implementation of the trait:

struct OrdinaryObject;
impl CommonObject for OrdinaryObject {}

struct ExoticObject;
impl CommonObject for ExoticObject {
    fn get(&self) -> usize {
        self.ordinary_get() * 100  
    }
} 

And finally, we would implement a static dispatch with an enum:

enum Object {
    OrdinaryObject(OrdinaryObject),
    ExoticObject(ExoticObject) // Maybe make ExoticObject an Enum or put all Exotic Objects below
}

impl Object { 
    fn get(&self) -> usize {
        match self {
            Object::OrdinaryObject(obj) => obj.get(),
            Object::ExoticObject(obj) => obj.get(),
        }
    }
}

A little boilerplatey but it works.

pub fn main() {
    
    let objarr = [Object::OrdinaryObject(OrdinaryObject), Object::ExoticObject(ExoticObject)];
    
    for obj in objarr {
        println!("{}", obj.get()); 
    }
    // prints:
    // 64
    // 6400
}

Rust playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics discussion Issues needing more discussion E-Medium Medium difficulty problem execution Issues or PRs related to code execution help wanted Extra attention is needed
Projects
None yet
3 participants