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

[explicit-function-return-type] allowTypedFunctionExpressions should support return values and default parameters #682

Closed
davidje13 opened this issue Jul 7, 2019 · 2 comments · Fixed by #4250
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@davidje13
Copy link

Repro

{
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "sourceType": "module"
  },
  "plugins": ["@typescript-eslint"],
  "rules": {
    "@typescript-eslint/explicit-function-return-type": ["error", {
      "allowTypedFunctionExpressions": true,
    }]
  }
}
function outer(): () => string {
  return () => 'hello';
}

function func(param: () => string = () => 'hello'): string {
  return param();
}

interface X {
  foobar: () => string;
}

function makeX(): X {
  return {
    foobar: () => 'hi',
  };
}

Expected Result

No errors;

  • The function returned by outer must be a () => string due to the return type of the containing function
  • The type of the default parameter for func must be () => string because the parameter is typed as that
  • The type of foobar in makeX can be inferred from the return type

Actual Result

Missing return type on function (@typescript-eslint/explicit-function-return-type) at foobar/Tests.ts:2:10
Missing return type on function (@typescript-eslint/explicit-function-return-type) at foobar/Tests.ts:5:37
Missing return type on function (@typescript-eslint/explicit-function-return-type) at foobar/Tests.ts:15:13

Additional Info

After reporting #679 I started looking for other situations where allowTypedFunctionExpressions feels like it should apply but currently has no effect. I don't have specific use-cases in mind for each, but some examples:

  • The first example is a common pattern for a function factory or monadic function chain; only the type of the base function is really needed, because it contains all information about the returned function already.
  • The second example is a pretty clear DRY violation; right now it would need to be written as func(param: () => string = (): string => 'hello') (worse if it takes parameters)
  • The third example is rare, but could happen if building an object to implement an interface on-the-fly rather than using a proper class. I think it would be OK if the current behaviour remained in this case. It could also appear for input parameters when using the named parameter pattern (not sure if that was resolved as part of feat(eslint-plugin): [explicit-function-return-type] add handling for usage as arguments #680)

For the monad case, there may even be an argument for inferring the parent type from a child type, but I think that is a separate issue (and likely a separate option).

Versions

package version
@typescript-eslint/eslint-plugin 1.11.0
@typescript-eslint/parser 1.11.0
TypeScript 3.5.2
ESLint 5.16.0
node 12.4.0
npm 6.9.0
@davidje13 davidje13 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jul 7, 2019
@bradzacher
Copy link
Member

bradzacher commented Jul 8, 2019

function func(param: () => string = () => 'hello'): string {
  return param();
}

This should be added as to the allowTypedFunctionExpressions option.

Though as an aside I thought the common practice was to not type the arg if you give it a default value?

function func(param = (): string => 'hello'): string {
  return param();
}

function outer(): () => string {
  return () => 'hello';
}

allowHigherOrderFunctions will take care of this for you to some degree, if you have no other statements in your function body (the following is legal):

function outer() {
  return (): string => 'hello';
}

That example, plus the other case of inferring a function on an object which is implicitly typed because it's being returned is getting into pretty complex edge cases for exactly what we should infer.

We do allow both of those cases for variable declarations, but I don't know if allowing these case is the correct course of action, because the type definition is very far removed from the return type definition.
I'll have to think about this some more.

@bradzacher bradzacher added enhancement New feature or request and removed triage Waiting for maintainers to take a look labels Jul 8, 2019
@davidje13
Copy link
Author

Ah yes, I see that allowHigherOrderFunctions is a better solution for monadic function chains (and was added explicitly for that in #193)

As I mentioned, the third example isn't very common and I think it's OK to leave it unimplemented.

For the first example, I agree with your reasoning that usually the type is omitted, but consider the case where it could have alternative forms:

function func(param: (() => string) | string = () => 'hello'): string {
  return param();
}

Or where the default does not need to use all available parameters:

function func(param: (a: number, b: number) => string = (a: number) => `hello-${a}`): string {
  return param(1, 2);
}

(specifying b in the default function here would lead to an unused variable warning).

But the main reason I mentioned it is because it feels like a (very) similar case which is currently not caught. I don't have a specific requirement for it; the above is speculation.

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants