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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [explicit-function-return-type] support AllowTypedFunctionExpression within AllowHigherOrderFunction #4250

Merged
merged 8 commits into from Jan 17, 2022
Merged

fix(eslint-plugin): [explicit-function-return-type] support AllowTypedFunctionExpression within AllowHigherOrderFunction #4250

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1e8caf8
refactor: move function to be used more widely
lonyele Dec 5, 2021
bbe491f
feat: find if the ancestor(top level) is typed
lonyele Dec 5, 2021
c565182
test: add more test cases
lonyele Dec 5, 2021
34c4977
Merge branch 'main' into feat/allow-typed-func-expression-with-higher…
lonyele Dec 5, 2021
7804ba8
fix: lint fail on test case
lonyele Dec 5, 2021
c732d95
fix: make working with option combos
lonyele Dec 20, 2021
10922cd
chore: remove unnecessary comment
lonyele Dec 20, 2021
a7365e0
Merge branch 'main' into feat/allow-typed-func-expression-with-higher…
lonyele Dec 20, 2021
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
16 changes: 14 additions & 2 deletions packages/eslint-plugin/src/rules/explicit-function-return-type.ts
View file
Open in desktop
Expand Up @@ -5,7 +5,8 @@ import {
import * as util from '../util';
import {
checkFunctionReturnType,
checkFunctionExpressionReturnType,
isValidFunctionExpressionReturnType,
ancestorHasReturnType,
} from '../util/explicitReturnTypeUtils';

type Options = [
Expand Down Expand Up @@ -81,7 +82,14 @@ export default util.createRule<Options, MessageIds>({
return;
}

checkFunctionExpressionReturnType(node, options, sourceCode, loc =>
if (
isValidFunctionExpressionReturnType(node, options) ||
ancestorHasReturnType(node)
) {
return;
}

checkFunctionReturnType(node, options, sourceCode, loc =>
context.report({
node,
loc,
Expand All @@ -90,6 +98,10 @@ export default util.createRule<Options, MessageIds>({
);
},
FunctionDeclaration(node): void {
if (options.allowTypedFunctionExpressions && node.returnType) {
return;
}

checkFunctionReturnType(node, options, sourceCode, loc =>
context.report({
node,
Expand Down
50 changes: 1 addition & 49 deletions packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import {
FunctionExpression,
FunctionNode,
isTypedFunctionExpression,
ancestorHasReturnType,
} from '../util/explicitReturnTypeUtils';

type Options = [
Expand Down Expand Up @@ -390,55 +391,6 @@ export default util.createRule<Options, MessageIds>({
}
}

/**
* Check whether any ancestor of the provided function has a valid return type.
* This function assumes that the function either:
* - belongs to an exported function chain validated by isExportedHigherOrderFunction
* - is directly exported itself
*/
function ancestorHasReturnType(node: FunctionNode): boolean {
let ancestor = node.parent;

if (ancestor?.type === AST_NODE_TYPES.Property) {
ancestor = ancestor.value;
}

// if the ancestor is not a return, then this function was not returned at all, so we can exit early
const isReturnStatement =
ancestor?.type === AST_NODE_TYPES.ReturnStatement;
const isBodylessArrow =
ancestor?.type === AST_NODE_TYPES.ArrowFunctionExpression &&
ancestor.body.type !== AST_NODE_TYPES.BlockStatement;
if (!isReturnStatement && !isBodylessArrow) {
return false;
}

while (ancestor) {
switch (ancestor.type) {
case AST_NODE_TYPES.ArrowFunctionExpression:
case AST_NODE_TYPES.FunctionExpression:
case AST_NODE_TYPES.FunctionDeclaration:
if (ancestor.returnType) {
return true;
}
// assume
break;

// const x: Foo = () => {};
// Assume that a typed variable types the function expression
case AST_NODE_TYPES.VariableDeclarator:
if (ancestor.id.typeAnnotation) {
return true;
}
break;
}

ancestor = ancestor.parent;
}

return false;
}

function checkEmptyBodyFunctionExpression(
node: TSESTree.TSEmptyBodyFunctionExpression,
): void {
Expand Down
47 changes: 47 additions & 0 deletions packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts
View file
Open in desktop
Expand Up @@ -287,11 +287,58 @@ function checkFunctionExpressionReturnType(
checkFunctionReturnType(node, options, sourceCode, report);
}

/**
* Check whether any ancestor of the provided function has a valid return type.
*/
function ancestorHasReturnType(node: FunctionNode): boolean {
let ancestor = node.parent;

if (ancestor?.type === AST_NODE_TYPES.Property) {
ancestor = ancestor.value;
}

// if the ancestor is not a return, then this function was not returned at all, so we can exit early
const isReturnStatement = ancestor?.type === AST_NODE_TYPES.ReturnStatement;
const isBodylessArrow =
ancestor?.type === AST_NODE_TYPES.ArrowFunctionExpression &&
ancestor.body.type !== AST_NODE_TYPES.BlockStatement;
if (!isReturnStatement && !isBodylessArrow) {
return false;
}

while (ancestor) {
switch (ancestor.type) {
case AST_NODE_TYPES.ArrowFunctionExpression:
case AST_NODE_TYPES.FunctionExpression:
case AST_NODE_TYPES.FunctionDeclaration:
if (ancestor.returnType) {
return true;
}
// assume
Copy link
Member

Choose a reason for hiding this comment

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

assume... what?

Copy link
Member

Choose a reason for hiding this comment

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

this was copy pasted from the old code.
idk what it was supposed to be (I probably wrote it lol)

Copy link
Contributor Author

@lonyele lonyele Dec 20, 2021 •
edited

Choose a reason for hiding this comment

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

I think it was just a mistake to remove it. Comments on this function I found are already giving some good "whys" and leaving it may confuse others(actually including me when I first saw it :D) thus I removed it.

break;

// const x: Foo = () => {};
// Assume that a typed variable types the function expression
case AST_NODE_TYPES.VariableDeclarator:
if (ancestor.id.typeAnnotation) {
return true;
}
break;
}

ancestor = ancestor.parent;
}

return false;
}

export {
checkFunctionExpressionReturnType,
checkFunctionReturnType,
doesImmediatelyReturnFunctionExpression,
FunctionExpression,
FunctionNode,
isTypedFunctionExpression,
isValidFunctionExpressionReturnType,
ancestorHasReturnType,
};
49 changes: 49 additions & 0 deletions packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts
View file
Open in desktop
Expand Up @@ -400,6 +400,55 @@ new Foo(1, () => {});
code: 'const log = (message: string) => void console.log(message);',
options: [{ allowConciseArrowFunctionExpressionsStartingWithVoid: true }],
},
{
filename: 'test.ts',
code: `
type HigherOrderType = () => (arg1: string) => (arg2: number) => string;
const x: HigherOrderType = () => arg1 => arg2 => 'foo';
`,
options: [
{
allowTypedFunctionExpressions: true,
allowHigherOrderFunctions: true,
},
],
},
{
filename: 'test.ts',
code: `
interface Foo {
foo: string;
arrowFn: () => string;
}

function foo(): Foo {
return {
foo: 'foo',
arrowFn: () => 'test',
};
}
`,
options: [
{
allowTypedFunctionExpressions: true,
allowHigherOrderFunctions: true,
},
],
},
{
filename: 'test.ts',
code: `
type Foo = (arg1: string) => string;
type Bar<T> = (arg2: string) => T;
const x: Bar<Foo> = arg1 => arg2 => arg1 + arg2;
`,
options: [
{
allowTypedFunctionExpressions: true,
allowHigherOrderFunctions: true,
},
],
},
],
invalid: [
{
Expand Down