Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

adding new __replaceFunctionImplementation_unsafe built-in #2533

Closed
wants to merge 3 commits into from

Conversation

NTillmann
Copy link
Contributor

@NTillmann NTillmann commented Sep 6, 2018

Release notes: providing __replaceFunctionImplementation_unsafe built-in

This new built-in allows replacing the method implementation (capture environment, body, ...) of source functions.

As a result, all method calls executed by the Prepack interpreter will be redirected,
and the replacement will carry over to the prepacked code.

Release notes: providing __replaceFunctionBody built-in

This new built-in allows replacing the method body of functions with source code.
As a result, all method calls executed by the Prepack interpreter will be redirected,
and the replacement will carry over to the prepacked code.
@hermanventer
Copy link
Contributor

This seems simple enough, but I am lost trying to figure out why you need it. I'm also not quite sure it is correct. I don't have any particular reason to believe it is incorrect, but it is not OBVIOUSLY correct either.

@@ -551,6 +551,58 @@ export default function(realm: Realm): void {
})
);

// Helper function that replaces the body of a source function with another source function body.
// Note that this function affects un-tracked state, so care must be taken
// that this helper function is executed at the right time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the right time? What is the wrong time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on what one wants. In my scenario, I'll do this replacement before the function in question will be invoked for the first time. (Ideally, I'd just replace the function object itself and be done with it without the need for __replaceFunctionBody; but unfortunately, the function is stored in a way that I can't update the reference to it.)

);
}

// relevant properties for functionValue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe in general to just update all of these properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure, but it's fine for my purposes...

@@ -551,6 +551,58 @@ export default function(realm: Realm): void {
})
);

// Helper function that replaces the body of a source function with another source function body.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not just replacing the body but the parameters too and much else beside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body and everything that the body may depend on. Do you have a better name in mind?

@NTillmann NTillmann changed the title adding new __replaceFunctionBody built-in adding new __replaceFunctionImplementation_unsafe built-in Sep 6, 2018
Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTillmann has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants