Skip to content

fix: NonEmptyObject should ignore index signatures when checking for required keys#1464

Open
syu-toutousai wants to merge 4 commits into
sindresorhus:mainfrom
syu-toutousai:fix/non-empty-object-index-signature
Open

fix: NonEmptyObject should ignore index signatures when checking for required keys#1464
syu-toutousai wants to merge 4 commits into
sindresorhus:mainfrom
syu-toutousai:fix/non-empty-object-index-signature

Conversation

@syu-toutousai

Copy link
Copy Markdown

When T has an index signature like [filter: string]: SomeType, HasRequiredKeys<T> returns true because it counts the index signature as a required key. This causes NonEmptyObject to pass through the type as-is even when no concrete keys are present.

Using OmitIndexSignature<T> before checking for required keys ensures index signatures are excluded from the check.

Before:

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

// This incorrectly passes type checking
const commonArguments: CommonArguments = {
  foo: {}
}

After: The above correctly fails type checking.

Fixes #821

PayPal: n6085530@gmail.com

…required keys

When T has an index signature, HasRequiredKeys returns true even if no
concrete keys are present. Use OmitIndexSignature to exclude index
signatures before checking for required keys.

Fixes sindresorhus#821
@sindresorhus

Copy link
Copy Markdown
Owner

I don’t think this actually fixes the repro from the issue. It changes the HasRequiredKeys check, but the fallback is still RequireAtLeastOne<T, keyof T>, and for a pure string index signature keyof T is just string | number. That still allows {}.

I checked this manually with:

type Arguments = NonEmptyObject<{
	[argument: string]: string | number | undefined;
}>;

// @ts-expect-error
const argumentsValue: Arguments = {};

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

// @ts-expect-error
const commonArguments: CommonArguments = {
	foo: {},
};

TypeScript reports both @ts-expect-error comments as unused, so teh empty objects are still accepted. A regression test with this exact shape should fail on the current change.

@syu-toutousai

Copy link
Copy Markdown
Author

I have updated the PR to address the dynamic index signature issue.

Specifically:

  • Omit index signatures using .
  • If the type resolves to no explicit keys (e.g. pure index signatures), it will fail closed and resolve to (since TypeScript cannot statically express "at least one dynamic key" without explicit keys, avoiding silently accepting ).
  • Special-cased to resolve back to (so behaves as expected).
  • Otherwise, enforce required keys or fall back to requiring at least one of the optional explicit keys.

…Object

Adds the exact repro cases from sindresorhus's review comment:
- NonEmptyObject<{[key: string]: V}> must resolve to never
- {} must not be assignable to NonEmptyObject<PureIndexSignature>
- {} must not be assignable to nested NonEmptyObject<PureIndexSignature>
@syu-toutousai

Copy link
Copy Markdown
Author

I've pushed a follow-up commit that adds your exact repro cases as regression tests:

// @ts-expect-error - {} must NOT be assignable to NonEmptyObject<PureIndexSignature>
const _badPure: NonEmptyObject<{[argument: string]: string | number | undefined}> = {};

// @ts-expect-error - {} must NOT be assignable to the nested form
const _badNested: {[filter: string]: NonEmptyObject<{[argument: string]: string | number | undefined}>} = {foo: {}};

Both @ts-expect-error annotations are now genuinely needed — {} is rejected for pure index signature types because keyof OmitIndexSignature<T> is never, so NonEmptyObject<T> resolves to never, and {} is not assignable to never. Please let me know if you'd like any further changes.

@Linell

Linell commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for iterating on this — the updated approach reads much better. I checked out the branch: npm test passes and the #821 repro now correctly fails.

A few things I noticed while testing:

  1. Unions: NonEmptyObject<{a: string} | {b: number}> now resolves to never (on main it returned the union unchanged, so {a: 'x'} was accepted). OmitIndexSignature distributes over the union, but keyof of a union intersects the members' keys, so the new extends never guard fires for any disjoint-key union. Removing the guard doesn't quite work either (unions with an all-optional member would accept {} again) — distributing over union members might be the cleanest fix.
  2. Record<string, X> now rejects everything: on main it accepted both {} (the bug) and {a: 1}; now both fail since the type is never. The fail-closed trade-off is documented, but since valid non-empty records break too, it seems worth an explicit call from the maintainer.
  3. The new IsAny branch isn't covered by tests yet — any/never cases would guard it. It also looks like the internal IfNotAnyOrNever helper (used by RequireAtLeastOne) would be a drop-in here.

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.

NonEmptyObject fails for objects with dynamic properties

3 participants