Make type checking lint rules faster

[jessegreenberg]

From #1114. Lint rules that use type checking are too slow. They will be slower because they use the type checker but currently they are just way too slow to use.

Maybe we can make it faster by having eslint use the same tsc cache as WebStorm. The config file tsconfig.eslint.json is really inefficient. I think it makes eslint recompile every file in the project every time the linter is run. Maybe we can make that better.

[jessegreenberg]

In #1114 (comment) @samreid said

Specifying this in package.json for center-and-spread

 "eslintConfig": {
    "extends": "../chipper/eslint/sim_eslintrc.js",
    "parserOptions": {
      "project": [
        "./tsconfig.json"
      ]
    }
  }

Changes the time of time grunt lint --disable-eslint-cache from 12 seconds to 2 seconds. With caching and no code changes it is 0.6sec whether you use the full tsconfig or the sim-specific one.

I just confirmed that works great for me too. Without that patch, running time grunt lint --disable-eslint-cache takes 27 seconds. With the patch it takes ~5 seconds.

This works in sim repos but doesn't work in common code repos like scenery utterance-queue and sun, I am seeing lots of

C:\Users\Jesse\Documents\Development\phetsims\sun\js\sunStrings.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: js\sunStrings.ts.
The file must be included in at least one of the projects provided

[jessegreenberg]

Changing parserOptions.project to "project": "./tsconfig-module.json" makes some of the errors go away but not all of them in common code repos.

For example in utterance-queue I am seeing

C:\Users\Jesse\Documents\Development\phetsims\utterance-queue\js\ResponsePacket.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: js\ResponsePacket.ts.
The file must be included in at least one of the projects provided

Adding "js/ResponsePacket.ts", explicitly to the include of the tsconfig-module.json does not fix it.

[samreid]

According to typescript-eslint/typescript-eslint#2094, typescript-eslint does not support project references, but they are experimenting with adding that as a feature. The workaround described in typescript-eslint/typescript-eslint#2094 (comment) indicates to list all projects explicitly, which I tried to do like so:

      parserOptions: {
        sourceType: 'module',

        // Provide a tsconfig so that we can use rules that require type information. tsconfig.eslint.json
        // gives eslint project information without needing to modify our actual tsconfig setup.
        // NOTE: Providing this slows down eslint substantially, see https://github.com/phetsims/chipper/issues/1114#issuecomment-1065927717
        project: [ '../*/tsconfig.json' ]
      },

Linting sims seemed to work OK and I was more confident it could read all source files for type checking. If I recall correctly, it took 20-30 seconds in checking a new repo, and was much faster for re-runs.

When I tried to apply this to lint-everything, I ran into out of memory errors. Following the guide at https://futurestud.io/tutorials/node-js-increase-the-memory-limit-for-your-process I increased the allowed heap and ran

node -max-old-space-size=12192 /usr/local/bin/grunt lint-everything

It is still running to this day.

[jessegreenberg]

A temporary flag to tap into this in added in typescript-eslint/typescript-eslint#2094 (comment) and reports suggest is it working well. It was added to the "canary" build, but if I understand correctly the canary build is just the one right before main so this flag could be in the main build by now.

Turning it on should just require setting parserOptions.EXPERIMENTAL_useSourceOfProjectReferenceRedirect = true.

[jessegreenberg]

Adding this to sun/package.json does seem to be working well:

    "overrides": [
      {
        "files": [
          "**/*.ts"
        ],
        "parserOptions": {
          "project": [
            "./tsconfig.json",
            "./tsconfig-module.json"
          ],
          "EXPERIMENTAL_useSourceOfProjectReferenceRedirect": true
        }
      }
    ]

[jessegreenberg]

Some notes after playing with this for a while:

1. EXPERIMENTAL_useSourceOfProjectReferenceRedirect is working well, I am not seeing any errors yet.
2. I added parserOptions.project.EXPERIMENTAL_useSourceOfProjectReferenceRedirect to chipper's .eslintrc.js and it works well there too.
3. I wrote a grunt task to modify package.json for all repos, if we want to proceed with this:

// Copyright 2022, University of Colorado Boulder

/**
 * Modifies package.json for ALL repos under the project directory that have a package.json with overrides
 * for eslintConfig. It adds a reference to the tsconfig.json files so that the repo can run typescript-eslint
 * rules that leverage type information.
 */

const _ = require( 'lodash' ); // eslint-disable-line
const grunt = require( 'grunt' );
const fs = require( 'fs' );

/**
 * Checks out master for all repositories in the git root directory.
 * @public
 */
module.exports = function() {

  const gitRoots = grunt.file.expand( { cwd: '..' }, '*' );

  for ( let i = 0; i < gitRoots.length; i++ ) {
    const filename = gitRoots[ i ]; // Don't change to const without rewrapping usages in the closure
    if ( filename !== 'babel' && grunt.file.isDir( `../${filename}` ) && grunt.file.exists( `../${filename}/package.json` ) ) {

      const rawData = fs.readFileSync( `../${filename}/package.json` );
      const jsonData = JSON.parse( rawData );

      if ( jsonData.eslintConfig ) {

        if ( jsonData.eslintConfig.overrides ) {
          console.log( 'found package.json with eslintconfig.overrides in ' + filename );
        }
        else {
          const eslintConfigOverrides = [
            {
              files: [
                '**/*.ts'
              ],
              parserOptions: {
                project: [
                  `../${filename}/tsconfig.json`,
                  `../${filename}/tsconfig-module.json`
                ]
              }
            }
          ];

          // modify object in memory
          jsonData.eslintConfig.overrides = eslintConfigOverrides;

          // stringify in human readable format
          const stringified = JSON.stringify( jsonData, null, 2 );
          fs.writeFileSync( `../${filename}/package.json`, stringified );
        }
      }
    }
  }
};

4. With this change, time grunt lint --disable-eslint-cache in center-and-variability takes ~8.5 seconds for me. Running with no code changes and cache also takes ~8.5 seconds. For some reason its is better in joist, the same test in takes 11 seconds without cache, 2 seconds with cache.
5. I enabled @typescript-eslint/restrict-plus-operands and went to one of the found problems in scenery. Fixing then restoring the error felt snappy in the IDE for the single file even though running time grunt lint in scenery takes 15 seconds for me.
6. When I ran grunt lint-everything I still hit the out of memory problem. Maybe the issue is with our lint-everything grunt task? It seems to get stuck in https://eslint.org/docs/developer-guide/nodejs-api#-eslintlintfilespatterns so maybe we can implement it without using that function to control the flow/memory ourselves. That could also slow down grunt task lint-everything, Im not sure.

EDIT: Oh and here is my chipper patch as well as my change to center-and-variability to test in a single repo.

CHipper patch:

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 61879822b0e749dce4849d2ee6324b1144b73166)
+++ b/eslint/.eslintrc.js	(date 1652131330699)
@@ -25,12 +25,16 @@
       ],
       parser: '@typescript-eslint/parser',
       parserOptions: {
-        sourceType: 'module'
+        sourceType: 'module',
 
         // Provide a tsconfig so that we can use rules that require type information. tsconfig.eslint.json
         // gives eslint project information without needing to modify our actual tsconfig setup.
         // NOTE: Providing this slows down eslint substantially, see https://github.com/phetsims/chipper/issues/1114#issuecomment-1065927717
-        // ,project: [ '../chipper/eslint/tsconfig.eslint.json' ]
+        // project: [ '../chipper/eslint/tsconfig.eslint.json' ],
+        project: [ '../chipper/tsconfig.json' ],
+
+        // Enable experimental project references feature of typescript-eslint.
+        EXPERIMENTAL_useSourceOfProjectReferenceRedirect: true
       },
       plugins: [
         '@typescript-eslint'
@@ -78,13 +82,13 @@
         // '@typescript-eslint/no-empty-function': 'error', // 41 errors
         '@typescript-eslint/no-extra-semi': 'error',
         // '@typescript-eslint/no-unused-vars': 'error', //TODO https://github.com/phetsims/chipper/issues/1230
-        '@typescript-eslint/no-loss-of-precision': 'error'
+        '@typescript-eslint/no-loss-of-precision': 'error',
 
         ///////////////////////////////////////////////////////////////////////
         //
         // Typescript rules that require type information (may be slow)
         // These require parserOptions.project.
-        // '@typescript-eslint/no-unnecessary-type-assertion':'error',
+        // '@typescript-eslint/no-unnecessary-type-assertion': 'error'
         // '@typescript-eslint/no-unsafe-member-access':'error',
         // '@typescript-eslint/restrict-plus-operands':'error',
         // '@typescript-eslint/prefer-readonly': 'error' // readonly when possible
@@ -100,7 +104,7 @@
         // '@typescript-eslint/no-unsafe-call': 'error',
         // '@typescript-eslint/no-unsafe-member-access': 'error',
         // '@typescript-eslint/no-unsafe-return': 'error',
-        // '@typescript-eslint/restrict-plus-operands': 'error',
+        '@typescript-eslint/restrict-plus-operands': 'error',
         // '@typescript-eslint/restrict-template-expressions': 'error',
         // '@typescript-eslint/unbound-method': 'error',
 

Center and variability patch:

Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/package.json b/package.json
--- a/package.json	(revision f9176187ad52720c03ddc06c2e656979b7cb56f3)
+++ b/package.json	(date 1652131124861)
@@ -29,6 +29,19 @@
     }
   },
   "eslintConfig": {
-    "extends": "../chipper/eslint/sim_eslintrc.js"
+    "extends": "../chipper/eslint/sim_eslintrc.js",
+    "overrides": [
+      {
+        "files": [
+          "**/*.ts"
+        ],
+        "parserOptions": {
+          "project": [
+            "../center-and-variability/tsconfig.json"
+          ]
+        }
+      }
+    ]
   }
 }
\ No newline at end of file

[jessegreenberg]

Now that we don't use project references we should try this again and see if this is faster. #1238 (comment) probably isn't relevant anymore because of this.

[zepumph]

• If we can get this fast enough to turn on these rules, please create a new issue to determine what type-checking rules we should use (likely by seeing how many errors these rules have to begin with).

[samreid]

Increasing priority, since I hope to look into this during this week.

[jessegreenberg]

It is much faster! Well done @samreid. Timing results below. However, there is still a problem with grunt lint-everything, it takes a very long time.

I changed parserOptions.project to './tsconfig.json' in chipper and it seems to work for all repos. Timing grunt lint with and without cache:

• quadrilateral:
  • without cache: 9 seconds
  • with cache: 2 seconds
• scenery:
  • without cache: 25 seconds
  • with cache: 2 seconds
  • I observed it took ~15 seconds for WebStorm to run lint when I opened Node.ts.
  • Making incremental changes, it took WebStorm ~5 seconds to report new lint errors in Node.ts.
• chipper:
  • without cache: 8 seconds
  • with cache: 2 seconds
• grunt lint-everything
  • without cache: It has been running for 10 minutes with no results yet.

IF memory isn't a problem at 10-30 seconds per repo maybe grunt lint-everything would now take 30-90 minutes for 184 repos?

UPDATE: grunt lint-everything --disable-cache finished! TOtal time: 30m31.578s. IT didn't run out of memory.

However, I see many errors like

C:\Users\Jesse\Documents\Development\phetsims\forces-and-motion-basics\images\brickTile_png.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: ..\forces-and-motion-basics\images\brickTile_png.ts.
The file must be included in at least one of the projects provided

EDIT: It is resolved if parserOptions.project is project: [ './tsconfig.json' ] instead of pointing to chipper's. That makes sense, but it means that grunt lint-everything will not work because it will look for the file relative to perennial where the command is forwarded. Maybe we just need to add a paraserOptions.project to all package.jsons.

[jessegreenberg]

I added

        "parserOptions": {
          "project": [
            "../ohms-law/tsconfig.json"
          ]
        }

to all package.jsons and ran lint-everything again. After 1min 48 seconds I got an out of memory error. It seems to me that #1238 (comment) still applies. We may be able to use a different implementation of the grunt lint-everything task.

But the best case scenario would be a ~30 minute run time for the command when there is no cache. That doesn't seem worth pursuing.

[jessegreenberg]

@samreid and I met to try more things:

1. We modified a tsconfig file in a sim and verified that running grunt lint-everything used the sim's tsconfig and not chipper's when parserOptions.project is [ './tsconfig.json' ] in chipper.
2. We changed chipper's parserOptions.project to point to '../chipper/tsconfig/all/tsconfig.json'. We found we had to add all sims to that file in order for it to work. Currently it just lists TypeScript sims. It works! But we think lint-everything will take ~30 minutes.

Alternatively could we add this to each repo's package.json so that tsc doesn't get slower?

  "eslintConfig": {
    "extends": "../chipper/eslint/sim_eslintrc.js",
    "overrides": [
      {
        "files": [
          "**/*.ts"
        ],
        "parserOptions": {
          "project": [
            "../acid-base-solutions/tsconfig.json"
          ]
        }
      }
    ]
  }

That wont make lint-everything any faster.

[jessegreenberg]

Alternatively could we add this to each repo's package.json so that tsc doesn't get slower?

I did this with just the a-b sims in active-repos and ran out of memory before 2 minutes. Otherwise that works, there were no other problems running lint.

EDIT: I tried this change to lint-everything task and still ran out of memory around build-a-molecule

Index: js/grunt/Gruntfile.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/Gruntfile.js b/js/grunt/Gruntfile.js
--- a/js/grunt/Gruntfile.js	(revision 01b78de7541b9622ce7e2fbeb9544f78a46c14a3)
+++ b/js/grunt/Gruntfile.js	(date 1653003330654)
@@ -516,10 +516,14 @@
     // Don't always require this, as we may have an older chipper checked out.  Also make sure it is the promise-based lint.
     const lint = require( '../../../chipper/js/grunt/lint' );
     if ( lint.chipperAPIVersion === 'promises1' ) {
-      await lint( activeRepos.map( repo => `../${repo}` ), {
-        cache: cache,
-        fix: fix,
-        format: format
+      const lintables = activeRepos.map( repo => `../${repo}` );
+      lintables.forEach( async lintable => {
+        console.log( 'linting ', lintable );
+        await lint( [ lintable ], {
+          cache: cache,
+          fix: fix,
+          format: format
+        } );
       } );
     }
   } ) );

[samreid]

I tried https://typescript-eslint.io/docs/linting/monorepo/

        tsconfigRootDir: __dirname,
        project: [ '../../*/tsconfig.json' ]

But it failed after about 4 minutes with an out of memory error.

[jessegreenberg]

I tried

1. setting parserOptions.project to [ '../chipper/tsconfig/all/tsconfig.json' ]
2. Add all active repos to all/tsconfig.json
3. Changing `lint-everything to lint each active repo one at a time (mostly to see how far it gets in the command)

Running tsc in chipper/tsconfig/all now takes ~20-40 seconds. Running grunt lint in a single repo takes ~2 minutes without cache, 2 seconds with cache. Running lint-everything worked without memory problems and took ~30 minutes to finish the first time. I ran it again and it was able to use the cache, it took ~6 seconds!

[samreid]

Good experiments! I also ran a few tests. I wanted to see the performance characteristic of running the idiomatic way described on https://eslint.org/docs/user-guide/command-line-interface which is to run:

npx eslint js/** --rulesdir ../chipper/eslint/rules/

Which I think only works because I have a node_modules as a sibling of my checkout repos.

I also tried with and without --cache. And I tried with the current all/tsconfig and compared to the approach listed in #1238 (comment) where each sim package.json specifies its own tsconfig.

For these tests, I enabled '@typescript-eslint/no-unnecessary-type-assertion' and tested in Gravity and Orbits.

Using tsconfig/all and no cache: 11.6s
Using tsconfig/all and cache (2nd run): 0.96s

Using

  "eslintConfig": {
    "extends": "../chipper/eslint/sim_eslintrc.js",
    "overrides": [
      {
        "files": [
          "**/*.ts"
        ],
        "parserOptions": {
          "project": [
            "../gravity-and-orbits/tsconfig.json"
          ]
        }
      }
    ]
  }

without cache: 3.1s
with cache (2nd run): 0.70s

The latter result seems fast enough that we could use it for linting in the IDE and for precommit hooks, etc. We could split up chipper/js/grunt/lint.js so it is more repo-oriented if we like this approach.

@jessegreenberg based on the results in your experiment and this one, how do you think we should proceed?

[zepumph]

The latter result seems fast enough that we could use it for linting in the IDE and for precommit hooks, etc. We could split up chipper/js/grunt/lint.js so it is more repo-oriented if we like this approach.

Or can we just dynamically set the "project" flag based on the "patterns" parameter?

[jessegreenberg]

Re #1238 (comment)

That is quite fast! Good to know.

Or can we just dynamically set the "project"

Yes, that is what I was thinking too. I went ahead and added an option for the lint task called --type-info. It adds the typescript eslint rules and sets the project flag from patterns.

It seems to work well. It would be simple to use these for git hooks. But I couldn't figure out a way to get this strategy to work with the IDE.

@samreid can you please review this change? Do you see a way to make this work with the IDE without breaking lint-everything? Should we start using this with git hooks? If yes, we should disable no-unnecessary-type-assertion from type_info_eslintrc.js until it is discussed with the team.

[samreid]

I tested looping over each repo like so and it seemed to have good characteristics (including running lint-everything faster), we may want to base our strategy on this. Stashing a copy to clean my working copy.

Index: js/grunt/Gruntfile.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/Gruntfile.js b/js/grunt/Gruntfile.js
--- a/js/grunt/Gruntfile.js	(revision a886e4d2ddae82503a9acb30753079987edfb687)
+++ b/js/grunt/Gruntfile.js	(date 1653972435290)
@@ -516,11 +516,13 @@
     // Don't always require this, as we may have an older chipper checked out.  Also make sure it is the promise-based lint.
     const lint = require( '../../../chipper/js/grunt/lint' );
     if ( lint.chipperAPIVersion === 'promises1' ) {
-      await lint( activeRepos.map( repo => `../${repo}` ), {
-        cache: cache,
-        fix: fix,
-        format: format
-      } );
+      for ( let i = 0; i < activeRepos.length; i++ ) {
+        await lint( [ activeRepos[ i ] ], {
+          cache: cache,
+          fix: fix,
+          format: format
+        } );
+      }
     }
   } ) );
 

</details.

[jessegreenberg]

That is promising but I don't think that patch is linting anything. The lint command should be

await lint( [ `../${activeRepos[ i ]}` ], {

I tried this with type checking lint rules in #1238 (comment) but ran out of memory.

[samreid]

In the commit, I enabled parserOptions.project and enabled a rule that happened to have 0 failures. I think this issue is ready to close. We can evaluate rules as part of #1277. @jessegreenberg what do you think?

[zepumph]

Today I'm noticing that my git hooks are slower by a factor of 3-5. I suspect it is from this change. I would recommend reverting it until we know for sure that a lint rule in this list is worth the great productivity hit I've experienced. That said, during the typescript sprint I am making many more cross-repo commits than usual.

[jessegreenberg]

Understood. Lets check in tomorrow or at dev meeting with the team. We have options for how to proceed:

1. Disable these rules generally and opt in to them occasionally with a command line option.
2. We may be able to use a setup now that has the rules enabled for WebStorm but disabled for git hooks.
3. Keep looking for a solution that works everywhere. Given the amount of time in this issue already I don't recommend this.
4. Change our work process from pre-commit to pre-push hooks: Should we transition from precommit hooks to prepush hooks? Or can we speed up the precommit hooks? #1269

[zepumph]

@jessegreenberg, @samreid and I discussed this in depth this afternoon.

RE: 1. We want these lint rules to work in the IDE, so this won't work.

RE: 2. Not ideal, we as a team most often error on the side of overly cautious, even at the cost of time.

RE: 3. We spent a fair bit of time on this today, investigating the "individual repo" linting strategy with the patch in #1238 (comment). We also found that most of the time has to do with the tsc command in the pre-commit hook, and not lint.

RE: 4. This seems like the best path forward for maximum productivity. We want to investigate further. At the very least it can be something that members opt into, disabling their pre-commit hooks in exchange for using a script to push-all + a hook script.

[zepumph]

Next steps:

• @jessegreenberg will remove an old type-checking-specific eslintrc
• I will commit the new caching strategy for lint in linting multiple repos should cache individually #1286
• @samreid will investigate pre-push hooks in Should we transition from precommit hooks to prepush hooks? Or can we speed up the precommit hooks? #1269

@jessegreenberg perhaps we are ready to close this issue when you have cleaned up?

[jessegreenberg]

typeInfo was removed from lint.js and the extra file type_info_eslintrc.js has been deleted.

[jessegreenberg]

We touched on this in during developer meeting today. There is nothing more to do here. Pre-commit hooks take still take longer. and we discussed #1269 as a workflow change to improve productivity. See #1269 (comment), we will continue there.