From 9cb66939042cb63acfcee325fec94a8061b0e872 Mon Sep 17 00:00:00 2001 From: karitham Date: Sat, 8 Aug 2026 01:57:27 +0200 Subject: [PATCH] lsp: use FsPath for filesystem access so Windows URIs resolve; drop dead resolver code --- lsp/cache/fs_memoized.go | 7 +- lsp/cache/snapshot.go | 4 +- lsp/initialize.go | 5 +- lsp/source/completion_utils.go | 7 +- lsp/source/name.go | 17 ++- lsp/source/provider.go | 2 +- resolver/integration_test.go | 205 --------------------------------- resolver/resolver.go | 50 +++----- 8 files changed, 40 insertions(+), 257 deletions(-) diff --git a/lsp/cache/fs_memoized.go b/lsp/cache/fs_memoized.go index 85c28db..4d79706 100644 --- a/lsp/cache/fs_memoized.go +++ b/lsp/cache/fs_memoized.go @@ -44,7 +44,7 @@ func (h *DiskFile) Content() ([]byte, error) { return h.content, h.err } // ReadFile stats and (maybe) reads the file, updates the cache, and returns it. func (fs *memoizedFS) ReadFile(ctx context.Context, uri uri.URI) (FileHandle, error) { - id, mtime, err := GetFileID(uri.Path()) + id, mtime, err := GetFileID(uri.FsPath()) if err != nil { // file does not exist return &DiskFile{ @@ -124,7 +124,10 @@ func readFile(ctx context.Context, uri uri.URI, mtime time.Time) (*DiskFile, err // ID, or whose mtime differs from the given mtime. However, in these cases // we expect the client to notify of a subsequent file change, and the file // content should be eventually consistent. - content, err := os.ReadFile(uri.Path()) // ~20us + // FsPath, not Path: Path keeps the leading slash before a Windows drive + // letter ("/c:/dir/x.thrift"), which Win32 rejects with an invalid-name + // error. + content, err := os.ReadFile(uri.FsPath()) // ~20us if err != nil { content = nil // just in case } diff --git a/lsp/cache/snapshot.go b/lsp/cache/snapshot.go index 20ebe43..c4cf6de 100644 --- a/lsp/cache/snapshot.go +++ b/lsp/cache/snapshot.go @@ -94,7 +94,9 @@ func (r *Resolver) IncludePaths() []string { // ResolveInclude resolves an include path to a file URI. // It first tries relative to the current file, then tries each include path. func (r *Resolver) ResolveInclude(cur uri.URI, includePath string) uri.URI { - filePath := cur.Path() + // FsPath, not Path: Path keeps the leading slash before a Windows drive + // letter ("/c:/dir/x.thrift"), which breaks the resolver's filepath ops. + filePath := cur.FsPath() resolvedPath := r.central.Resolve(filePath, includePath) return uri.File(resolvedPath) diff --git a/lsp/initialize.go b/lsp/initialize.go index e539654..b33977c 100644 --- a/lsp/initialize.go +++ b/lsp/initialize.go @@ -122,8 +122,9 @@ func (s *Server) walkFoldersThriftFile(folder uri.URI) { // resolve to it via ContainsFile. s.session.AddView(folder) - // WalkDir walk files with lexical order - _ = filepath.WalkDir(folder.Path(), func(path string, d fs.DirEntry, err error) error { + // WalkDir walk files with lexical order. FsPath, not Path: Path keeps + // the leading slash before a Windows drive letter, which Win32 rejects. + _ = filepath.WalkDir(folder.FsPath(), func(path string, d fs.DirEntry, err error) error { slog.Debug("walking", "path", path) if err != nil { diff --git a/lsp/source/completion_utils.go b/lsp/source/completion_utils.go index 87ea4a6..f7c4dbe 100644 --- a/lsp/source/completion_utils.go +++ b/lsp/source/completion_utils.go @@ -2,6 +2,7 @@ package source import ( "os" + "path" "path/filepath" "strings" @@ -38,7 +39,7 @@ func ListDirAndFiles(dir string, includePaths []string, prefix string) []Candida var res []Candidate for _, root := range roots { - entries, err := os.ReadDir(filepath.Join(root, dirPart)) + entries, err := os.ReadDir(filepath.Join(root, filepath.FromSlash(dirPart))) if err != nil { continue } @@ -53,9 +54,9 @@ func ListDirAndFiles(dir string, includePaths []string, prefix string) []Candida switch { case e.IsDir(): - text = filepath.Join(dirPart, name) + "/" + text = path.Join(dirPart, name) + "/" case strings.HasSuffix(name, ".thrift"): - text = filepath.Join(dirPart, name) + text = path.Join(dirPart, name) default: continue } diff --git a/lsp/source/name.go b/lsp/source/name.go index dd73b00..cd8dd0c 100644 --- a/lsp/source/name.go +++ b/lsp/source/name.go @@ -1,7 +1,7 @@ package source import ( - "path/filepath" + "path" "sort" "strings" @@ -13,14 +13,11 @@ import ( // includeNameOf returns the include name of a file URI: the base name // without extension. file:///base.thrift -> "base". func includeNameOf(file uri.URI) string { - fileName := file.Path() + // URI paths are always slash-separated, even for Windows drive + // letters, so path (not filepath) is the matching stdlib. + fileName := path.Base(file.Path()) - index := strings.LastIndexByte(fileName, filepath.Separator) - if index != -1 { - fileName = string(fileName[index+1:]) - } - - index = strings.LastIndexByte(fileName, '.') + index := strings.LastIndexByte(fileName, '.') if index == -1 { return fileName } @@ -57,8 +54,8 @@ func parseIdent(cur uri.URI, includes []*syntax.Include, identifier string) (inc // includeNames returns include names from include ast nodes func includeNames(cur uri.URI, includes []*syntax.Include) (includeNames []string) { for _, inc := range includes { - if path := inc.PathText(); path != "" { - u := uri.File(filepath.Join(filepath.Dir(cur.Path()), path)) + if p := inc.PathText(); p != "" { + u := uri.File(path.Join(path.Dir(cur.Path()), p)) includeNames = append(includeNames, includeNameOf(u)) } } diff --git a/lsp/source/provider.go b/lsp/source/provider.go index d5857a8..3b82110 100644 --- a/lsp/source/provider.go +++ b/lsp/source/provider.go @@ -58,7 +58,7 @@ type includeProvider struct{} func (includeProvider) Kind() ContextKind { return CtxIncludePath } func (includeProvider) Candidates(_ context.Context, ss *cache.Snapshot, file uri.URI, c Context) []Candidate { - return ListDirAndFiles(filepath.Dir(file.Path()), ss.Resolver().IncludePaths(), c.Prefix) + return ListDirAndFiles(filepath.Dir(file.FsPath()), ss.Resolver().IncludePaths(), c.Prefix) } type typeProvider struct{} diff --git a/resolver/integration_test.go b/resolver/integration_test.go index e3d3b32..3458663 100644 --- a/resolver/integration_test.go +++ b/resolver/integration_test.go @@ -108,211 +108,6 @@ struct User { } } -func TestResolver_Integration_ResolutionOrder(t *testing.T) { - // Test that include paths are checked before relative resolution - tmpDir := t.TempDir() - includeDir := filepath.Join(tmpDir, "includes") - srcDir := filepath.Join(tmpDir, "src") - - if err := os.MkdirAll(includeDir, 0o755); err != nil { - t.Fatal(err) - } - - if err := os.MkdirAll(srcDir, 0o755); err != nil { - t.Fatal(err) - } - - // Create a version in include directory - includeVersion := filepath.Join(includeDir, "shared.thrift") - - includeContent := `namespace * shared - -struct SharedInInclude { - 1: string value -}` - if err := os.WriteFile(includeVersion, []byte(includeContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create a different version in src directory - srcVersion := filepath.Join(srcDir, "shared.thrift") - - srcContent := `namespace * shared - -struct SharedInSrc { - 1: string name -}` - if err := os.WriteFile(srcVersion, []byte(srcContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create main file in src - mainFile := filepath.Join(srcDir, "main.thrift") - - mainContent := `include "shared.thrift" - -namespace * test - -struct Data { - 1: string field -}` - if err := os.WriteFile(mainFile, []byte(mainContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create resolver with include path - r := New([]string{includeDir}) - - // Test: IncludeCall should find include path first, not relative - includeCall := r.IncludeCall(mainFile) - - filename, content, err := includeCall("shared.thrift") - if err != nil { - t.Fatalf("failed to resolve shared.thrift: %v", err) - } - - // Should have found the include directory version - if filename != includeVersion { - t.Errorf("expected include path version %q, got %q", includeVersion, filename) - } - - if string(content) != includeContent { - t.Error("expected content from include directory") - } -} - -func TestResolver_Integration_RelativeFallback(t *testing.T) { - // Test that relative resolution works when include paths don't have the file - tmpDir := t.TempDir() - - // Create file in root (no include directories) - localFile := filepath.Join(tmpDir, "local.thrift") - - localContent := `namespace * local - -struct LocalData { - 1: string value -}` - if err := os.WriteFile(localFile, []byte(localContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create main file that references local - mainFile := filepath.Join(tmpDir, "main.thrift") - - mainContent := `include "local.thrift" - -namespace * main - -struct Container { - 1: LocalData data -}` - if err := os.WriteFile(mainFile, []byte(mainContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create resolver without include paths - r := New([]string{}) - - // Test: Should fall back to relative resolution - includeCall := r.IncludeCall(mainFile) - - filename, content, err := includeCall("local.thrift") - if err != nil { - t.Fatalf("failed to resolve local.thrift: %v", err) - } - - if filename != localFile { - t.Errorf("expected %q, got %q", localFile, filename) - } - - if string(content) != localContent { - t.Error("content mismatch") - } -} - -func TestResolver_Integration_MultipleIncludePaths(t *testing.T) { - // Test resolution order across multiple include paths - tmpDir := t.TempDir() - includeDir1 := filepath.Join(tmpDir, "includes1") - includeDir2 := filepath.Join(tmpDir, "includes2") - - if err := os.MkdirAll(includeDir1, 0o755); err != nil { - t.Fatal(err) - } - - if err := os.MkdirAll(includeDir2, 0o755); err != nil { - t.Fatal(err) - } - - // File only in include path 2 - file2 := filepath.Join(includeDir2, "unique.thrift") - - content2 := `namespace * unique - -struct UniqueInDir2 { - 1: string value -}` - if err := os.WriteFile(file2, []byte(content2), 0o644); err != nil { - t.Fatal(err) - } - - // File in both include paths (dir1 should win) - fileBoth1 := filepath.Join(includeDir1, "both.thrift") - - contentBoth1 := `namespace * both - -struct BothFromDir1 { - 1: string value -}` - if err := os.WriteFile(fileBoth1, []byte(contentBoth1), 0o644); err != nil { - t.Fatal(err) - } - - fileBoth2 := filepath.Join(includeDir2, "both.thrift") - - contentBoth2 := `namespace * both - -struct BothFromDir2 { - 1: string name -}` - if err := os.WriteFile(fileBoth2, []byte(contentBoth2), 0o644); err != nil { - t.Fatal(err) - } - - mainFile := filepath.Join(tmpDir, "main.thrift") - - mainContent := `include "unique.thrift" -include "both.thrift"` - if err := os.WriteFile(mainFile, []byte(mainContent), 0o644); err != nil { - t.Fatal(err) - } - - // Create resolver with multiple include paths (dir1 first) - r := New([]string{includeDir1, includeDir2}) - includeCall := r.IncludeCall(mainFile) - - // Test: unique.thrift should be found in includeDir2 - filename, _, err := includeCall("unique.thrift") - if err != nil { - t.Fatalf("failed to resolve unique.thrift: %v", err) - } - - if filename != file2 { - t.Errorf("expected %q, got %q", file2, filename) - } - - // Test: both.thrift should be found in includeDir1 (first in list) - filename2, _, err := includeCall("both.thrift") - if err != nil { - t.Fatalf("failed to resolve both.thrift: %v", err) - } - - if filename2 != fileBoth1 { - t.Errorf("expected first include path %q, got %q", fileBoth1, filename2) - } -} - func TestResolver_Integration_DeeplyNestedIncludes(t *testing.T) { // Test deeply nested include chain: a -> b -> c -> d tmpDir := t.TempDir() diff --git a/resolver/resolver.go b/resolver/resolver.go index b8d84b4..77bab6d 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -12,11 +12,28 @@ import ( // paths. type absFS struct{ fsys fs.FS } +// isWindowsPath reports whether name is a native Windows path (drive letter +// or backslash separators). Such names cannot go through an fs.FS, which +// only accepts slash-separated relative names; the underlying fs is the +// real filesystem anyway (tests use POSIX names), so go straight to the OS. +func isWindowsPath(name string) bool { + return strings.Contains(name, `\`) || + (len(name) >= 2 && name[1] == ':' && (name[0]|0x20) >= 'a' && (name[0]|0x20) <= 'z') +} + func (a absFS) Open(name string) (fs.File, error) { + if isWindowsPath(name) { + return os.Open(name) + } + return a.fsys.Open(strings.TrimPrefix(name, "/")) } func (a absFS) Stat(name string) (fs.FileInfo, error) { + if isWindowsPath(name) { + return os.Stat(name) + } + return fs.Stat(a.fsys, strings.TrimPrefix(name, "/")) } @@ -78,36 +95,3 @@ func (r *Resolver) exists(path string) bool { return err == nil } - -// IncludeCall is a function that resolves and reads an include file. -type IncludeCall func(include string) (filename string, content []byte, err error) - -// ResolveContent resolves an include path and reads the file content. -func (r *Resolver) ResolveContent(currentFile, includePath string) (filename string, content []byte, err error) { - filename = r.Resolve(currentFile, includePath) - - content, err = fs.ReadFile(r.fsys, filename) - if err != nil { - return filename, nil, err - } - - return filename, content, nil -} - -// IncludeCall creates an IncludeCall function for the given file. The -// returned function resolves includes using include paths first, then falls -// back to relative resolution from initialFile. -func (r *Resolver) IncludeCall(initialFile string) IncludeCall { - return func(include string) (filename string, content []byte, err error) { - for _, ip := range r.includePaths { - candidatePath := filepath.Join(ip, include) - if r.exists(candidatePath) { - content, err = fs.ReadFile(r.fsys, candidatePath) - - return candidatePath, content, err - } - } - - return r.ResolveContent(initialFile, include) - } -} -- 2.51.2