Watch cli efficiency update

[johnfav03]

The watch CLI option previously recompiled the program on a set interval even when files didn't change; this PR implements several efficiency fixes for this feature including debouncing, caching config, and parsing dependencies to recompile only when files have been changed. This PR also fixes several open issues with the watcher.

Fixes #1317
Fixes #2317
Fixes #2640
Fixes #3014
Fixes #3015

[Copilot]

Pull request overview

This PR improves the tsgo --watch implementation to avoid rebuilding on a fixed interval when inputs haven’t changed, and adds coverage for a broad set of watch-trigger scenarios (file edits, config changes, module resolution, and filesystem operations).

Changes:

• Track compilation file-system dependencies and skip rebuilds when no watched inputs changed.
• Add debouncing logic for non-test watch cycles.
• Expand watch baseline coverage with new command-line watch scenarios.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/execute/watcher.go	Reworks watch to track accessed FS paths, detect changes via polling, debounce rebuilds, and reparse tsconfig for semantic change detection.
internal/execute/tsctests/tscwatch_test.go	Adds extensive new watch scenarios (file lifecycle, tsconfig changes, module resolution, symlinks).
testdata/baselines/reference/tscWatch/commandLineWatch/watch-skips-build-when-no-files-change.js	New baseline asserting no rebuild when no inputs change.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-rebuilds-when-file-is-modified.js	New baseline asserting rebuild on source edit.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-rebuilds-when-source-file-is-deleted.js	New baseline asserting rebuild/diagnostics when an imported source is deleted.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-new-file-resolving-failed-import.js	New baseline asserting rebuild when a missing import target is created.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-imported-file-added-in-new-directory.js	New baseline asserting rebuild when a new directory+file resolves an import.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-imported-directory-removed.js	New baseline asserting rebuild when an imported directory/file is removed.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-import-path-restructured.js	New baseline asserting rebuild when files move and import paths change.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-rebuilds-when-tsconfig-include-pattern-adds-file.js	New baseline asserting rebuild when include is widened and adds a file.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-rebuilds-when-tsconfig-is-modified-to-change-strict.js	New baseline asserting rebuild when compiler options change (strict).
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-file-added-to-previously-non-existent-include-path.js	New baseline asserting rebuild when a file appears under an include glob directory.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-node-modules-package-added.js	New baseline asserting rebuild when a missing node_modules package is installed.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-node-modules-package-removed.js	New baseline asserting rebuild when a previously resolved package is removed.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-handles-tsconfig-deleted.js	New baseline asserting behavior when tsconfig.json is deleted.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-handles-tsconfig-with-extends-base-modified.js	New baseline asserting rebuild when an extended base config changes.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-skips-rebuild-when-tsconfig-is-touched-but-content-unchanged.js	New baseline asserting no rebuild when tsconfig mtime changes but semantics don’t.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-with-tsconfig-files-list-entry-deleted.js	New baseline asserting rebuild when a files entry is removed via deletion.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-module-going-missing-then-coming-back.js	New baseline asserting rebuild when an imported module disappears and reappears.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-scoped-package-installed.js	New baseline asserting rebuild when a scoped node_modules package appears.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-package-json-main-field-edited.js	New baseline asserting rebuild when a package’s package.json changes (currently tests types field).
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-at-types-package-installed-later.js	New baseline asserting rebuild when @types/* is installed after an initial missing-types error.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-file-renamed-and-renamed-back.js	New baseline asserting rebuild when a file is renamed and renamed back.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-file-deleted-and-new-file-added-simultaneously.js	New baseline asserting rebuild when delete+create happen together with import update.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-handles-file-rapidly-recreated.js	New baseline asserting rebuild for rapid delete/recreate with changed content.
testdata/baselines/reference/tscWatch/commandLineWatch/watch-detects-change-in-symlinked-file.js	New baseline asserting rebuild when a symlink target changes.

[andrewbranch]

The two things I see off the bat that are missing that we discussed while whiteboarding:

1. This doesn't include the config's include wildcard directories. I think the watch detects file added to previously non-existent include path test only works because the edit actually creates the src directory. If the directory already existed and a new file was added into it, it looks like the watcher won't pick it up.
2. We talked about being able to plug this watcher into the LSP by separating the program building logic from the watching logic, but that isn't done here; it's all together. I don't think that's a blocker for right now, but I also don't think it's very much work to split it up—I want a watcher that takes a list of files or globs and calls a callback when something changes. I feel like if you just replace w.doBuild() with w.callback(), and have the program and caches owned by a different struct, you're pretty much there. If it's reasonably low-effort, it might be a good idea to decouple them now so it doesn't inadvertently grow harder to untangle over time.

[johnfav03]

2. We talked about being able to plug this watcher into the LSP by separating the program building logic from the watching logic, but that isn't done here; it's all together. I don't think that's a blocker for right now, but I also don't think it's very much work to split it up—I want a watcher that takes a list of files or globs and calls a callback when something changes. I feel like if you just replace w.doBuild() with w.callback(), and have the program and caches owned by a different struct, you're pretty much there. If it's reasonably low-effort, it might be a good idea to decouple them now so it doesn't inadvertently grow harder to untangle over time.

I just implemented this change by seperating the file tracking into a FileWatcher struct, which builds off of the previous watchState and adds a callback field. In the case of the CLI, this callback is linked to doCycle, which contains doBuild and all of the program construction logic. The caches, program construction, and emitting logic are all still owned by Watcher, which is wrapped around FileWatcher.

[andrewbranch]

I don't think 5ef0db6 is sufficient for wildcard watching—I can't quite figure out what the baseline tests are saying there (what does “incremental skips emit for new unreferenced file” mean?) but I think you’re going to have to read the directory (recursively if the pattern is recursive) to see if there are any new files.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

[andrewbranch]

Like Jake, I thought I saw some concurrency bugs, so I threw Copilot at it to write some tests to exercise them, and it did indeed find some with the race detector. Commit: 365407c

Summary:

  Races found (run with go test -race)

  Running the vfswatch tests produces 16+ distinct race warnings, and the watcher tests produce 394 race warnings across:

  ┌───────────────────────┬────────────────────────────────┬──────────────────────────────────────────────────────────────────┐
  │ Field                 │ Location                       │ Issue                                                            │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ WatchState            │ vfswatch.go:33,36,38,114,126   │ Plain map read in HasChanges, written in UpdateWatchedFiles      │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ WildcardDirectories   │ vfswatch.go:42,52,54,57,126    │ Plain map read in HasChanges/currentState, assigned in doBuild   │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ PollInterval          │ vfswatch.go                    │ Plain time.Duration read in Run, written in doBuild              │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ configModified        │ watcher.go:141,185             │ Plain bool read/written without sync                             │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ config                │ watcher.go:162-184,263-267     │ Pointer replaced in hasErrorsInTsConfig, read in doBuild         │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ program               │ watcher.go:176,203             │ Pointer replaced in doBuild, read in compileAndEmit              │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ configHasErrors       │ watcher.go:228,261             │ Plain bool read/written in hasErrorsInTsConfig                   │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ configFilePaths       │ watcher.go:229,262             │ Slice replaced without sync                                      │
  ├───────────────────────┼────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
  │ extendedConfigCache   │ watcher.go:159,267             │ Pointer replaced without sync                                    │
  └───────────────────────┴────────────────────────────────┴──────────────────────────────────────────────────────────────────┘

[johnfav03]

Like Jake, I thought I saw some concurrency bugs, so I threw Copilot at it to write some tests to exercise them, and it did indeed find some with the race detector.

Thanks for the race tests! I added mutexes to the Watcher and FileWatcher structs to govern access to WatchState and WildcardDirectories. Also, I had copilot analyze the same sets of tests and it doesn't turn up any race warnings now.

[andrewbranch]

So, right now, there's a kind of confusing interplay between SetWildcardDirectories and UpdateWatchedFiles. Both are different ways of controlling what the watcher scans, and the latter depends on a very specific type that's tailor made for the compiler implementation. I think what we want is just SetWildcardDirectories. It should be the compiler runner's job to come up with the right set of directories to watch rather than passing in a whole specialized file system implementation to expose which files were touched. Basically, just move the SeenFiles iteration out of vfswatch and into execute, so execute can come up with a combined set of directories to watch (see how the LSP does this with the tspath.GetCommonParents helper).

One result of this refactor will be that you lose some fidelity—the watcher will fire its callback on some files that you don't care about if SeenFiles includes directories that aren't also covered by wildcards. To handle that, you'll need the file watcher to pass arguments to its callback summarizing what changed—what files were created, deleted, and changed—i.e., each scan will have to be complete instead of returning early as soon as it's found something that changed. With that, the DoCycle method can look at what files changed and consult SeenFiles and its config wildcard directories to see whether a rebuild is actually necessary. The set of watch changes is also needed if we want to have the option to use the watcher in the LSP.

This is different from the esbuild watcher, which calls its callback with no info as soon as it finds any dirty path. I feel like we might have led you astray with that reference; sorry about that 😕.
https://go-review.googlesource.com/c/tools/+/754320/21/gopls/internal/filewatcher/poll_watcher.go#111 is a reference that does a complete scan.

I think my feedback can be split into two parts:

1. Needed before merge: FileWatcher should have a single API point for controlling what to watch, and that type should be defined in its own package, not coupled with a FS implementation. It should only be called once per program build, with both the wildcard directories and set of seen files in the same call. This is basically a code hygiene thing but keeps basically the same strategy.
2. Debatable whether to do before merge: switch to a directory-only API and complete scan, passing event info to the callback. This is needed if we're going to use the watcher in the language server, and I think the biggest motivator for that is that VS apparently doesn't support file watching over LSP directly, so we need to do something there. But @jakebailey had suggested to @joj that maybe the VS client could shim this functionality itself instead, and I'm not sure if there's consensus on what's going to happen there.

I'd like to get Jake's input before deciding whether to pursue (2) now.

[jakebailey]

I don't want to block this PR on being able to use it within the LSP, really; if it's going to take too long I think it's optional given the CLI stuff is important.

But, we know there are clients that can't do watch mode at all, so I think it's in general worth making work if we can get it out faster than it takes to hack the VS LSP client. @joj or @navya9singh would know better how hard that is to manage, but I think it's "as simple" as handling our watch requests manually and then just using dotnet's watcher to watch a dir recursively, as that is actually built into dotnet.

[johnfav03]

To address the first suggestion, I combined SetWildcardDirectories and UpdateWatchedFiles into UpdateWatchState which takes in both wildcard dirs and seen files. Once we get more feedback on whether it's better to handle file watching via the LSP or the client, I'll open a new PR and make the changes for the second suggestion there.