Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions changelog.d/10817-manifest-drift-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
**Manifest drift fix + gate relocation (#463/#512):** `API_MANIFEST` was missing 15 entries that
exist in `NATIVE_MODULE_TABLE` — 5 from `b36554a2d7` (node:http client `rawHeaders`/
`httpVersionMajor`/`httpVersionMinor`/`complete`, #10467/#10468/#10469) and 10 from `64ca0ebfe7`
(net.Socket surface cluster: `prependListener`/`prependOnceListener`/`pipe`/`unpipe`/`writable`/
`readable`/`writableEnded`/`readableEnded`/`_writableState`/`_readableState`,
#10441/#10442/#10444/#10465). Added to `crates/perry-api-manifest/src/entries/part_1.rs` and
`part_4.rs`, matching the file's existing convention of representing a zero-arg
`NativeMethodCall` property read as `ApiKind::Method { has_receiver: true, .. }`.

Nothing caught this drift when it landed because `every_dispatch_entry_has_manifest_counterpart`
lived in `crates/perry-codegen/tests/manifest_consistency.rs`, an integration suite CI's
`e2e-scoped` only runs per-PR when the diff names that file — the one file a PR that merely adds
`NATIVE_MODULE_TABLE` rows has no reason to touch. Moved the check to a `#[cfg(test)]` unit test
(`crates/perry-codegen/src/manifest_consistency.rs`) so it runs on every `cargo-test` invocation
regardless of diff scope; `perry-codegen` already depends on `perry-api-manifest` as an ordinary
dependency, so no new dependency edge was needed. The integration test's copy was removed (its
trigger condition was a strict subset of the unit test's); the file's other checks are unchanged.
20 changes: 20 additions & 0 deletions crates/perry-api-manifest/src/entries/part_1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,26 @@ pub(crate) const API_MANIFEST_PART_1: &[ApiEntry] = &[
method("net", "getX509Certificate", true, Some("Socket")),
method("net", "getPeerX509Certificate", true, Some("Socket")),
method("net", "setKeyCert", true, Some("Socket")),
// #10441/#10442 — front-inserting listener variants (net.Socket is
// an EventEmitter), and #10444 pipe/unpipe (net.Socket is a
// stream.Duplex). Absent entirely pre-fix: a typed `net.Socket`
// receiver fell through to a plain property read for these names
// and got `undefined` instead of dispatching.
method("net", "prependListener", true, Some("Socket")),
method("net", "prependOnceListener", true, Some("Socket")),
method("net", "pipe", true, Some("Socket")),
method("net", "unpipe", true, Some("Socket")),
// #10465 — writable/readable/writableEnded/readableEnded/
// _writableState/_readableState state accessors. No class_filter
// in the dispatch table (native_table/net_events.rs) — same
// class_filter: None shape the generic `stream` module rows use
// for their own writable/readable/writableEnded/readableEnded.
method("net", "writable", true, None),
method("net", "readable", true, None),
method("net", "writableEnded", true, None),
method("net", "readableEnded", true, None),
method("net", "_writableState", true, None),
method("net", "_readableState", true, None),
// Issue #1123 followup — `net.Server` instance methods backing
// `createServer(...).listen/.close/.address/.on`. Mirrors the
// shape of the http-server rows at entries.rs:2298. The
Expand Down
11 changes: 11 additions & 0 deletions crates/perry-api-manifest/src/entries/part_4.rs
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,16 @@ pub(crate) const API_MANIFEST_PART_4: &[ApiEntry] = &[
method("http", "statusMessage", true, Some("IncomingMessage")),
method("http", "headers", true, Some("IncomingMessage")),
method("http", "trailers", true, Some("IncomingMessage")),
// #10467 — client-side rawHeaders/httpVersionMajor/httpVersionMinor/
// complete bare-name accessors (paired with the __get_rawHeaders row
// below). Previously only the __get_* HIR-rewrite targets existed for
// httpVersionMajor/httpVersionMinor/complete and rawHeaders had no
// manifest row at all, so a typed `IncomingMessage` receiver reading
// the bare property missed dispatch entirely.
method("http", "rawHeaders", true, Some("IncomingMessage")),
method("http", "httpVersionMajor", true, Some("IncomingMessage")),
method("http", "httpVersionMinor", true, Some("IncomingMessage")),
method("http", "complete", true, Some("IncomingMessage")),
method("http", "setStatus", true, Some("ServerResponse")),
method("http", "getStatus", true, Some("ServerResponse")),
method("http", "__get_method", true, Some("IncomingMessage")),
Expand All @@ -898,6 +908,7 @@ pub(crate) const API_MANIFEST_PART_4: &[ApiEntry] = &[
method("http", "__get_statusMessage", true, Some("IncomingMessage")),
method("http", "__get_headers", true, Some("IncomingMessage")),
method("http", "__get_trailers", true, Some("IncomingMessage")),
method("http", "__get_rawHeaders", true, Some("IncomingMessage")),
method("http", "__get_statusCode", true, Some("ServerResponse")),
method("http", "__set_statusCode", true, Some("ServerResponse")),
method("http", "__set_statusMessage", true, Some("ServerResponse")),
Expand Down
5 changes: 5 additions & 0 deletions crates/perry-codegen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ pub(crate) mod lower_call;
pub(crate) mod lower_conditional;
pub(crate) mod lower_string_concat;
pub(crate) mod lower_string_method;
/// #463/#512 dispatch-table/manifest drift check — see the module docs
/// for why this moved here from an integration test (#10668's 15-row
/// drift, which nothing caught until it surfaced on an unrelated PR).
#[cfg(test)]
mod manifest_consistency;
pub mod module;
pub mod nanbox;
#[cfg(feature = "llvm-inprocess")]
Expand Down
83 changes: 83 additions & 0 deletions crates/perry-codegen/src/manifest_consistency.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//! In-crate coverage for the #463/#512 manifest-drift check that
//! `crates/perry-codegen/tests/manifest_consistency.rs` used to own alone.
//!
//! # Why this exists (a gate that could not fail on the change that broke it)
//!
//! `every_dispatch_entry_has_manifest_counterpart` compares two tables:
//! `NATIVE_MODULE_TABLE` (this crate's dispatch table, walked through
//! [`crate::iter_native_method_signatures`]) against
//! `perry_api_manifest::API_MANIFEST`. By construction it can be tripped by
//! an edit to EITHER table — but as an integration test under
//! `crates/perry-codegen/tests/`, CI's `e2e-scoped` only runs an integration
//! suite per-PR when the diff names it (CLAUDE.md: "Integration suites under
//! `crates/*/tests/*.rs` run per-PR only when the diff names them"). The one
//! file a drifting PR will almost never touch is `manifest_consistency.rs`
//! itself — a PR that adds `NATIVE_MODULE_TABLE` rows has no reason to edit
//! the test file that checks them. #10668 (node:http client response
//! surface + net.Socket surface cluster) landed 15 such rows, and nothing
//! caught the drift until it happened to surface on a later, unrelated PR
//! that ran in the same CI job.
//!
//! This is a fifth way a gate can be unable to fail, distinct from the four
//! CLAUDE.md already tracks under "Four ways a gate can be unable to fail":
//! there the gate itself is broken (`continue-on-error`, not required,
//! cancelled, or its subject never runs). Here the gate is fine — it runs,
//! it can go red, it's required — and the change class it exists to guard
//! simply never triggers it, because trigger condition and subject are
//! disjoint by construction.
//!
//! Moving the assertion into a `#[cfg(test)]` unit test inside this crate
//! puts it on the `cargo-test`-visible per-PR gate unconditionally
//! (CLAUDE.md again: "Prefer putting acceptance coverage in
//! `cargo-test`-visible unit tests (#5960)"). `perry-codegen` already
//! depends on `perry-api-manifest` as an ordinary (non-dev) dependency — see
//! this crate's `Cargo.toml` — so reaching `API_MANIFEST` from here adds no
//! new dependency edge.
//!
//! The integration test's copy of this same check was removed rather than
//! kept as a duplicate: its trigger condition is a strict subset of this
//! module's (this module runs on every PR; the integration test ran only on
//! PRs that touched its own file), so a passing integration-test copy could
//! never catch anything this module doesn't already catch first. The
//! integration file's other checks — `manifest_param_counts_match_dispatch_table`,
//! the reverse-direction module/binding checks — are unaffected and stay
//! where they are; see that file's header for why.

use perry_api_manifest::{ApiKind, API_MANIFEST};

#[test]
fn every_dispatch_entry_has_manifest_counterpart() {
let mut missing: Vec<String> = Vec::new();

for sig in crate::iter_native_method_signatures() {
// Look for a manifest entry on the same (module, name) where
// the kind is Method with matching has_receiver. class_filter
// mismatches across rows of the same (module, method) pair are
// expected — the dispatch table specializes by class, the
// manifest does not.
let hit = API_MANIFEST.iter().any(|e| {
e.module == sig.module
&& e.name == sig.method
&& matches!(
e.kind,
ApiKind::Method { has_receiver, .. } if has_receiver == sig.has_receiver
)
});
if !hit {
let cls = sig.class_filter.unwrap_or("-");
missing.push(format!(
"{}::{} (has_receiver={}, class_filter={})",
sig.module, sig.method, sig.has_receiver, cls
));
}
}

assert!(
missing.is_empty(),
"API_MANIFEST is missing {} entry/entries that exist in NATIVE_MODULE_TABLE:\n {}\n\n\
Add the missing rows to crates/perry-api-manifest/src/entries.rs — \
drift here would make the unimplemented-API check (#463) error on real implementations.",
missing.len(),
missing.join("\n ")
);
}
63 changes: 21 additions & 42 deletions crates/perry-codegen/tests/manifest_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,31 @@
//! Every row of `NATIVE_MODULE_TABLE` (the static dispatch table in
//! `lower_call.rs`) must have a counterpart entry in `API_MANIFEST`,
//! otherwise the unimplemented-API check would error on a real
//! implementation. This file covers two drifts:
//! implementation.
//!
//! 1. `every_dispatch_entry_has_manifest_counterpart` — by name only;
//! catches new dispatch rows that nobody added to the manifest.
//! 2. `manifest_param_counts_match_dispatch_table` (#512) — for
//! **`every_dispatch_entry_has_manifest_counterpart` moved** to
//! `perry_codegen::manifest_consistency`, a `#[cfg(test)]` unit test in
//! `crates/perry-codegen/src/manifest_consistency.rs` — see that module's
//! doc comment for the full reasoning. Short version: as an integration test
//! here, it only ran per-PR when the diff named this file, which is the one
//! file a drifting PR (one that only adds `NATIVE_MODULE_TABLE` rows) has no
//! reason to touch. #10668 landed 15 such rows and nothing caught it until an
//! unrelated PR happened to run this suite. The unit test runs on every
//! `cargo-test` invocation regardless of which files the diff touches.
//!
//! This file still covers:
//!
//! 1. `manifest_param_counts_match_dispatch_table` (#512) — for
//! auto-derivable rows (`has_receiver: false`, no class filter) the
//! manifest's `params.len()` must match the dispatch table's args
//! arity, so the generated `.d.ts` doesn't claim a different shape
//! than what codegen actually accepts.
//! than what codegen actually accepts. (Same disjoint-trigger gap as
//! the moved check applies here too — left as an integration test for
//! now, out of scope for this pass.)
//! 2. `every_native_module_has_at_least_one_manifest_entry` (#513) — the
//! reverse-direction structural check.
//! 3. `cjs_style_node_builtins_have_default_entries`.
//! 4. `every_well_known_binding_has_manifest_entry` (#513).
//!
//! Class-filtered duplicates collapse to one manifest entry — the
//! manifest tracks "is this method known on this module?", not the
Expand All @@ -20,43 +36,6 @@
use perry_api_manifest::{ApiKind, ParamSpec, TypeSpec, API_MANIFEST};
use perry_codegen::iter_native_method_signatures;

#[test]
fn every_dispatch_entry_has_manifest_counterpart() {
let mut missing: Vec<String> = Vec::new();

for sig in iter_native_method_signatures() {
// Look for a manifest entry on the same (module, name) where
// the kind is Method with matching has_receiver. class_filter
// mismatches across rows of the same (module, method) pair are
// expected — the dispatch table specializes by class, the
// manifest does not.
let hit = API_MANIFEST.iter().any(|e| {
e.module == sig.module
&& e.name == sig.method
&& matches!(
e.kind,
ApiKind::Method { has_receiver, .. } if has_receiver == sig.has_receiver
)
});
if !hit {
let cls = sig.class_filter.unwrap_or("-");
missing.push(format!(
"{}::{} (has_receiver={}, class_filter={})",
sig.module, sig.method, sig.has_receiver, cls
));
}
}

assert!(
missing.is_empty(),
"API_MANIFEST is missing {} entry/entries that exist in NATIVE_MODULE_TABLE:\n {}\n\n\
Add the missing rows to crates/perry-api-manifest/src/entries.rs — \
drift here would make the unimplemented-API check (#463) error on real implementations.",
missing.len(),
missing.join("\n ")
);
}

/// #512: for every auto-derivable dispatch row (no receiver, no class
/// filter) the manifest's `params` length must match the dispatch
/// table's args length, AND each `NA_STR` in the dispatch table must
Expand Down
Loading