Add ts eslint rule: explicit-function-return-type

[zepumph]

From #1114, there are currently 2963 issues with default options (which I'm unsure we will want). We certainly want this rule though, so I'll take a look.

To turn it on (not sure if these are the options either)

Index: eslint/.eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/.eslintrc.js b/eslint/.eslintrc.js
--- a/eslint/.eslintrc.js	(revision 091109036fa7ad87accd58e81223959e287f2ae1)
+++ b/eslint/.eslintrc.js	(date 1649108358576)
@@ -40,7 +40,16 @@
         // Put TypeScript specific rules here
         // '@typescript-eslint/no-inferrable-types': 'error',
         // '@typescript-eslint/no-unnecessary-type-constraint': 'error',
-        '@typescript-eslint/member-delimiter-style': 'error' // semi colons in type declarations.
+        '@typescript-eslint/member-delimiter-style': 'error', // semi colons in type declarations.
+
+        // functions get return types.
+        '@typescript-eslint/explicit-function-return-type': [ 'error', {
+          allowExpressions: true,
+          allowTypedFunctionExpressions: false,
+          allowHigherOrderFunctions: true,
+          allowDirectConstAssertionInArrowFunctions: true,
+          allowConciseArrowFunctionExpressionsStartingWithVoid: false
+        } ]
 
         // Typescript rules that require type information (may be slow)
         // These require parserOptions.project.

[zepumph]

I'm not sure we are going to be able to use this rule, because it is requiring return types on all functions, not just methods. There is no option to check just method declarations. That said, if we adapted our own (not recommending) we could just check on function declarations and not on function/arrow function expressions.

[zepumph]

It looks like there is a compiler option: noImplicitReturns. If this does what we want (only for methods, not all anonymous functions). The doc for this is /* Report error when not all code paths in function return a value. */. So I don't think it will help, but it is already a lint rule I believe, so I'm going to turn it on.

[zepumph]

@jessegreenberg offered to help out on this.

[zepumph]

Perhaps it would be pretty easy to use this as a base, and or the visibility annotation rule we already have for javascript.

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/explicit-function-return-type.ts

[jessegreenberg]

Surprisingly nothing different had to be done for custom TS rules. I added a custom lint rule that requires a return type for method definitions excluding constructors getters and setters.

There are 1584 errors, I scanned them and they seem like legit hits. I disabled it for now. Could be a lot of work now to get clean.

[samreid]

How difficult is it to write an autofix step that uses the inferred type? I'm not saying to commit that blindly, but it could convert 1584 occurrences and then spot checking them could be much faster than writing all of them.

[jessegreenberg]

That could be worthwhile, Ill take a look.

[jessegreenberg]

Yes, this is working well! This rule now requires type information (parserOptions.project in .eslintrc.js and is slower). But running with --fix will insert the inferred type automatically.

To run this with auto fix

1. uncomment this line in chipper/eslint/.eslintrc.js
   

   chipper/eslint/.eslintrc.js

   Line 33 in 00b8fcb

   // project: [ '../chipper/eslint/tsconfig.eslint.json' ]

2. uncomment the explicit-method-return-type rule
   

   chipper/eslint/.eslintrc.js

   Line 45 in 00b8fcb

   // 'explicit-method-return-type': 'error', // 1584 errors, TODO https://github.com/phetsims/chipper/issues/1215

3. Run grunt lint in a repo to see the errors. Run grunt lint --fix to fill in missing return types.

[zepumph]

Can we do this and then remove the dependency on parserOptions.project after we fix?

[zepumph]

With the great help from @jessegreenberg I was able to turn this rule on! Thanks for helping to squash false positives. There should be no spots where the fixer input any. Instead I looked there myself to see what was best. Type checking and linting is passing, and I commented out the fixer so that we don't nee type information for the rule generally. Thanks @jessegreenberg for the doing the brunt of the work. What a great lintrule!

[zepumph]

Three more TODOs in this issue:

chipper/eslint/rules/explicit-method-return-type.js

Lines 50 to 52 in 02150cc

	// TODO: do Range/Node etc as a second fix to easily find spots where we are using the DOM type.
	// TODO: any should be filled in themselves.
	// TODO: look into some derivedProperty or other Property returns. Perhaps do those manually.

From #1372