feat(core): JsRuntime::replace_op

[bartlomieju]

Adds a new API to "JsRuntime" that allows to replace (or overwrite) an existing op
with a new handler.

This is kind of "very unsafe, but I know what I'm doing, please leave me alone" API,
but has actual use cases in:

• [WIP] fix(testing): don't allow test files to call Deno.exit() #12938 - to replace "op_exit" with a handler that terminates execution, to prevent
  calling "Deno.exit()" API in test code
• [WIP] feat: Add "deno jupyter" subcommand #13122 - to replace "op_print" with a handler that forwards all "console.*" messages
  over a socket to a notebook frontend

[satyarohith]

LGTM.

[AaronO]

I'm not in favour of this.

Op-middleware allows wrapping/replacing/disabling ops with a single primitive, e.g:

  let my_ext = Extension::builder()
    .middleware(|name, opfn| match name {
      "op_print" => deno_core::void_op_sync(),
      _ => opfn,
    })
    .build();

Any reason why this pattern doesn't work for your aforementioned use-cases ?

[bnoordhuis]

Side discussion: it might makes sense to someday rewrite these (replace_op, register_op, etc.) to:

pub fn frob_op(&mut self, name: &str, op_fn: Rc<OpFn>) {
  // ...
}

Because right now they turn into a combinatorial explosion of monomorphized calls.

Truthfully, that's probably more relevant for op_sync() and op_async() because they're called way more often than register_op() - approx. 300 vs. 35 calls.

[AaronO]

Mid-term I would be in favour of only allowing ops to be registered declaratively at init via Extensions.

[bnoordhuis]

Any reason why this pattern doesn't work for your aforementioned use-cases ?

The goal of this PR is clearly to replace ops at runtime and without overhead.

[AaronO]

Any reason why this pattern doesn't work for your aforementioned use-cases ?

The goal of this PR is clearly to replace ops at runtime and without overhead.

#13122 and #12938 both replace ops at isolate-boot time. Op-middleware handles precisely this use-case and there's no difference in opcall overhead for the replaced ops.

[bnoordhuis]

At isolate creation time isn't at runtime but maybe that's sufficient for the two linked use cases.

[AaronO]

At isolate creation time isn't at runtime but maybe that's sufficient for the two linked use cases.

It is sufficient for the aforementioned use-cases and dynamically replacing ops at runtime is an anti-pattern IMO unless we're discussing "plugins" but that's off the table.

So in short unless there's a very clear use-case that middleware doesn't already solve, we should not add this API.

I refactored the CLI code in #13223 so @bartlomieju can easily slot in his op_exit override for #12938

[tim-palmer]

Possible additional use-case: #12918

If it's not possible to move op_apply_source_map, op_format_diagnostic and op_format_file_name into deno_runtime due to dependencies on cli then presumably deno_runtime will need alternative implementations that are then replaced in cli?