Skip to content

Commit

Permalink
feat(eslint-plugin): [no-unsafe-return] check promise any (#8693)
Browse files Browse the repository at this point in the history
* feat(eslint-plugin): [no-unsafe-return] check promise any

* add testcases

* apply reviews

* fix lint errors

* refactor

* add type

* improve messages

* update comment

* apply review

* fix

* fix tests

* update docs

* apply review

* fix tests

* change to use internal getAwaitedType

* add todo comment

* Update packages/eslint-plugin/docs/rules/no-unsafe-return.mdx

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

* apply reviews

---------

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
  • Loading branch information
yeonjuan and JoshuaKGoldberg authored Aug 12, 2024
1 parent 3d9ae44 commit c20bd2f
Show file tree
Hide file tree
Showing 7 changed files with 375 additions and 26 deletions.
10 changes: 9 additions & 1 deletion packages/eslint-plugin/docs/rules/no-unsafe-return.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Using `any` disables many type checking rules and is generally best used only as
Despite your best intentions, the `any` type can sometimes leak into your codebase.
Returning an an `any`-typed value from a function creates a potential type safety hole and source of bugs in your codebase.

This rule disallows returning `any` or `any[]` from a function.
This rule disallows returning `any` or `any[]` from a function and returning `Promise<any>` from an async function.

This rule also compares generic type argument types to ensure you don't return an unsafe `any` in a generic position to a function that's expecting a specific type.
For example, it will error if you return `Set<any>` from a function declared as returning `Set<string>`.
Expand Down Expand Up @@ -56,6 +56,10 @@ const foo10 = () => [] as any[];

const foo11 = (): string[] => [1, 2, 3] as any[];

async function foo13() {
return Promise.resolve({} as any);
}

// generic position examples
function assignability1(): Set<string> {
return new Set<any>([1]);
Expand All @@ -78,6 +82,10 @@ function foo2() {
const foo3 = () => [];
const foo4 = () => ['a'];

async function foo5() {
return Promise.resolve(1);
}

function assignability1(): Set<string> {
return new Set<string>(['foo']);
}
Expand Down
60 changes: 50 additions & 10 deletions packages/eslint-plugin/src/rules/no-unsafe-return.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import * as ts from 'typescript';
import {
AnyType,
createRule,
discriminateAnyType,
getConstrainedTypeAtLocation,
getContextualType,
getParserServices,
getThisExpression,
isAnyOrAnyArrayTypeDiscriminated,
isTypeAnyType,
isTypeFlagSet,
isTypeUnknownArrayType,
Expand All @@ -28,9 +28,9 @@ export default createRule({
requiresTypeChecking: true,
},
messages: {
unsafeReturn: 'Unsafe return of an {{type}} typed value.',
unsafeReturn: 'Unsafe return of a value of type {{type}}.',
unsafeReturnThis: [
'Unsafe return of an `{{type}}` typed value. `this` is typed as `any`.',
'Unsafe return of a value of type `{{type}}`. `this` is typed as `any`.',
'You can try to fix this by turning on the `noImplicitThis` compiler option, or adding a `this` parameter to the function.',
].join('\n'),
unsafeReturnAssignment:
Expand Down Expand Up @@ -78,7 +78,14 @@ export default createRule({
reportingNode: TSESTree.Node = returnNode,
): void {
const tsNode = services.esTreeNodeToTSNodeMap.get(returnNode);
const anyType = isAnyOrAnyArrayTypeDiscriminated(tsNode, checker);
const type = checker.getTypeAtLocation(tsNode);

const anyType = discriminateAnyType(
type,
checker,
services.program,
tsNode,
);
const functionNode = getParentFunctionNode(returnNode);
/* istanbul ignore if */ if (!functionNode) {
return;
Expand All @@ -100,27 +107,46 @@ export default createRule({
if (!functionType) {
functionType = services.getTypeAtLocation(functionNode);
}

const callSignatures = tsutils.getCallSignaturesOfType(functionType);
// If there is an explicit type annotation *and* that type matches the actual
// function return type, we shouldn't complain (it's intentional, even if unsafe)
if (functionTSNode.type) {
for (const signature of tsutils.getCallSignaturesOfType(functionType)) {
for (const signature of callSignatures) {
const signatureReturnType = signature.getReturnType();

if (
returnNodeType === signature.getReturnType() ||
returnNodeType === signatureReturnType ||
isTypeFlagSet(
signature.getReturnType(),
signatureReturnType,
ts.TypeFlags.Any | ts.TypeFlags.Unknown,
)
) {
return;
}
if (functionNode.async) {
const awaitedSignatureReturnType =
checker.getAwaitedType(signatureReturnType);

const awaitedReturnNodeType =
checker.getAwaitedType(returnNodeType);
if (
awaitedReturnNodeType === awaitedSignatureReturnType ||
(awaitedSignatureReturnType &&
isTypeFlagSet(
awaitedSignatureReturnType,
ts.TypeFlags.Any | ts.TypeFlags.Unknown,
))
) {
return;
}
}
}
}

if (anyType !== AnyType.Safe) {
// Allow cases when the declared return type of the function is either unknown or unknown[]
// and the function is returning any or any[].
for (const signature of functionType.getCallSignatures()) {
for (const signature of callSignatures) {
const functionReturnType = signature.getReturnType();
if (
anyType === AnyType.Any &&
Expand All @@ -134,6 +160,18 @@ export default createRule({
) {
return;
}
const awaitedType = checker.getAwaitedType(functionReturnType);
if (
awaitedType &&
anyType === AnyType.PromiseAny &&
isTypeUnknownType(awaitedType)
) {
return;
}
}

if (anyType === AnyType.PromiseAny && !functionNode.async) {
return;
}

let messageId: 'unsafeReturn' | 'unsafeReturnThis' = 'unsafeReturn';
Expand Down Expand Up @@ -161,7 +199,9 @@ export default createRule({
? 'error'
: anyType === AnyType.Any
? '`any`'
: '`any[]`',
: anyType === AnyType.PromiseAny
? '`Promise<any>`'
: '`any[]`',
},
});
}
Expand Down

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

Loading

0 comments on commit c20bd2f

Please sign in to comment.