Fix nil dereference crash in loadModuleFromSpecificNodeModulesDirectory (#3941)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>

Author: 3 people

internal/module/resolver.go

@@ -1134,7 +1134,7 @@ func (r *resolutionState) loadModuleFromSpecificNodeModulesDirectory(ext extensi
 			// https://github.com/microsoft/TypeScript/pull/49327
 			return r.loadModuleFromExports(packageInfo, ext, tspath.CombinePaths(".", rest))
 		}
-		if rest != "" {
+		if rest != "" && packageInfo.Exists() {
 			versionPaths := packageInfo.Contents.GetVersionPaths(r.getTraceFunc())
 			if versionPaths.Exists() {
 				if r.tracer != nil {

internal/module/resolver_test.go

@@ -5,7 +5,6 @@ import (
 	"sync"
 	"sync/atomic"
 	"testing"
-	"time"
 
 	"github.com/microsoft/typescript-go/internal/core"
 	"github.com/microsoft/typescript-go/internal/module"
@@ -51,25 +50,80 @@ func TestResolveModuleNameTrailingSlash(t *testing.T) {
 }
 
 // blockingFS wraps a vfs.FS and forces FileExists calls for `targetPath` to
-// block on `gate` until released. It also counts how many goroutines are
-// waiting at the gate. This is used to deterministically reproduce the
+// block on `gate` until released. Each caller sends on `arrived` when it
+// reaches the gate. This is used to deterministically reproduce the
 // `package.json` info-cache insert race described in
 // https://github.com/microsoft/typescript-go/issues/3526.
 type blockingFS struct {
 	vfs.FS
 	targetPath string
 	gate       chan struct{}
-	waiting    atomic.Int32
+	arrived    chan struct{} // each blocked goroutine sends one value
+}
+
+// waitForSignal waits for a synchronization point in these race regression
+// tests and converts deadlocks into deterministic test failures.
+func waitForSignal(t *testing.T, ch <-chan struct{}, description string) {
+	t.Helper()
+	select {
+	case <-ch:
+		return
+	case <-t.Context().Done():
+		t.Fatalf("timed out waiting for %s", description)
+	}
 }
 
 func (f *blockingFS) FileExists(path string) bool {
 	if path == f.targetPath {
-		f.waiting.Add(1)
+		f.arrived <- struct{}{}
 		<-f.gate
 	}
 	return f.FS.FileExists(path)
 }
 
+// flipFileExistsFS wraps a vfs.FS and returns false for the first
+// FileExists call to `targetPath`, then true for the second. Both calls
+// signal arrival via channel then block until released via their respective
+// gate channels. ReadFile for the target path also signals arrival then
+// blocks, so the "file doesn't exist" Set completes before the "file exists"
+// Set (reproducing the LoadOrStore race).
+type flipFileExistsFS struct {
+	vfs.FS
+	targetPath    string
+	callCount     atomic.Int32
+	firstArrived  chan struct{} // closed when the first FileExists caller arrives
+	secondArrived chan struct{} // closed when the second FileExists caller arrives
+	firstGate     chan struct{}
+	secondGate    chan struct{}
+	readArrived   chan struct{} // closed when ReadFile caller arrives
+	readGate      chan struct{}
+}
+
+func (f *flipFileExistsFS) FileExists(path string) bool {
+	if path == f.targetPath {
+		n := f.callCount.Add(1)
+		if n == 1 {
+			close(f.firstArrived)
+			<-f.firstGate
+			return false // first caller: simulate "file not yet visible"
+		}
+		if n == 2 {
+			close(f.secondArrived)
+			<-f.secondGate
+			return f.FS.FileExists(path) // second caller: file is visible
+		}
+	}
+	return f.FS.FileExists(path)
+}
+
+func (f *flipFileExistsFS) ReadFile(path string) (string, bool) {
+	if path == f.targetPath {
+		close(f.readArrived)
+		<-f.readGate
+	}
+	return f.FS.ReadFile(path)
+}
+
 // Regression test for https://github.com/microsoft/typescript-go/issues/3526.
 //
 // Two goroutines resolve the same package via specifiers that differ only by
@@ -106,6 +160,7 @@ func TestResolveModuleNameTrailingSlashRace(t *testing.T) {
 		FS:         vfstest.FromMap(files, true),
 		targetPath: pkgJSONPath,
 		gate:       make(chan struct{}),
+		arrived:    make(chan struct{}, 2),
 	}
 	host := &resolutionHostStub{fs: fs, cwd: "/repo"}
 	opts := &core.CompilerOptions{
@@ -115,11 +170,11 @@ func TestResolveModuleNameTrailingSlashRace(t *testing.T) {
 	}
 	resolver := module.NewResolver(host, opts, "", "")
 
-	type result struct {
+	type resolutionResult struct {
 		name     string
 		resolved bool
 	}
-	results := make(chan result, 2)
+	results := make(chan resolutionResult, 2)
 	var wg sync.WaitGroup
 	for _, name := range []string{"pkg", "pkg/"} {
 		containingFile := "/repo/src/a/file.ts"
@@ -128,19 +183,14 @@ func TestResolveModuleNameTrailingSlashRace(t *testing.T) {
 		}
 		wg.Go(func() {
 			r, _ := resolver.ResolveModuleName(name, containingFile, core.ModuleKindESNext, nil)
-			results <- result{name, r.IsResolved()}
+			results <- resolutionResult{name, r.IsResolved()}
 		})
 	}
 
 	// Wait for both goroutines to reach the FileExists gate, guaranteeing
 	// both have observed a package.json info-cache miss.
-	deadline := time.Now().Add(5 * time.Second)
-	for fs.waiting.Load() < 2 {
-		if time.Now().After(deadline) {
-			t.Fatalf("timed out waiting for both goroutines to reach FileExists gate; got %d", fs.waiting.Load())
-		}
-		time.Sleep(time.Millisecond)
-	}
+	waitForSignal(t, fs.arrived, "first FileExists gate arrival")
+	waitForSignal(t, fs.arrived, "second FileExists gate arrival")
 	close(fs.gate)
 
 	wg.Wait()
@@ -151,3 +201,99 @@ func TestResolveModuleNameTrailingSlashRace(t *testing.T) {
 		}
 	}
 }
+
+// Regression test for https://github.com/microsoft/typescript-go/issues/1290.
+//
+// Two goroutines resolve `pkg/sub` concurrently. Both miss the package.json
+// info-cache for the root package directory. A `flipFileExistsFS` forces the
+// first goroutine's `FileExists` to return false (simulating the file not yet
+// being visible), so it stores a nil-Contents cache entry. The second
+// goroutine's `FileExists` returns true, but its `Set` call (`LoadOrStore`)
+// returns the first goroutine's nil-Contents entry. Without the `Exists()`
+// guard on the `typesVersions` lookup, `packageInfo.Contents.GetVersionPaths`
+// dereferences nil and panics. With the guard the nil-Contents entry is safely
+// skipped.
+func TestResolveSubpathNilContentsRace(t *testing.T) {
+	t.Parallel()
+
+	const rootPkgJSON = "/repo/node_modules/pkg/package.json"
+	files := map[string]string{
+		rootPkgJSON:                             `{"name":"pkg","version":"1.0.0"}`,
+		"/repo/node_modules/pkg/sub/index.d.ts": "export declare const sub: number;",
+		"/repo/node_modules/pkg/sub/index.js":   "exports.sub = 1;",
+		"/repo/src/a/file.ts":                   "",
+		"/repo/src/b/file.ts":                   "",
+	}
+	fs := &flipFileExistsFS{
+		FS:            vfstest.FromMap(files, true),
+		targetPath:    rootPkgJSON,
+		firstArrived:  make(chan struct{}),
+		secondArrived: make(chan struct{}),
+		firstGate:     make(chan struct{}),
+		secondGate:    make(chan struct{}),
+		readArrived:   make(chan struct{}),
+		readGate:      make(chan struct{}),
+	}
+	host := &resolutionHostStub{fs: fs, cwd: "/repo"}
+	opts := &core.CompilerOptions{
+		ModuleResolution: core.ModuleResolutionKindBundler,
+		Module:           core.ModuleKindESNext,
+		Target:           core.ScriptTargetESNext,
+	}
+	resolver := module.NewResolver(host, opts, "", "")
+
+	var panicked atomic.Bool
+	type resolutionResult struct {
+		containingFile string
+		resolved       bool
+	}
+	results := make(chan resolutionResult, 2)
+	var wg sync.WaitGroup
+	// Two goroutines both resolve "pkg/sub". Each calls getPackageJsonInfo
+	// for the root package directory, reaching FileExists for rootPkgJSON.
+	for _, containingFile := range []string{"/repo/src/a/file.ts", "/repo/src/b/file.ts"} {
+		wg.Go(func() {
+			resolved := false
+			defer func() {
+				if r := recover(); r != nil {
+					panicked.Store(true)
+				}
+				results <- resolutionResult{containingFile: containingFile, resolved: resolved}
+			}()
+			r, _ := resolver.ResolveModuleName("pkg/sub", containingFile, core.ModuleKindESNext, nil)
+			resolved = r.IsResolved()
+		})
+	}
+
+	// Phase 1: Wait for both goroutines to reach FileExists for the root
+	// package.json, guaranteeing both have observed a cache miss.
+	waitForSignal(t, fs.firstArrived, "first root package.json FileExists arrival")
+	waitForSignal(t, fs.secondArrived, "second root package.json FileExists arrival")
+
+	// Phase 2: Release the first FileExists caller (returns false).
+	// It enters the "file not found" branch and stores a nil-Contents entry
+	// via Set — this is nearly instant (no ReadFile).
+	close(fs.firstGate)
+
+	// Phase 3: Release the second FileExists caller (returns true).
+	// It proceeds to ReadFile, which we gate separately to ensure the first
+	// goroutine's nil-Contents Set has completed.
+	close(fs.secondGate)
+
+	// Phase 4: Wait for the second goroutine to reach ReadFile, then release.
+	// By this point the first goroutine has stored its nil-Contents entry.
+	// The second goroutine's Set (LoadOrStore) will return that stale entry.
+	waitForSignal(t, fs.readArrived, "root package.json ReadFile arrival")
+	close(fs.readGate)
+
+	wg.Wait()
+	close(results)
+	if panicked.Load() {
+		t.Fatal("resolver panicked due to nil Contents dereference in loadModuleFromSpecificNodeModulesDirectory")
+	}
+	for r := range results {
+		if !r.resolved {
+			t.Fatalf("%q failed to resolve pkg/sub", r.containingFile)
+		}
+	}
+}