Make the HostHooks shareable between app and context

[hansl]

Previously it was very difficult to keep a stateful HostHooks, since it needed to be a static reference. Now it can be shared. This is particularly useful for mocking hooks for testing.

This also follows the pattern for the job executor and the module loader.

I did not see any impact on performance.

[codecov]

Codecov Report

Attention: Patch coverage is 90.47619% with 4 lines in your changes missing coverage. Please review.

Project coverage is 53.55%. Comparing base (6ddc2b4) to head (4ec5884).
Report is 356 commits behind head on main.

Files with missing lines	Patch %	Lines
core/engine/src/context/mod.rs	66.66%	2 Missing ⚠️
core/engine/src/builtins/date/mod.rs	97.14%	1 Missing ⚠️
core/engine/src/object/builtins/jsdate.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4141      +/-   ##
==========================================
+ Coverage   47.24%   53.55%   +6.31%     
==========================================
  Files         476      487      +11     
  Lines       46892    48874    +1982     
==========================================
+ Hits        22154    26176    +4022     
+ Misses      24738    22698    -2040     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jedel1043]

IIRC we had Rc<HostHooks> before, but it was determined to be unnecessary since the hooks are not supposed to carry additional data, opting for HostDefined if some special data was needed.

I think it's fine to make it possible to add special data to the hooks, but I'm slightly worried that some subtle bugs could be triggered because the hooks are used at realm creation time.

[jedel1043]

Preemptively approving since this is immediately useful, but it would be good to make a survey on how the host hooks are defined on other engines to be sure that we're not introducing bugs by doing this.

[hansl]

IIRC we had Rc before, but it was determined to be unnecessary since the hooks are not supposed to carry additional data, opting for HostDefined if some special data was needed.

Then we would prevent hooks from ever be mocked, which is in general not a great idea.

it would be good to make a survey on how the host hooks are defined on other engines

I'll have a look at the public API of quickjs, SpiderMonkey and V8, but since they're all in C I'll venture it's all pointers with the undertext that "you need to make sure this outlives the engine", which doesn't work that well in Rust.

[jedel1043]

I'll have a look at the public API of quickjs, SpiderMonkey and V8, but since they're all in C I'll venture it's all pointers with the undertext that "you need to make sure this outlives the engine", which doesn't work that well in Rust.

I was more talking about other Rust based engines: rhai, RustPython, Piccolo. Even wrappers such as rusty_v8 or rquickjs. See what they offer that is similar to "HostHooks" and how they offer them.

[hansl]

Well that's going to be kind of boring :P

• Rhai doesn't have host hooks, it uses the stdlib directly in its modules (e.g. timestamp() calls std::time::Instant::now() directly).
• RustPython is the same (but uses SystemTime, probably because they realized Instant is too restrictive). It does have Settings for initialization but none of those relate to hosting it. It's meant to run python modules, not integrate within an app as a scripting engine.
• Piccolo has no hooks, nothing. It's very barebone.
• rusty_v8 does not seem to support hooks. I'm fairly sure (but not 100%) that v8 itself does at least for clock.
• rquickjs is a bit weirder, but all date/time stuff come from quickjs, there's nothing in the rust counterpart.

So I'll go check more C++ code :)