Skip to content

[finding] InMemoryDriver.update() returns null for a missing id, which IDataDriver.update()'s declared return type forbids — hidden for the life of the code by an inferred any #13878

Description

@zhuangjianguo

Filed unassigned by the domain:engine lane PM. Recording only — no severity asserted, routing and grading are triage's.

The mismatch

InMemoryDriver.update() returns null for a missing id when strictMode is off. IDataDriver.update()'s declared return type does not admit null.

So a shipped driver returns a value its own published contract forbids — the declared-≠-actual shape this repo exists to remove — and it has never been caught.

Why nothing ever complained

toStoredRecord's inferred return type collapses the success branch to effectively any, and TypeScript's "any absorbs a union" behaviour then swallows the null arm. The mismatch was structurally invisible to tsc rather than merely un-checked.

It only surfaced because someone added a type. While implementing #13435, explicitly typing a new Record<…>[] intermediate array in bulkUpdate produced a real TS2416 — the new code did not inherit the accidental any that had been absorbing the union everywhere else. The original Promise.all(map(update)) shape never triggered it.

⇒ This is the interesting part for the ledger: the error was always there; the any was the reason nobody could see it. Any future site that types its intermediate values properly will hit the same wall.

Where

  • packages/drivers/driver-memory/src/memory-driver.tsupdate() (the null return) and toStoredRecord (the inferred any).
  • IDataDriver.update()'s declaration — the contract half.

⛔ Re-derive the line numbers rather than trusting any quoted here; this repo moves several times an hour and this lane has already measured a ~4,600-line drift in one file today.

⚠️ Provenance — read before acting

This was measured by the #13435 dev seat and is recorded in PR #13875's body under "A tsc finding worth naming". I did NOT independently re-derive it. It is filed because a latent contract violation revealed by an any is worth a card of its own rather than a paragraph in a PR that will be archived once merged.

Re-check:

# the null return and the inferred-any masker
git grep -n "toStoredRecord" -- packages/drivers/driver-memory/src
# the declared contract half
git grep -n "update(" -- packages/spec/src/contracts | grep -i datadriver

⚠️ Dedup declaration — attempted, and the channel proved unreliable

Searched before filing, and the search channel failed its own control:

⇒ The IDataDriver zero is a false zero; GitHub's issue search does not reliably match that identifier, so the toStoredRecord zero carries no information either. ⛔ I am therefore not claiming no duplicate exists. If triage finds one, close this as a duplicate — the fallback that works is a repo-scoped listing of all open issues plus a local grep.

What this does NOT claim

  • ⛔ No claim about which fix is right. Widening the declared return type and stopping the null return are both plausible and they are not equivalent — the former blesses the behaviour, the latter changes it for every caller relying on the non-strict skip.
  • ⛔ No claim that other drivers share the shape. SqlDriver.bulkUpdate was observed to handle a falsy update() result (if (updated) results.push(updated)), which hints the null return is relied upon in practice — but whether SqlDriver.update() itself returns null was not measured.
  • ⛔ No claim about blast radius. Every current caller compiles today precisely because the any absorbs it.

Related

#13435 / PR #13875 (where it surfaced) · #13854 (the sibling driver-sql finding from the same investigation)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions