diff options
| -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 | ||||
| -rw-r--r-- | docs/dev/plugin-class-classification.md | 118 |
6 files changed, 145 insertions, 49 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) diff --git a/docs/dev/plugin-class-classification.md b/docs/dev/plugin-class-classification.md index 7046ea48..1c7aec09 100644 --- a/docs/dev/plugin-class-classification.md +++ b/docs/dev/plugin-class-classification.md @@ -36,7 +36,7 @@ exactly one category. | Category | Entity lives | PHP child process sees | Rust obligation | |---|---|---|---| | `rust-proxy` | Rust | generated proxy stub (methods RPC to Rust) | full-fidelity reproduction; every public/protected method needs an RPC handler | -| `rust-snapshot` | Rust | generated snapshot class (`__rhandle` + eagerly copied fields, getters answer locally) | full-fidelity reproduction; snapshot serializer | +| `rust-snapshot` | Rust | the real class, revived from the object record the wire carries (no handle, getters answer locally) | full-fidelity reproduction; both halves of the value codec | | `contract` | n/a (interface / abstract type) | generated declaration preserving the `extends`/`implements` hierarchy | depends on direction attributes | | `two-world` | both, independent siblings | the real PHP implementation (same FQCN, re-defined or vendor-loaded) | independent Rust implementation; only the seam objects (composer / io / dispatcher) are shared | | `php-native` | PHP | the real, unmodified PHP source | none — Rust may or may not have its own port for internal use, and that port is free to diverge in shape | @@ -59,10 +59,15 @@ entity. #### rust-snapshot -Immutable value objects, e.g. `Link` and the security-advisory family. The -child receives the field values together with an interned `__rhandle`, so -identity (`===`) is preserved while getters answer locally with zero -round-trips. +Immutable value objects, e.g. `Link` and the security-advisory family. A +value has no entity to point at, so it gets no handle and no stub: the wire +carries the object record `serialize()` writes for it, `unserialize()` +revives a genuine instance of the real class without running a constructor, +and getters answer locally with zero round-trips. Identity is not preserved +— two calls of the same getter yield two objects in the child (see +`docs/dev/php-rpc.md`). Only the classes on the codec's closed list cross +this way today (`Link` and the `composer/semver` constraints it holds); the +rest of the category has no artifact yet and is guarded. #### contract @@ -124,8 +129,10 @@ Drives which side needs stubs and which needs adapters. A non-abstract class with a public constructor that is also reachable from the graph (e.g. `JsonFile`, obtainable via `Locker::getJsonFile()` *and* freely `new`ed by plugins). These need a constructor story on the stub (the -stub ctor must RPC a `NewObject` so the entity is allocated Rust-side); the -classifier surfaces them because they are individually design-sensitive. +stub constructor forwards to `__shirabeConstruct` on the runtime service +endpoint, and the Rust side allocates the entity and answers with its +handle); the classifier surfaces them because they are individually +design-sensitive. #### mutable-static @@ -350,8 +357,10 @@ reviewable outcome in the report, not a silent guess. ## Known deviations and open questions The mechanical rules surfaced several points where earlier design prose was -incomplete or a decision is still owed. Each needs an explicit user -decision; the tool keeps them visible instead of resolving them silently. +incomplete or a decision is still owed. Some have been decided since, and +this section records what the implementation does instead; the rest still +need an explicit user decision, and the tool keeps them visible instead of +resolving them silently. ### ProcessExecutor and HttpDownloader are reachable @@ -365,28 +374,55 @@ it as a stateless utility — survives only for plugin-`new`ed instances. This is exactly the dual-instantiation situation the `plugin-constructible` attribute exists to surface. -Proxy-side access to the graph-owned instances (the `getLoop()` route) is -left unimplemented — a `todo!()`-style explicit error, not a silent stub — -until a real plugin demonstrates the need. Note that `executeAsync()` -throws a `LogicException` without the `@internal` `enableAsync()` (called -only by `Loop::__construct`), so plugin-`new`ed instances never reach the -async path anyway. +Both routes raise an explicit error today, and which one gets a story first +is still open. The graph-owned instances cannot be obtained at all: the +`Composer` proxy answers `getLoop()` with an explicit error, and `Loop`, +`ProcessExecutor` and `HttpDownloader` are guarded classes, so a +plugin-`new`ed instance is an explicit error too rather than a second +instance the Rust side never sees. + +An earlier argument for leaving plugin-`new`ed instances alone does not +hold: `executeAsync()` does throw a `LogicException` unless `enableAsync()` +ran first, but `enableAsync()`, `wait()` and `countActiveJobs()` are all +public — `@internal` is a docblock note — so nothing stops a plugin from +driving its own instance through the async path. ### Dual instantiation `Locker::getJsonFile(): JsonFile` makes `JsonFile` reachable, so it is `rust-proxy` + `plugin-constructible` — and plugins `new JsonFile(...)` -constantly. The question is not cosmetic: `ProcessExecutor`, `JsonFile`, -and `Util\Filesystem` being `rust-proxy` is what demotes the VCS/auth -utility belt (`Git`, `GitHub`, `GitLab`, `Bitbucket`, `Svn`, `AuthHelper`, -`RemoteFilesystem`) to `unsupported` — each of them constructs one of those -three internally. `ArrayLoader` is a fourth member: plugins `new -ArrayLoader` constantly, and it drives constructor-plus-setters on the -package classes, so its fate follows theirs. These utilities can become -php-native the moment plugin-`new`ed instances of the trio may live -PHP-locally (or the stub `NewObject` constructor story lands); until the -user decides, the tool reports them as `unsupported` with the constructing -site named. +constantly. The question is not cosmetic: a `rust-proxy` class a plugin +constructs demotes the constructing class, and the demotions cascade. +`ProcessExecutor` is what carries the VCS/auth utility belt to +`unsupported`: `GitHub`, `GitLab`, `Bitbucket` and `Svn` construct one, +`Git` constructs a `Bitbucket`, `AuthHelper` a `GitHub`, and +`RemoteFilesystem` an `AuthHelper`. `JsonFile` takes `ConfigValidator` and +`RepositoryFactory` the same way, and `ArrayLoader` — which plugins `new` +just as constantly, and which drives constructor-plus-setters on the +package classes — takes `VersionSelector` and `VersionBumper`. + +The construction half now exists as a mechanism rather than a question: a +stub constructor forwards to `__shirabeConstruct`, the Rust side allocates +the entity and answers with its handle, and the plugin-`new`ed object is +then the same entity the graph sees. It is filled in per class, driven by +the explicit errors real plugins hit — today `Package`, `CompletePackage`, +the three alias packages, the five solver operations and +`Composer\Util\Filesystem` can be built this way, the last one only without +a `ProcessExecutor` argument (that class has no stub, so the argument could +only be an instance the Rust side never sees). Every other proxied class +answers with an explicit error naming it. + +`JsonFile`, `ArrayLoader` and `ProcessExecutor` are still undecided, so the +classes above stay `unsupported` — but a guard now shadows each of them in +the child, so touching one is an explicit error instead of real code +running against a second instance. + +The demotion rule has not been taught about the construction stories: it +demotes for constructing any `rust-proxy` service, whether or not that +service can now be built. `Composer\Package\Archiver\ArchivableFilesFinder` +is `unsupported` for `new Filesystem` alone, which no longer forks any +state; promoting such a class means feeding the per-class construction +stories back into the rule. ### Process: dual instantiation split by caller @@ -425,13 +461,26 @@ The child process necessarily `require`s the project's real `InstalledVersions`) to autoload plugin code, before any stub could load. A same-FQCN stub cannot coexist; both are overridden to `php-native`. `InstalledVersions::$installed` is nonetheless genuinely shared state — -Rust rewrites `installed.php` on every dump — so the Rust side must push a -reload (`InstalledVersions::reload()`) after installs, or post-install -event handlers read stale data. Whether and when that reload push happens, -and how plugin-class autoloading is split between the two worlds, is not -yet decided; until it is, the `InstalledVersions` state a plugin observes -after a Rust-side dump is undefined. The Rust-side reload site carries a -`TODO(plugin)` marker. +Rust rewrites `installed.php` on every dump — and the Rust side pushes the +reload: right after writing the file it calls +`__shirabe_installed_versions_reload` in the worker, which mirrors the tail +of `FilesystemRepository::write` (the unconditional +`InstalledVersions::reload($versions)` plus the reflection-based +`selfDir`/`installedIsLocalDir` restore). It is skipped when no worker runs, +and inside the worker when the class is not even autoloadable there; in +both cases there is no observer code either. + +Plugin classes are autoloaded from the Rust side. The loaders +`PluginManager::registerPackage` builds live in the Rust process, and the +worker's autoloader asks them for a file through the runtime service +endpoint (`__shirabe_find_file`); the files-autoload entries and the +`_composer_tmp` class-rename path run in the worker. One gap remains: the +Rust `spl_autoload_register` is a no-op and `ClassLoader::register` keeps +one loader per vendor directory (matching upstream's `$registeredLoaders`) +where PHP's autoload stack keeps every registered loader, so a second +plugin loader registered under the same vendor directory evicts the first, +and a class of the earlier plugin that was never loaded becomes +unresolvable. ### ConsoleIO leaks world-2 objects @@ -440,7 +489,8 @@ seam: `getTable(): Table` / `getProgressBar(): ProgressBar`, and its constructor takes `InputInterface`/`OutputInterface`/`HelperSet` — none of which can cross the wire as values. The stub needs a bespoke story (e.g. a local Table bound to a proxying `OutputInterface`); until one is designed, -both members raise explicit errors. +both members raise explicit errors, as does the constructor — the Rust side +has no construction story for this class. ## The classifier tool |