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 JSX attributes in allowTypedFunctionExpressions #7553

Merged
merged 3 commits into from Nov 10, 2023
Merged

fix(eslint-plugin): [explicit-function-return-type] support JSX attributes in allowTypedFunctionExpressions #7553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56cfcca
fix(eslint-plugin): support JSX attrs in allowTypedFunctionExpressions
merrywhether Aug 28, 2023
52ff72c
Expand test suite
merrywhether Sep 12, 2023
a4df798
Merge branch 'main' into explicit-return-type-jsx
merrywhether Nov 6, 2023
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
4 changes: 4 additions & 0 deletions packages/eslint-plugin/docs/rules/explicit-function-return-type.md
View file
Open in desktop
Expand Up @@ -172,6 +172,10 @@ functionWithObjectArg({
return 1;
},
});

const Comp: FC = () => {
return <button onClick={() => {}} />;
};
```

### `allowHigherOrderFunctions`
Expand Down
77 changes: 49 additions & 28 deletions packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts
View file
Open in desktop
Expand Up @@ -37,6 +37,53 @@ function isPropertyDefinitionWithTypeAnnotation(
);
}

/**
* Checks if a node belongs to:
* ```
* foo(() => 1)
* ```
*/
function isFunctionArgument(
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
parent: TSESTree.Node,
callee?: FunctionExpression,
): parent is TSESTree.CallExpression {
return (
parent.type === AST_NODE_TYPES.CallExpression &&
// make sure this isn't an IIFE
parent.callee !== callee
);
}

/**
* Checks if a node is type-constrained in JSX
* ```
* <Foo x={() => {}} />
* <Bar>{() => {}}</Bar>
* <Baz {...props} />
* ```
*/
function isTypedJSX(
node: TSESTree.Node,
): node is TSESTree.JSXExpressionContainer | TSESTree.JSXSpreadAttribute {
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
return (
node.type === AST_NODE_TYPES.JSXExpressionContainer ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically the relevant RHS type of JSXAttribute, so this check could also recurse one level higher if that is preferred. This should be the only way to directly supply a function in JSX though.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want to check the parent because this is another valid location for a JSXExpressionContainer:

<>{() => {}}</>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradzacher Ah, I totally forgot about the render-props pattern, it's been a while since their heyday! Running this down actually led me across an existing issue in TS where fragment shorthands are not typed properly: microsoft/TypeScript#50429

I went around on this for a while, but I again think all JSXExpressionContainer instances are fully bound by types barring that issue (reference CodeSandbox with render props):

  1. Attribute assignment: original case; type is constrained by assignment
  2. Render props of built-ins and <Fragment />: not allowed due to limited child types (JSX.ElementType or JSX.Element | null); all functions will be type errors here
  3. Render props of <> fragment shorthand: all functions should be type errors here
  4. Render props of components with custom function children: properly typed-constrained by explicit children prop, the same as if children was defined via attribute

I eventually realized that while something like this is unsafe:

<>{() => 'bug'}</>

Having a lint error that turns it into this doesn't make things better as it's still not a type error but won't work as expected at runtime (especially if you return an object instead):

<>{(): string => 'bug'}</>

While this is a currently an issue with TypeScript, it doesn't seem like the lint rule can do anything better here in place of proper type-checking. But functions can be valid in children expression containers generally.

Reference TS-ESLint playground

node.type === AST_NODE_TYPES.JSXSpreadAttribute
);
}

function isTypedParent(
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
parent: TSESTree.Node,
callee?: FunctionExpression,
): boolean {
return (
isTypeAssertion(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isFunctionArgument(parent, callee) ||
isTypedJSX(parent)
);
}

/**
* Checks if a node belongs to:
* ```
Expand Down Expand Up @@ -78,13 +125,7 @@ function isPropertyOfObjectWithType(
return false;
}

return (
isTypeAssertion(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isFunctionArgument(parent) ||
isPropertyOfObjectWithType(parent)
);
return isTypedParent(parent) || isPropertyOfObjectWithType(parent);
}

/**
Expand Down Expand Up @@ -127,23 +168,6 @@ function doesImmediatelyReturnFunctionExpression({
);
}

/**
* Checks if a node belongs to:
* ```
* foo(() => 1)
* ```
*/
function isFunctionArgument(
parent: TSESTree.Node,
callee?: FunctionExpression,
): parent is TSESTree.CallExpression {
return (
parent.type === AST_NODE_TYPES.CallExpression &&
// make sure this isn't an IIFE
parent.callee !== callee
);
}

/**
* Checks if a function belongs to:
* ```
Expand Down Expand Up @@ -194,11 +218,8 @@ function isTypedFunctionExpression(
}

return (
isTypeAssertion(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isTypedParent(parent, node) ||
isPropertyOfObjectWithType(parent) ||
isFunctionArgument(parent, node) ||
isConstructorArgument(parent)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRYing these out, it makes it more obvious that the recursive object-property case doesn't include this isConstructorArgument() case, but it seems like it should?

Happy to move it into isTypedParent() and add tests if this is indeed a bug.

);
}
Expand Down
137 changes: 137 additions & 0 deletions packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts
View file
Open in desktop
Expand Up @@ -180,6 +180,53 @@ class App {
`,
options: [{ allowTypedFunctionExpressions: true }],
},
// https://github.com/typescript-eslint/typescript-eslint/issues/7552
{
code: 'const foo = <button onClick={() => {}} />;',
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
code: 'const foo = <button on={{ click: () => {} }} />;',
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
code: 'const foo = <Bar>{() => {}}</Bar>;',
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
code: 'const foo = <Bar>{{ on: () => {} }}</Bar>;',
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
code: 'const foo = <button {...{ onClick: () => {} }} />;',
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},

// https://github.com/typescript-eslint/typescript-eslint/issues/525
{
code: `
Expand Down Expand Up @@ -977,6 +1024,96 @@ const x: Foo = {
},
],
},
{
code: 'const foo = <button onClick={() => {}} />;',
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 1,
endLine: 1,
column: 33,
endColumn: 35,
},
],
},
{
code: 'const foo = <button on={{ click: () => {} }} />;',
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 1,
endLine: 1,
column: 27,
endColumn: 34,
},
],
},
{
code: 'const foo = <Bar>{() => {}}</Bar>;',
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 1,
endLine: 1,
column: 22,
endColumn: 24,
},
],
},
{
code: 'const foo = <Bar>{{ on: () => {} }}</Bar>;',
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 1,
endLine: 1,
column: 21,
endColumn: 25,
},
],
},
{
code: 'const foo = <button {...{ onClick: () => {} }} />;',
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 1,
endLine: 1,
column: 27,
endColumn: 36,
},
],
},
{
code: '() => () => {};',
options: [{ allowHigherOrderFunctions: true }],
Expand Down