Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): split no-empty-object-type out from ban-types and no-empty-interfaces #8977

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bcfbb03
feat(eslint-plugin): split no-empty-object-type rule out from ban-typ…
JoshuaKGoldberg Apr 23, 2024
173251c
Mention no-props
JoshuaKGoldberg Apr 23, 2024
b83cb50
Update packages/eslint-plugin/docs/rules/no-empty-object-type.mdx
JoshuaKGoldberg Apr 23, 2024
39870f1
Update packages/eslint-plugin/docs/rules/no-empty-object-type.mdx
JoshuaKGoldberg Apr 23, 2024
f90ab3f
Allow in intersections, and touch up docs per suggestions
JoshuaKGoldberg Apr 24, 2024
0a5c2bd
Update packages/eslint-plugin/docs/rules/no-empty-object-type.mdx
JoshuaKGoldberg Apr 24, 2024
c01f10e
Trimming
JoshuaKGoldberg Apr 25, 2024
f021d26
Update snapshots
JoshuaKGoldberg Apr 25, 2024
ca7fb0f
Finish removing Record
JoshuaKGoldberg Apr 26, 2024
fad7a5e
Correct phrasing on Object
JoshuaKGoldberg Apr 26, 2024
e8a2d7f
Merge with no-empty-interface
JoshuaKGoldberg Apr 30, 2024
3971d71
nit the tip
JoshuaKGoldberg Apr 30, 2024
d33a246
Explicit report message for interfaces
JoshuaKGoldberg May 8, 2024
28ed70d
Add in-type-alias-with-name
JoshuaKGoldberg May 9, 2024
9b6dd1a
Switched to more general allowWithName
JoshuaKGoldberg May 9, 2024
e0bbd1d
Merge branch 'v8'
JoshuaKGoldberg May 9, 2024
b7a70b8
snapshot -u
JoshuaKGoldberg May 9, 2024
713404a
snapshot -u
JoshuaKGoldberg May 9, 2024
218d8e5
Fixed up unit tests
JoshuaKGoldberg May 9, 2024
9987c2b
Update packages/eslint-plugin/docs/rules/no-empty-object-type.mdx
JoshuaKGoldberg May 12, 2024
bd764ef
docs touchups from Kirk
JoshuaKGoldberg May 12, 2024
7dc88d7
replacedBy too
JoshuaKGoldberg May 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 0 additions & 9 deletions packages/eslint-plugin/docs/rules/ban-types.mdx
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -36,9 +36,6 @@ const func: Function = () => 1;
// use safer object types
const lowerObj: Object = {};
const capitalObj: Object = { a: 'string' };

const curly1: {} = 1;
const curly2: {} = { a: 'string' };
```

</TabItem>
Expand All @@ -58,9 +55,6 @@ const func: () => number = () => 1;
// use safer object types
const lowerObj: object = {};
const capitalObj: { a: string } = { a: 'string' };

const curly1: number = 1;
const curly2: Record<'a', string> = { a: 'string' };
```

</TabItem>
Expand All @@ -74,9 +68,6 @@ The default options provide a set of "best practices", intended to provide safet
- Avoid the `Function` type, as it provides little safety for the following reasons:
- It provides no type safety when calling the value, which means it's easy to provide the wrong arguments.
- It accepts class declarations, which will fail when called, as they are called without the `new` keyword.
- Avoid the `Object` and `{}` types, as they mean "any non-nullish value".
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
- This is a point of confusion for many developers, who think it means "any object type".
- See [this comment for more information](https://github.com/typescript-eslint/typescript-eslint/issues/2063#issuecomment-675156492).

<details>
<summary>Default Options</summary>
Expand Down
4 changes: 4 additions & 0 deletions packages/eslint-plugin/docs/rules/no-empty-interface.mdx
kirkwaiblinger marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -61,3 +61,7 @@ interface Baz extends Foo, Bar {}
## When Not To Use It

If you don't care about having empty/meaningless interfaces, then you will not need this rule.

## Related To

- [`no-empty-object-type`](./no-empty-object-type.mdx)
65 changes: 65 additions & 0 deletions packages/eslint-plugin/docs/rules/no-empty-object-type.mdx
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,65 @@
---
description: 'Disallow accidentally using the "empty object" type.'
---

import Tabs from '@theme/Tabs';
import TabItem from '@theme/TabItem';

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/no-empty-object-type** for documentation.

The `{}`, or "empty object" type in TypeScript is a common source of confusion for developers unfamiliar with TypeScript's structural typing.
`{}` represents any _non-nullish value_, including literals like `0` and `""`:

```ts
let anyNonNullishValue: {} = 'Intentionally allowed by TypeScript.';
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
```

Often, developers writing `{}` actually mean either:

- `object`: representing any _object_ value
- `unknown`: representing any value _other than `null` and `undefined`_
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

To avoid confusion around the `{}` type allowing non-object values, this rule bans usage of the `{}` type.

:::tip
If you do have a use case for an API allowing any _non-nullish value_, you can always use an [ESLint disable comment](https://eslint.org/docs/latest/use/configure/rules#using-configuration-comments-1) or [disable the rule in your ESLint config](https://eslint.org/docs/latest/use/configure/rules#using-configuration-files-1).
:::
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

## Examples

<Tabs>
<TabItem value="❌ Incorrect">

```ts
let anyObject: {};
let anyValue: {};
let emptyObject: {};
```

</TabItem>
<TabItem value="✅ Correct">

```ts
let anyObject: object;
let anyValue: unknown;
let emptyObject: Record<string, never>;
```

</TabItem>
</Tabs>

## When Not To Use It

If your code commonly needs to represent the _"any non-nullish value"_ type, this rule may not be for you.
Projects that extensively use type operations such as conditional types and mapped types oftentimes benefit from disabling this rule.

## Further Reading

- [Enhancement: [ban-types] Split the {} ban into a separate, better-phrased rule](https://github.com/typescript-eslint/typescript-eslint/issues/8700)
- [The Empty Object Type in TypeScript](https://www.totaltypescript.com/the-empty-object-type-in-typescript)
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

## Related To

- [`no-empty-interface`](./no-empty-interface.mdx)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like these two rules are really one, maybe as an option allowInterface for declaration merging

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I see them as separate rules. They're definitely related but I think the justifications for disabling either are very different.

Copy link
Member

@Josh-Cena Josh-Cena Apr 25, 2024

Choose a reason for hiding this comment

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

interface T {} is not any different from type T = {} by itself, though. The motivation says:

An empty interface in TypeScript does very little: any non-nullable value is assignable to {}. Using an empty interface is often a sign of programmer error, such as misunderstanding the concept of {} or forgetting to fill in fields.

That is exactly applicable to empty object types. There is only slightly more reason to disable no-empty-interface because it allows declaration merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. I'll need to think on that a bit... Just confirming, you're suggesting we should merge no-empty-interface into this rule for v8, too?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Member

Choose a reason for hiding this comment

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

the difference between the two is that no-empty-interface allows you to ban interface Foo extends Bar {} in favour of type Foo = Bar.
However a similar rule doesn't make sense for type aliases.

But there's no reason we couldn't merge them and keep the allowSingleExtends option just for interfaces.

but you're JoshG isn't wrong - the scope is different because the current form bans {} anywhere - not just in type aliases.

I personally can't think of a reason you'd ever want to have one banned, but not the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, I don't have a strong preference either way, and it sounds like the current consensus is leaning towards unification? Unless @kirkwaiblinger has strong input I'm up for unifying.

Copy link
Member

Choose a reason for hiding this comment

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

no strong opinions on this!

Copy link
Member

Choose a reason for hiding this comment

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

I will say, though, it does feel like yet another breaking change for users just to preserve the existing behavior. So, I guess "no strong opinion" defaults to not merging in this case 🤷

Copy link
Member

Choose a reason for hiding this comment

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

it does feel like yet another breaking change for users just to preserve the existing behavior

I would have said the opposite - it's a breaking change TO merge.
Cos if you don't merge - people are just switching from ban-types to this new rule.
But if you merge they ALSO need to switch from no-empty-interface to this new rule.

1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.ts
Expand Up @@ -55,6 +55,7 @@ export = {
'no-empty-function': 'off',
'@typescript-eslint/no-empty-function': 'error',
'@typescript-eslint/no-empty-interface': 'error',
'@typescript-eslint/no-empty-object-type': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/no-extra-non-null-assertion': 'error',
'@typescript-eslint/no-extraneous-class': 'error',
Expand Down
Expand Up @@ -18,6 +18,7 @@ export = {
'@typescript-eslint/no-base-to-string': 'error',
'@typescript-eslint/no-duplicate-enum-values': 'error',
'@typescript-eslint/no-duplicate-type-constituents': 'error',
'@typescript-eslint/no-empty-object-type': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/no-extra-non-null-assertion': 'error',
'@typescript-eslint/no-floating-promises': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/recommended.ts
Expand Up @@ -15,6 +15,7 @@ export = {
'no-array-constructor': 'off',
'@typescript-eslint/no-array-constructor': 'error',
'@typescript-eslint/no-duplicate-enum-values': 'error',
'@typescript-eslint/no-empty-object-type': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/no-extra-non-null-assertion': 'error',
'@typescript-eslint/no-misused-new': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/strict-type-checked.ts
Expand Up @@ -24,6 +24,7 @@ export = {
'@typescript-eslint/no-duplicate-enum-values': 'error',
'@typescript-eslint/no-duplicate-type-constituents': 'error',
'@typescript-eslint/no-dynamic-delete': 'error',
'@typescript-eslint/no-empty-object-type': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/no-extra-non-null-assertion': 'error',
'@typescript-eslint/no-extraneous-class': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/strict.ts
Expand Up @@ -19,6 +19,7 @@ export = {
'@typescript-eslint/no-array-constructor': 'error',
'@typescript-eslint/no-duplicate-enum-values': 'error',
'@typescript-eslint/no-dynamic-delete': 'error',
'@typescript-eslint/no-empty-object-type': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/no-extra-non-null-assertion': 'error',
'@typescript-eslint/no-extraneous-class': 'error',
Expand Down
31 changes: 4 additions & 27 deletions packages/eslint-plugin/src/rules/ban-types.ts
Expand Up @@ -73,7 +73,10 @@ const defaultTypes: Types = {
message: 'Use bigint instead',
fixWith: 'bigint',
},

Object: {
message: 'Use object instead',
fixWith: 'object',
},
Function: {
message: [
'The `Function` type accepts any function-like value.',
Expand All @@ -82,32 +85,6 @@ const defaultTypes: Types = {
'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.',
].join('\n'),
},

// object typing
Object: {
Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't explicitly discuss Object, but I think it makes sense in the spirit of this change to switch it to being treated as another banned uppercase alias. I've never seen people confuse Object for {}.

message: [
'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.',
'- If you want a type meaning "any object", you probably want `object` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
'- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead.',
].join('\n'),
suggest: ['object', 'unknown', 'NonNullable<unknown>'],
},
'{}': {
message: [
'`{}` actually means "any non-nullish value".',
'- If you want a type meaning "any object", you probably want `object` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
'- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.',
'- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead.',
].join('\n'),
suggest: [
'object',
'unknown',
'Record<string, never>',
'NonNullable<unknown>',
],
},
};

export const TYPE_KEYWORDS = {
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -36,6 +36,7 @@ import noDuplicateTypeConstituents from './no-duplicate-type-constituents';
import noDynamicDelete from './no-dynamic-delete';
import noEmptyFunction from './no-empty-function';
import noEmptyInterface from './no-empty-interface';
import noEmptyObjectType from './no-empty-object-type';
import noExplicitAny from './no-explicit-any';
import noExtraNonNullAssertion from './no-extra-non-null-assertion';
import noExtraneousClass from './no-extraneous-class';
Expand Down Expand Up @@ -160,6 +161,7 @@ export default {
'no-dynamic-delete': noDynamicDelete,
'no-empty-function': noEmptyFunction,
'no-empty-interface': noEmptyInterface,
'no-empty-object-type': noEmptyObjectType,
'no-explicit-any': noExplicitAny,
'no-extra-non-null-assertion': noExtraNonNullAssertion,
'no-extraneous-class': noExtraneousClass,
Expand Down
45 changes: 45 additions & 0 deletions packages/eslint-plugin/src/rules/no-empty-object-type.ts
@@ -0,0 +1,45 @@
import type { TSESLint } from '@typescript-eslint/utils';

import { createRule } from '../util';

export default createRule({
name: 'no-empty-object-type',
meta: {
type: 'suggestion',
docs: {
description: 'Disallow accidentally using the "empty object" type',
recommended: 'recommended',
},
hasSuggestions: true,
messages: {
banEmptyObjectType: [
'The `{}` ("empty object") type allows any non-nullish value, including literals like `0` and `""`.',
"- If that's what you want, disable this lint rule with an inline comment or in your ESLint config.",
'- If you want a type meaning "any object", you probably want `object` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
'- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.',
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
].join('\n'),
replaceEmptyObjectType: 'Replace `{}` with `{{replacement}}`.',
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
},
schema: [],
},
defaultOptions: [],
create(context) {
return {
'TSTypeLiteral[members.length=0]'(node): void {
context.report({
messageId: 'banEmptyObjectType',
node,
suggest: ['object', 'unknown', 'Record<string, never>'].map(
replacement => ({
data: { replacement },
messageId: 'replaceEmptyObjectType',
fix: (fixer): TSESLint.RuleFix =>
fixer.replaceText(node, replacement),
}),
),
});
},
};
},
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 0 additions & 38 deletions packages/eslint-plugin/tests/rules/ban-types.test.ts
Expand Up @@ -137,44 +137,6 @@ ruleTester.run('ban-types', rule, {
],
options,
},
{
code: 'let a: Object;',
output: null,
errors: [
{
messageId: 'bannedTypeMessage',
data: {
name: 'Object',
customMessage: [
' The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.',
'- If you want a type meaning "any object", you probably want `object` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
'- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead.',
].join('\n'),
},
line: 1,
column: 8,
suggestions: [
{
messageId: 'bannedTypeReplacement',
data: { name: 'Object', replacement: 'object' },
output: 'let a: object;',
},
{
messageId: 'bannedTypeReplacement',
data: { name: 'Object', replacement: 'unknown' },
output: 'let a: unknown;',
},
{
messageId: 'bannedTypeReplacement',
data: { name: 'Object', replacement: 'NonNullable<unknown>' },
output: 'let a: NonNullable<unknown>;',
},
],
},
],
options: [{}],
},
{
code: 'let aa: Foo;',
output: null,
Expand Down