phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
MIT License
2 stars 5 forks source link

The precommit hooks must type check downstream usages #272

Closed samreid closed 2 years ago

samreid commented 2 years ago

From https://github.com/phetsims/chipper/issues/1174

I also realized that if you change code in e.g., sun you need to run the type checker not just on sun and its dependencies, but on things that use sun (things that have sun as a dependency) so you can make sure they didn’t break.

For instance, if you make a change to SUN/Panel, then the precommit hooks just check tsc -b in sun/, then the type won't check any sim usages of SUN/Panel. If an API was broken, the type checker wouldn't catch it.

This may be the reason some type errors have passed the precommit type checks and got pushed to master.

There are a few levels for how we could address this:

  1. Unsafe/fast/simple: Our current strategy of just tsc -b on the current repo, but this is unsafe since it doesn't type check usages.
  2. Safe/slow/simple: The safest strategy would be to run tsc -b on all for every precommit hook. However, that means that a change in sim 1 could prevent pushing to unrelated sim 2 which is undesirable. (Unless we are OK checking the type errors and seeing they are not related to sim 1 and turning off git hooks to push anyways).
  3. Safe/fast/complex: If we build our own dependency graph, we can detect which repos are downsteam of the changed repo, and run tsc -b on the downstream repos. This would be more complex since we would need to implement and maintain our own dependency graph.
  4. mostly safe/medium speed/pretty simple: Changes to common code repos could run tsc -b on all. Changes to sim repos could just run tsc -b on the sim repo. We would need to make sure that sim common code repos like "circuit-construction-kit-common" are counted as common code repos. But we wouldn't want type checking circuit-construction-kit-common to check for errors in geometric optics, say.

Based on these remarks, I'm thinking we should aim for (1) or (2).

@pixelzoom @jonathanolson or @zepumph what do you recommend?

samreid commented 2 years ago

This script takes about 24 seconds on my machine to find all direct usages (non-transitive) of Panel.ts exports:

```ts const { Project } = require( 'ts-morph' ); const getRepoList = require( '../common/getRepoList' ); const project = new Project( { tsConfigFilePath: 'chipper/tsconfig/all/tsconfig.json' } ); const sourceFile = project.addSourceFileAtPath( 'sun/js/Panel.ts' ); const repos = getRepoList( 'active-repos' ); repos.forEach( repo => { try { project.addSourceFilesFromTsConfig( repo + '/tsconfig.json' ); } catch( e ) { console.log( 'skipping: ' + repo ); } } ); // project.addSourceFileAtPath( 'area-builder/js/explore/view/BoardDisplayModePanel.js' ); for ( const [ name, declarations ] of sourceFile.getExportedDeclarations() ) { console.log( `${name}: ${declarations.map( d => d.getText() ).join( '', '' )}` ); declarations.forEach( declaration => { const referencedSymbols = declaration.findReferences(); for ( const referencedSymbol of referencedSymbols ) { for ( const reference of referencedSymbol.getReferences() ) { console.log( '---------' ); console.log( 'REFERENCE' ); console.log( '---------' ); console.log( 'File path: ' + reference.getSourceFile().getFilePath() ); console.log( 'Start: ' + reference.getTextSpan().getStart() ); console.log( 'Length: ' + reference.getTextSpan().getLength() ); console.log( 'Parent kind: ' + reference.getNode().getParentOrThrow().getKindName() ); console.log(); console.log(); } } } ); } ```

The question is whether that can be optimized and combined with a typescript command that only type checks a subset of files. I'm not sure whether that is compatible with project references. Also, do we need to trace transitive usages?

Also, this is pretty much what the tsc -b --watch is supposed to do. But we have reported false positives and false negatives using --watch recently.

samreid commented 2 years ago

It looks like TypeScript experimented with a tool that detected downstream places that needed to be type checked: https://github.com/microsoft/TypeScript/issues/13538#issuecomment-273695261

Fun fact: A previous implementation of TS tried to maintain a reverse mapping of dependencies such that given an invalidated node, you could determine which other nodes needed to be re-typechecked. This never worked 100%. I wrote a tool that iterated file edits and compared this "incremental" typecheck to the "clean" typecheck and produced a log file when the results were not the same; this tool never stopped finding bugs until we just gave up on the approach.

zepumph commented 2 years ago

I think that 0) is not workable. An improvement to it (IMO) would be to have zero type checking, so that the dev isn't lulled into a sense of security from the half measure. If there was no type checking, then it would be up to the dev to ensure that they run tsc -b on the whole project before committing, which is something that I am getting used to doing now. This is (of course) NOT the preferred strategy, but it may lead to less type checking errors in the interim.

In the long term, I WANT to trust that tsc will continually improve etc, so spinning up our own graph library may be wasted time, but I don't feel that confident. Especially with our a-typical project structure, perhaps we could create a script that can rebuild our project dependencies each night for hooks to use (your option (2)). But we wouldn't have to worry about this traversal on each commit. Then if you make a large refactor, grunt update or another script can be run to regenerate the graph before commit.

I also feel like perhaps part of this is on hold until we investigate if tsc can be improved generally in issues like https://github.com/phetsims/chipper/issues/1239 and perhaps others.

pixelzoom commented 2 years ago

Figuring out why --watch is so broken sounds like it might help this issue.

As for the cost to type-check... My vote is for a process that prevents checking in code that does not pass type-checking. It makes no sense to go "all in on TypeScript (and incur the costs therein), then choose to dumb-down/disable githooks, or require devs to take responsibility for type-checking.

jonathanolson commented 2 years ago

(1) would require developers to have everything checked out, no? Hooks slowing things down still seems quite unfortunate.

This might be a side note, but... is tsc -b able to be run in parallel 50 times if we're committing to 50 repos? I'd guess it would run into conflicts when the commits happen in parallel, tsc -b happens in parallel for all, and many processes try to read and write the same (changing) files at the same time... sounds like a recipe for disaster.

(2) We already partially have a dependency graph, no?

(0) is still an option for me personally. Waiting for pre-commit hooks has taken significantly more time from me than fixing up anything broken that happens.

samreid commented 2 years ago

(1) would require developers to have everything checked out, no?

In https://github.com/phetsims/chipper/issues/1245, we use include patterns for tsconfig, which are graceful (don't complain if a file isn't there).

Also, after https://github.com/phetsims/chipper/issues/1245 it seem the precommit hook should just run the full type checker. It is a bit faster now and may be ok.

This might be a side note, but... is tsc -b able to be run in parallel 50 times if we're committing to 50 repos?

Maybe in that case, I would recommend running the type checker/ lint check/etc once, then disabling precommit hooks for that commit.

samreid commented 2 years ago

I notified slack #typescript

@channel after https://github.com/phetsims/perennial/issues/272 precommit hooks runs the type checker on tsconfig/all. Hopefully this improves things, but if your experience is significantnly degraded let me know and we’ll fix it.

samreid commented 2 years ago

Marking for dev meeting public service announcement, and to see if the issue can be closed.

zepumph commented 2 years ago

PSA during dev meeting today. All sounded good. I had questions about having simA's type errors effect committing to simB. As a whole the dev team decided that we want zero type errors on the master always and forever, and so if you encounter that case, it should be rare. Just ignore it by not running with git hooks. Closing