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): [require-types-exports] add new rule #8443

Open
wants to merge 81 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
81 commits
Select commit Hold shift + click to select a range
9a0c28a
feat(eslint-plugin): [require-types-exports] add new rule
StyleShit Feb 12, 2024
7778868
wip
StyleShit Feb 12, 2024
12fce5b
wip
StyleShit Feb 12, 2024
d62f86c
lint
StyleShit Feb 13, 2024
0ebebd2
wip
StyleShit Feb 13, 2024
30e9aa9
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Feb 13, 2024
bfee791
spelling...
StyleShit Feb 13, 2024
b309b51
wip
StyleShit Feb 13, 2024
6aa6446
wip
StyleShit Feb 13, 2024
892c368
wip
StyleShit Feb 13, 2024
0e8e58f
tuple generic
StyleShit Feb 13, 2024
f4018a8
wip
StyleShit Feb 13, 2024
89a8344
wip
StyleShit Feb 13, 2024
b2138e3
wip
StyleShit Feb 15, 2024
feedefd
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Feb 15, 2024
1161db0
wip
StyleShit Feb 16, 2024
d9875b3
refactor
StyleShit Feb 16, 2024
6338202
make it shorter & more readable
StyleShit Feb 16, 2024
428b2c1
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Feb 16, 2024
2cb3455
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Apr 24, 2024
1812e37
fix nested types in functions
StyleShit Apr 24, 2024
4bee779
fix docs
StyleShit Apr 24, 2024
26e7be7
add inferred return type test case
StyleShit Apr 24, 2024
e57985a
stupidly check for variable types
StyleShit Apr 24, 2024
cbb784c
support default exported variable
StyleShit Apr 24, 2024
2f2dfa4
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Apr 24, 2024
37a0171
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit May 19, 2024
6fb274a
update docs
StyleShit May 19, 2024
4672fe1
wip
StyleShit May 19, 2024
c79b5cb
wip
StyleShit May 19, 2024
279055a
wip
StyleShit May 19, 2024
7897abf
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit May 28, 2024
7082960
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 2, 2024
2f81933
improve types
StyleShit Jun 2, 2024
0f788d2
improve type reference search
StyleShit Jun 2, 2024
6cec0f5
don't report types from default library
StyleShit Jun 2, 2024
497957a
getTypeName
StyleShit Jun 2, 2024
702d4d0
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 4, 2024
700ff85
move utils out of the closure
StyleShit Jun 4, 2024
9a155b3
support namespaced types
StyleShit Jun 5, 2024
8d0d000
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 5, 2024
b65f9c4
fix namespaced imports
StyleShit Jun 5, 2024
078e24a
WIP
StyleShit Jun 5, 2024
ed23162
wip
StyleShit Jun 5, 2024
ac224eb
fix propertykey tests
StyleShit Jun 5, 2024
417cc91
ReturnType test
StyleShit Jun 5, 2024
62f1466
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 28, 2024
ae1b87c
wip
StyleShit Jun 28, 2024
d227408
collect type references recursively
StyleShit Jun 28, 2024
dca52d0
lib types
StyleShit Jun 28, 2024
ed30856
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 28, 2024
15fc51c
style
StyleShit Jun 28, 2024
59eda58
wip
StyleShit Jun 29, 2024
fc0858a
wip
StyleShit Jun 29, 2024
9d24c64
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 29, 2024
94a98eb
configs
StyleShit Jun 29, 2024
dee0fe4
don't report generic params in call expression
StyleShit Jun 29, 2024
0804b24
improve function types collection
StyleShit Jun 30, 2024
a0a4944
wip
StyleShit Jun 30, 2024
66a0aff
wip
StyleShit Jun 30, 2024
b67e1f9
remove `getVariable`
StyleShit Jun 30, 2024
9891e78
infer return type from return statements
StyleShit Jun 30, 2024
cb90d43
wip
StyleShit Jun 30, 2024
479f593
wip
StyleShit Jun 30, 2024
08f2ce2
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jun 30, 2024
e86427f
wip
StyleShit Jun 30, 2024
a61d49f
wip
StyleShit Jun 30, 2024
121f475
wip
StyleShit Jun 30, 2024
f3f8518
wip
StyleShit Jun 30, 2024
ab837b4
custom traversal
StyleShit Jul 7, 2024
1641272
some tests
StyleShit Jul 7, 2024
fd56a1c
add missing tests
StyleShit Jul 7, 2024
b0613d5
report default exported call expression
StyleShit Jul 7, 2024
b9f1148
report types used within exported types
StyleShit Jul 7, 2024
0415b60
fix false positives due to ordering
StyleShit Jul 7, 2024
a0c236e
change message
StyleShit Jul 7, 2024
3d5d695
wip
StyleShit Jul 7, 2024
2e76ce6
fix some reports
StyleShit Jul 7, 2024
88713cb
support keyof & typeof
StyleShit Jul 8, 2024
ff2c0a8
simplify tsconfig
StyleShit Jul 8, 2024
2fd30aa
Merge remote-tracking branch 'typescript-eslint/main' into feat/requi…
StyleShit Jul 8, 2024
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
80 changes: 80 additions & 0 deletions packages/eslint-plugin/docs/rules/require-types-exports.mdx
StyleShit marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

From #8443 (comment):

cases like this

That file's .d.ts equivalent is:

type A = number;
type C = boolean;

export function func(): { to: A, and: C } | number | string;

...so it should only report A and C as needing to be exported, no? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, sorry, I've mistyped the type names there, it should be A & C as you said,

Anyway... I didn't explain myself properly (it was after a lot of work on this, my brain basically melted 🧠)

  1. What I meant is that it (at least in my head and with my current implementation) becomes overly complicated to collect types from things like this:
type A = number;

const obj: { path: { to: { key: A } } } = {
  path: { to: { key: 1 } }
};

export function func() {
  return obj.path.to.key;
}

Because I need to somehow follow the whole path of the MemberExpression and try to infer its value from the root object. Maybe that's fine and easy and I'm just making it hard in my mind? 🤷

  1. Should A be reported in this case?
type A = number;

const obj = {
  value: 1 as A,
};

export function func() {
  return obj.value;
}

Copy link
Member

Choose a reason for hiding this comment

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

Because I need to somehow follow the whole path of the MemberExpression

Indeed. I think this is going to need recursive logic to traverse through object literals like that.

Should A be reported in this case?

I'd think so. I don't think we should care about what a type's actual type is - just that it's exported. We'd certainly want to report in this case:

type A = { inner: string };

const obj = {
  value: { inner: "" } as A,
};

// Inferred return type of func(): A
export function func() {
  return obj.value;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I understand the difference here

If the type is an alias to a built-in primitive type we shouldn't report, and if it's an object or something we should?

I mean... yeah, TS won't emit A in the .d.ts file in my example, so if I understand you correctly, I'll need to re-create TS's logic here about whether a type should appear in the function's declaration, right?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jul 6, 2024

Choose a reason for hiding this comment

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

I'll need to re-create TS's logic here about whether a type should appear in the function's declaration, right?

Not that, I'm suggesting the opposite. What I'm proposing is: any type that's visible in other exports should be exported itself. That would hold true for string, number | string, (number | string)[], { inner: string }, etc.

I'm suggesting this in part to try to make this rule easier to implement and maintain 😄. Having to determine whether some type is emitted is more work.

But also, folks sometimes use types for opaque values:

// TODO: Switch to a branded type once we decide on a library
type Id = string;
export declare function getId(): Id;

In this case, I think it'd match the intent of the rule to say Id should be exported.

...but anyway, what I'm trying to suggest here is the simplest approach: if something declared in the AST is used in an export, we assume it should be exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that's exactly what I thought!

Now I realize that I'm stupid and misunderstood you
You said "I think so", while I read it (for some reason...) "I think no" - totally different meaning 😅

Yeah, that's much easier

Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
---
description: 'Require exporting types that are used in exported entities.'
---

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/require-types-exports** for documentation.

When exporting entities from a module, it is recommended to export also all the
types that are used in their declarations. This is useful for consumers of the
module, as it allows them to use the types in their own code without having to
use utility types like [`Parameters`](https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype)
or [`ReturnType`](https://www.typescriptlang.org/docs/handbook/utility-types.html#returntypetype)
in order to extract the types from your code.

## Examples

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

```ts
type Arg = string;
type Result = number;

export function strLength(arg: Arg): Result {
return arg.length;
}

interface Fruit {
name: string;
color: string;
}

export const getFruitName = (fruit: Fruit) => fruit.name;

enum Color {
Red = 'red',
Green = 'green',
Blue = 'blue',
}

export declare function getRandomColor(): Color;
```

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

```ts
export type Arg = string;
export type Result = number;

export function strLength(arg: Arg): Result {
return arg.length;
}

export interface Fruit {
name: string;
color: string;
}

export const getFruitName = (fruit: Fruit) => fruit.name;

export enum Color {
Red = 'red',
Green = 'green',
Blue = 'blue',
}

export declare function getRandomColor(): Color;
```

</TabItem>
</Tabs>

## When Not To Use It

When you don't want to enforce exporting types that are used in exported functions declarations.
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ export = {
'@typescript-eslint/require-array-sort-compare': 'error',
'require-await': 'off',
'@typescript-eslint/require-await': 'error',
'@typescript-eslint/require-types-exports': 'error',
'@typescript-eslint/restrict-plus-operands': 'error',
'@typescript-eslint/restrict-template-expressions': 'error',
'no-return-await': 'off',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/disable-type-checked.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export = {
'@typescript-eslint/promise-function-async': 'off',
'@typescript-eslint/require-array-sort-compare': 'off',
'@typescript-eslint/require-await': 'off',
'@typescript-eslint/require-types-exports': 'off',
'@typescript-eslint/restrict-plus-operands': 'off',
'@typescript-eslint/restrict-template-expressions': 'off',
'@typescript-eslint/return-await': 'off',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export = {
'@typescript-eslint/prefer-return-this-type': 'error',
'require-await': 'off',
'@typescript-eslint/require-await': 'error',
'@typescript-eslint/require-types-exports': 'error',
'@typescript-eslint/restrict-plus-operands': [
'error',
{
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/strict-type-checked.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export = {
'@typescript-eslint/prefer-ts-expect-error': 'error',
'require-await': 'off',
'@typescript-eslint/require-await': 'error',
'@typescript-eslint/require-types-exports': 'error',
'@typescript-eslint/restrict-plus-operands': [
'error',
{
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ import promiseFunctionAsync from './promise-function-async';
import quotes from './quotes';
import requireArraySortCompare from './require-array-sort-compare';
import requireAwait from './require-await';
import requireTypesExports from './require-types-exports';
import restrictPlusOperands from './restrict-plus-operands';
import restrictTemplateExpressions from './restrict-template-expressions';
import returnAwait from './return-await';
Expand Down Expand Up @@ -272,6 +273,7 @@ export default {
quotes: quotes,
'require-array-sort-compare': requireArraySortCompare,
'require-await': requireAwait,
'require-types-exports': requireTypesExports,
'restrict-plus-operands': restrictPlusOperands,
'restrict-template-expressions': restrictTemplateExpressions,
'return-await': returnAwait,
Expand Down
241 changes: 241 additions & 0 deletions packages/eslint-plugin/src/rules/require-types-exports.ts
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
import type { Reference } from '@typescript-eslint/scope-manager';
import { DefinitionType } from '@typescript-eslint/scope-manager';
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';

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

type MessageIds = 'requireTypeExport';

type FunctionNode =
| TSESTree.FunctionDeclaration
| TSESTree.TSDeclareFunction
| TSESTree.ArrowFunctionExpression
| TSESTree.FunctionExpression;

type TypeReference = Reference & {
identifier: {
parent: TSESTree.TSTypeReference;
};
};

export default createRule<[], MessageIds>({
name: 'require-types-exports',
meta: {
type: 'suggestion',
docs: {
description: 'Require exporting types that are used in exported entities',
recommended: 'strict',
},
messages: {
requireTypeExport: 'Expected type "{{ name }}" to be exported',
Copy link
Member

Choose a reason for hiding this comment

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

[Docs] What do you think of this, for a more informative error?

Suggested change
requireTypeExport: 'Expected type "{{ name }}" to be exported',
requireTypeExport: '"{{ name }}" is used in other exports from this file, so it should also be exported.',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it
Done

},
schema: [],
},
defaultOptions: [],
create(context) {
const typeReferences = new Set<TypeReference>();
const externalizedTypes = new Set<string>();
const reportedTypes = new Set<string>();

function collectTypeReferences(node: TSESTree.Program): void {
const scope = context.sourceCode.getScope(node);

scope.references.forEach(r => {
if (
r.resolved?.isTypeVariable &&
r.identifier.parent.type === AST_NODE_TYPES.TSTypeReference
) {
typeReferences.add(r as TypeReference);
}
});
}

function collectImportedTypes(node: TSESTree.ImportSpecifier): void {
externalizedTypes.add(node.local.name);
}

function collectExportedTypes(
node:
| TSESTree.TSTypeAliasDeclaration
| TSESTree.TSInterfaceDeclaration
| TSESTree.TSEnumDeclaration,
): void {
externalizedTypes.add(node.id.name);
}

function visitExportedFunctionDeclaration(
node: (
| TSESTree.ExportNamedDeclaration
| TSESTree.DefaultExportDeclarations
) & {
declaration: TSESTree.FunctionDeclaration | TSESTree.TSDeclareFunction;
},
): void {
checkFunctionTypes(node.declaration);
}

function visitExportedVariableDeclaration(
node: TSESTree.ExportNamedDeclaration & {
declaration: TSESTree.VariableDeclaration;
},
): void {
for (const declaration of node.declaration.declarations) {
if (declaration.init?.type === AST_NODE_TYPES.ArrowFunctionExpression) {
checkFunctionTypes(declaration.init);
} else {
checkVariableTypes(declaration);
}
}
}

function visitDefaultExportedArrowFunction(
node: TSESTree.ExportDefaultDeclaration & {
declaration: TSESTree.ArrowFunctionExpression;
},
): void {
checkFunctionTypes(node.declaration);
}

function visitDefaultExportedIdentifier(
node: TSESTree.DefaultExportDeclarations & {
declaration: TSESTree.Identifier;
},
): void {
const scope = context.sourceCode.getScope(node);
const variable = scope.set.get(node.declaration.name);

if (!variable) {
return;
}

for (const def of variable.defs) {
if (def.type !== DefinitionType.Variable || !def.node.init) {
continue;
}

if (
def.node.init.type === AST_NODE_TYPES.ArrowFunctionExpression ||
def.node.init.type === AST_NODE_TYPES.FunctionExpression
) {
checkFunctionTypes(def.node.init);
} else {
checkVariableTypes(def.node);
}
}
}

function checkFunctionTypes(node: FunctionNode): void {
const scope = context.sourceCode.getScope(node);

scope.through
.map(ref => ref.identifier.parent)
.filter(
(node): node is TSESTree.TSTypeReference =>
node.type === AST_NODE_TYPES.TSTypeReference,
)
.forEach(checkTypeNode);
}

function checkVariableTypes(
node: TSESTree.LetOrConstOrVarDeclarator,
): void {
typeReferences.forEach(r => {
// TODO: Probably not the best way to do it...
if (isLocationOverlapping(r.identifier.loc, node.loc)) {
checkTypeNode(r.identifier.parent);
}
});
}

function checkTypeNode(node: TSESTree.TSTypeReference): void {
const name = getTypeName(node);

if (!name) {
// TODO: Report the whole function? Is this case even possible?
return;
}

const isExternalized = externalizedTypes.has(name);
const isReported = reportedTypes.has(name);

if (isExternalized || isReported) {
return;
}

context.report({
node: node,
messageId: 'requireTypeExport',
data: {
name,
},
});

reportedTypes.add(name);
}

function getTypeName(typeReference: TSESTree.TSTypeReference): string {
if (typeReference.typeName.type === AST_NODE_TYPES.Identifier) {
return typeReference.typeName.name;
}

return '';
StyleShit marked this conversation as resolved.
Show resolved Hide resolved
}

function isLocationOverlapping(
location: TSESTree.Node['loc'],
container: TSESTree.Node['loc'],
): boolean {
if (
location.start.line < container.start.line ||
location.end.line > container.end.line
) {
return false;
}

if (
location.start.line === container.start.line &&
location.start.column < container.start.column
) {
return false;
}

if (
location.end.line === container.end.line &&
location.end.column > container.end.column
) {
return false;
}

return true;
}

return {
Program: collectTypeReferences,

'ImportDeclaration ImportSpecifier, ImportSpecifier':
collectImportedTypes,

'ExportNamedDeclaration TSTypeAliasDeclaration, ExportNamedDeclaration TSInterfaceDeclaration, ExportNamedDeclaration TSEnumDeclaration':
collectExportedTypes,

'ExportNamedDeclaration[declaration.type="FunctionDeclaration"]':
visitExportedFunctionDeclaration,

'ExportNamedDeclaration[declaration.type="TSDeclareFunction"]':
visitExportedFunctionDeclaration,

'ExportNamedDeclaration[declaration.type="VariableDeclaration"]':
visitExportedVariableDeclaration,

'ExportDefaultDeclaration[declaration.type="FunctionDeclaration"]':
visitExportedFunctionDeclaration,

'ExportDefaultDeclaration[declaration.type="ArrowFunctionExpression"]':
visitDefaultExportedArrowFunction,

'ExportDefaultDeclaration[declaration.type="Identifier"]':
visitDefaultExportedIdentifier,
};
},
});
Loading
Loading