diff options
Diffstat (limited to 'crates')
| -rw-r--r-- | crates/shirabe-php-rpc/php/worker.php | 8 | ||||
| -rw-r--r-- | crates/shirabe-php-rpc/src/value.rs | 7 | ||||
| -rw-r--r-- | crates/shirabe/src/event_dispatcher/event_dispatcher.rs | 22 | ||||
| -rw-r--r-- | crates/shirabe/src/plugin/php_plugin_proxy.rs | 29 | ||||
| -rw-r--r-- | crates/shirabe/src/util/filesystem.rs | 10 |
5 files changed, 61 insertions, 15 deletions
diff --git a/crates/shirabe-php-rpc/php/worker.php b/crates/shirabe-php-rpc/php/worker.php index 164d73ea..6ed5df40 100644 --- a/crates/shirabe-php-rpc/php/worker.php +++ b/crates/shirabe-php-rpc/php/worker.php @@ -267,6 +267,12 @@ final class ShirabeRpcRuntime /** * Converts a decoded wire value: handle descriptor arrays become live objects. A * materialized value arrives as a real instance already, revived by unserialize(). + * + * TODO(type-model): a descriptor travels in-band as a plain array, so its exact key set is + * the only thing separating it from plugin data of the same shape. Every check below except + * __pclass matches on the reserved key alone, so an array a plugin built with a __rhandle or + * __phandle key of its own is read as a handle rather than kept as data. The Rust half + * (`decode_handle` in `src/value.rs`) matches the whole key set, and diverges the other way. */ public static function fromWire($value) { @@ -806,7 +812,7 @@ ShirabeRpcRuntime::$dispatch = [ }, // An already-fulfilled promise for a Rust-side call whose PHP signature declares // PromiseInterface. The Rust future ran to completion before this is called, so there is - // nothing left to defer; see .ken/plugin-arch/design.md §10.1.6. + // nothing left to defer. '__shirabe_resolved_promise' => static function ($args) { if (!function_exists('React\\Promise\\resolve')) { throw new RuntimeException( diff --git a/crates/shirabe-php-rpc/src/value.rs b/crates/shirabe-php-rpc/src/value.rs index c19c386f..7203c9fb 100644 --- a/crates/shirabe-php-rpc/src/value.rs +++ b/crates/shirabe-php-rpc/src/value.rs @@ -612,6 +612,13 @@ fn parse_quoted(payload: &[u8], pos: &mut usize, terminator: u8) -> anyhow::Resu } /// Recognizes the reserved handle-descriptor arrays by their exact key sets. +/// +/// TODO(type-model): a descriptor travels in-band as a plain array, so its exact key set is the +/// only thing separating it from plugin data of the same shape. An array that carries a reserved +/// key without matching a descriptor's whole key set is user data and should decode as an array; +/// it is a decode error here instead, and that is the fatal lane, so a plugin passing +/// `['__rhandle' => 1]` to a proxied method takes the channel down with it. The PHP half +/// (`fromWire` in `php/worker.php`) diverges the other way, matching on the reserved key alone. fn decode_handle(entries: &IndexMap<Vec<u8>, PluginValue>) -> anyhow::Result<Option<PluginValue>> { let get = |key: &[u8]| entries.get(key); diff --git a/crates/shirabe/src/event_dispatcher/event_dispatcher.rs b/crates/shirabe/src/event_dispatcher/event_dispatcher.rs index fb3ad5f4..a5a238bd 100644 --- a/crates/shirabe/src/event_dispatcher/event_dispatcher.rs +++ b/crates/shirabe/src/event_dispatcher/event_dispatcher.rs @@ -751,11 +751,12 @@ impl EventDispatcher { // PHP hosts the user's Command class in a throwaway, bare // `Symfony\Component\Console\Application` (NOT Composer's Application), - // built by a generated snippet running inside the worker. The command's - // output is captured in a BufferedOutput and written back through the - // dispatcher's IO; upstream hands the live output object of `$this->io` - // to `$app->run()` instead, so only the interleaving with concurrent - // writes differs. + // built by a generated snippet running inside the worker. + // + // TODO(plugin): the BufferedOutput has to go. The command has to run + // against the real output stream, the way upstream hands the live output + // object of `$this->io` to `$app->run()`; collecting the output and + // writing the buffer back once the run has returned is not a substitute. let args = additional_args .iter() .map(|arg| ProcessExecutor::escape(arg)) @@ -775,6 +776,12 @@ impl EventDispatcher { } else { output_interface::VERBOSITY_NORMAL }; + // TODO(error-model): the snippet's try/catch does not reproduce upstream's + // boundary. Upstream wraps `$app->run()` alone and catches `\Exception`, so + // an `\Error` from the command, and a throw from `new $className(...)`, + // both escape without the "terminated with an exception" line. Here the + // catch is `\Throwable`, and a constructor throw leaves the snippet as a + // `Throw` reply from `__shirabe_eval`, so the line is written either way. let snippet = format!( r#" $className = {class_name_lit}; @@ -821,6 +828,8 @@ try {{ )?; let result = match outcome { Ok(value) => value.to_php_mixed()?, + // TODO(error-model): `throw.exception_class` is dropped, so the class + // upstream rethrows unchanged collapses to RuntimeException here. Err(throw) => { self.io.write_error3( &format!( @@ -846,6 +855,9 @@ try {{ self.io.write3(&command_output, false, crate::io::NORMAL); } if let Some(throw) = result.as_array().and_then(|map| map.get("throw")) { + // TODO(error-model): the snippet reports `get_class($e)` as the first + // field and nothing reads it, so the class upstream rethrows unchanged + // collapses to RuntimeException here. let fields = throw .as_list() .expect("the eval snippet reports exceptions as a list"); diff --git a/crates/shirabe/src/plugin/php_plugin_proxy.rs b/crates/shirabe/src/plugin/php_plugin_proxy.rs index c9d6986b..8469593f 100644 --- a/crates/shirabe/src/plugin/php_plugin_proxy.rs +++ b/crates/shirabe/src/plugin/php_plugin_proxy.rs @@ -732,9 +732,10 @@ fn dispatch_config_method( /// The download manager's contract is asynchronous on both sides: PHP declares a /// `PromiseInterface` return, Rust an `async fn`. The Rust future is driven to completion here -/// and its value handed back as an already-settled React promise, which is the synchronous -/// fallback of the promise design (`.ken/plugin-arch/design.md` §10.1.6) rather than the -/// deferred resolution a concurrent engine would allow. +/// and its value handed back as an already-settled React promise. +/// +/// TODO(async): the boundary has no representation for a promise that is still pending, so the +/// deferred resolution the PHP contract allows collapses into a blocking wait here. fn dispatch_download_manager_method( dm: &std::rc::Rc<std::cell::RefCell<dyn crate::downloader::DownloadManagerInterface>>, method_name: &str, @@ -1006,12 +1007,15 @@ fn dispatch_event_dispatcher_method( if dispatcher.borrow_mut().has_event_listeners(&probe) { // TODO(plugin): dispatching a worker-constructed event through the Rust-side // dispatcher needs the event object (and the console input it carries) proxied - // back into this process; until then only the no-listener case — where - // upstream's dispatch is observably a no-op returning 0 — is supported. + // back into this process, so only the no-listener case is answered here. return Err(runtime_throw(format!( "dispatching `{name}` from the plugin process is not supported yet while listeners are registered for it" ))); } + // TODO(plugin): answering 0 here skips what `do_dispatch` does before it reaches the + // listener loop, and upstream does both regardless of the listener count: the + // `COMPOSER_DEBUG_EVENTS` trace line, and `push_event`'s circular-call detection + // (a nested dispatch of the same event name throws there even with no listeners). Ok(PluginValue::Int(0)) } other => Err(runtime_throw(format!( @@ -1610,6 +1614,15 @@ fn dispatch_package_method( } // `RootAliasPackage` overrides each of these to write through to its alias target, and // `RootPackage` reaches the same base state either way, so both go through the interface. + // + // TODO(type-model): choosing the body for the concrete variant belongs on `AnyPackage`, + // not here. The stub surface already decides which classes carry a method, so the + // `as_*_mut` accessors' "not available on an alias package" arms are unreachable for a + // method the alias stubs do not declare, and what is left is a per-variant dispatch that + // this guard only approximates: it covers `RootPackage` as well, where the extra hop is + // equivalent only while that impl keeps delegating to the base package, and nothing + // checks it. Method names repeated in the base-package arm below make the answer depend + // on arm order, and a new variant compiles into the wrong body without a diagnostic. "setRequires" | "setDevRequires" | "setConflicts" | "setProvides" | "setReplaces" | "setAutoload" | "setDevAutoload" | "setSuggests" | "setExtra" if package.borrow().is_root() => @@ -2497,8 +2510,10 @@ impl PhpInstallerProxy { /// The `?PromiseInterface` half of the installer contract. The Rust callers await the /// installer's effects rather than chaining continuations, so a returned promise is drained /// here: an already-settled one yields its value (or raises its rejection reason), while a - /// still-pending one is an explicit error — resolving it would need the concurrent execution - /// engine the boundary does not have (`.ken/plugin-arch/design.md` §10.1.6). + /// still-pending one is an explicit error. + /// + /// TODO(async): resolving a still-pending promise would need a concurrent execution engine + /// the boundary does not have. fn promise_result(&self, method: &str, value: PluginValue) -> anyhow::Result<Option<PhpMixed>> { let handle = match value { PluginValue::Null => return Ok(None), diff --git a/crates/shirabe/src/util/filesystem.rs b/crates/shirabe/src/util/filesystem.rs index 8408a8a9..b20f5f48 100644 --- a/crates/shirabe/src/util/filesystem.rs +++ b/crates/shirabe/src/util/filesystem.rs @@ -533,8 +533,12 @@ impl Filesystem { prefer_relative: bool, ) -> String { if !self.is_absolute_path(from) || !self.is_absolute_path(to) { - // PHP throws InvalidArgumentException - // Returning early-formatted Result is not possible without changing signature; panic to surface in tests. + // TODO(error-model): PHP throws InvalidArgumentException. Plugins reach this method + // through the RPC proxy, where a relative path is ordinary input rather than a + // programming error, and this panic kills the process instead of reaching their + // catch block; the plugin dispatcher repeats the check for that reason. Returning + // `anyhow::Result` from here and from `find_shortest_path_code` removes both the + // panic and the duplicated check. panic!( "{}", format!("$from ({}) and $to ({}) must be absolute paths.", from, to) @@ -596,6 +600,8 @@ impl Filesystem { prefer_relative: bool, ) -> String { if !self.is_absolute_path(from) || !self.is_absolute_path(to) { + // TODO(error-model): as in `find_shortest_path` — PHP throws + // InvalidArgumentException, and this panic cannot reach a plugin's catch block. panic!( "{}", format!("$from ({}) and $to ({}) must be absolute paths.", from, to) |