Skip to content

NonEmptyObject: Handle index signatures conservatively#1419

Open
KevinBigham wants to merge 1 commit into
sindresorhus:mainfrom
KevinBigham:fix/non-empty-object-index-signature
Open

NonEmptyObject: Handle index signatures conservatively#1419
KevinBigham wants to merge 1 commit into
sindresorhus:mainfrom
KevinBigham:fix/non-empty-object-index-signature

Conversation

@KevinBigham

Copy link
Copy Markdown

Addresses #821.

NonEmptyObject currently accepts {} for types whose only members are index signatures:

import type {NonEmptyObject} from 'type-fest';

interface CommonArguments {
	[filter: string]: NonEmptyObject<{[argument: string]: string | number | undefined}>;
}

const args: CommonArguments = {foo: {}}; // ← accepted on main; should fail

Why

A type whose only members are index signatures cannot statically express "at least one dynamic key" in TypeScript. The previous definition fell through to RequireAtLeastOne<T, keyof T>, which for keyof T = string mapped back to an index signature and trivially accepted {}.

What changed

Re-derive the key set via OmitIndexSignature so:

  • Pure-index-signature types fail closed and resolve to never.
  • For mixed types, HasRequiredKeys / RequireAtLeastOne operate on the explicit-key subset, so index signatures no longer mask non-emptiness enforcement.

Tests

Added to test-d/non-empty-object.ts:

  1. Pure string index signature → never.
  2. Pure number index signature → never.
  3. Exact NonEmptyObject fails for objects with dynamic properties #821 repro ({foo: {}}) is rejected.
  4. Mixed (index signature + required explicit key) still returns the original type.
  5. Mixed (index signature + only optional explicit keys) requires at least one of the explicit optional keys.

npm test passes locally.

Addresses sindresorhus#821.

A type whose only members are index signatures cannot statically express "at least one dynamic key" in TypeScript, so `NonEmptyObject` previously degenerated to the input type and silently accepted `{}`. Re-derive the key set via `OmitIndexSignature` so:

- Pure-index-signature types fail closed and resolve to `never`.
- Mixed types are driven off the explicit-key subset, so index signatures no longer mask `HasRequiredKeys` / `RequireAtLeastOne`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1dd996796

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +40 to +41
keyof OmitIndexSignature<T> extends never
? never

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve any inputs in NonEmptyObject

When callers instantiate NonEmptyObject<any> (or a generic that resolves to any), this new guard treats any like a pure index-signature type: OmitIndexSignature<any> maps over string | number | symbol and removes every key, so the conditional resolves to never. That regresses from the previous definition, which preserved any, and can turn permissive downstream generic APIs into impossible types.

Useful? React with 👍 / 👎.

@sindresorhus

Copy link
Copy Markdown
Owner

I think this accidentally changes NonEmptyObject<any> to never.

The new explicit-key guard treats any like an index-signature-only object: after omitting index signatures there are no keys left, so it falls into the never branch. That’s a regresion from main, where NonEmptyObject<any> stays any, and it also differs from RequireAtLeastOne<any>, which explicitly preserves any.

I manually checked this with tsd: this assertion passes on main and fails on this branch with “argument type never”.

expectType<any>({} as NonEmptyObject<any>);

Can you special-case any before the index-signature-only guard?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants