unbreak symbol lookup in remote modules

bufbuild · Oct 8, 2024 · 42ee60a · 42ee60a
1 parent 153ccd6
commit 42ee60a
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 77 deletions.
diff --git a/private/buf/buflsp/file.go b/private/buf/buflsp/file.go
@@ -297,7 +297,7 @@ func (f *file) IndexImports(ctx context.Context) {
 		if f.importablePathToObject == nil {
 			return
 		}
-	} else {
+	} else if f.importablePathToObject == nil {
 		f.importablePathToObject = importable
 	}
 
@@ -320,10 +320,31 @@ func (f *file) IndexImports(ctx context.Context) {
 			continue
 		}
 
+		// If this is an external file, it will be in the cache and therefore
+		// finding imports via lsp.findImportable() will not work correctly:
+		// the bucket for the workspace found for a dependency will have
+		// truncated paths, and those workspace files will appear to be
+		// local rather than external.
+		//
+		// Thus, we search for name and all of its path suffixes. This is not
+		// ideal but is our only option in this case.
+		var fileInfo storage.ObjectInfo
 		name := node.Name.AsString()
-		fileInfo, ok := importable[name]
-		if !ok {
-			f.lsp.logger.Warn(fmt.Sprintf("could not find URI for import %q", name))
+		for {
+			fileInfo, ok = importable[name]
+			if ok {
+				break
+			}
+
+			idx := strings.Index(name, "/")
+			if idx == -1 {
+				break
+			}
+
+			name = name[idx+1:]
+		}
+		if fileInfo == nil {
+			f.lsp.logger.Warn(fmt.Sprintf("could not find URI for import %q", node.Name.AsString()))
 			continue
 		}
 
@@ -338,27 +359,6 @@ func (f *file) IndexImports(ctx context.Context) {
 			imported = f
 		} else {
 			imported = f.Manager().Open(ctx, protocol.URI("file://"+fileInfo.LocalPath()))
-
-			isLocal := fileInfo.LocalPath() == fileInfo.ExternalPath()
-			if !isLocal && imported.importablePathToObject == nil {
-				// If this is an external file, it will be in the cache and therefore
-				// finding imports via lsp.findImportable() will not work correctly:
-				// the bucket for the workspace found for a dependency will have
-				// truncated paths, and those workspace files will appear to be
-				// local rather than external. It is not clear that GetWorkspace
-				// is intended for use with files in the cache.
-				//
-				// However, we can just re-use this file's import map, because:
-				//
-				// 1. It was computed from a local file path and has not
-				//    truncated file paths, and contains this file and all of
-				//    its dependencies, or
-				//
-				// 2. it is the import map of some local file that depends on f
-				//    and which copied its import map into it here in a previous
-				//    step.
-				imported.importablePathToObject = f.importablePathToObject
-			}
 		}
 
 		imported.objectInfo = fileInfo
@@ -672,6 +672,7 @@ func findImportable(
 	imports := make(map[string]storage.ObjectInfo)
 	for _, module := range workspace.Modules() {
 		err = module.WalkFileInfos(ctx, func(fileInfo bufmodule.FileInfo) error {
+
 			if fileInfo.FileType() != bufmodule.FileTypeProto {
 				return nil
 			}

diff --git a/private/buf/buflsp/symbol.go b/private/buf/buflsp/symbol.go
@@ -332,58 +332,46 @@ func (s *symbol) ResolveCrossFile(ctx context.Context) {
 	}
 }
 
-// func (s *symbol) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) {
-// 	enc.AddString("file", s.file.uri.Filename())
-
-// 	// zapPos converts an ast.SourcePos into a zap marshaller.
-// 	zapPos := func(pos ast.SourcePos) zapcore.ObjectMarshalerFunc {
-// 		return func(enc zapcore.ObjectEncoder) error {
-// 			enc.AddInt("offset", pos.Offset)
-// 			enc.AddInt("line", pos.Line)
-// 			enc.AddInt("col", pos.Col)
-// 			return nil
-// 		}
-// 	}
-
-// 	err = enc.AddObject("start", zapPos(s.info.Start()))
-// 	if err != nil {
-// 		return err
-// 	}
-
-// 	err = enc.AddObject("end", zapPos(s.info.End()))
-// 	if err != nil {
-// 		return err
-// 	}
-
-// 	switch kind := s.kind.(type) {
-// 	case *builtin:
-// 		enc.AddString("builtin", kind.name)
-
-// 	case *import_:
-// 		if kind.file != nil {
-// 			enc.AddString("imports", kind.file.uri.Filename())
-// 		}
-
-// 	case *definition:
-// 		enc.AddString("defines", strings.Join(kind.path, "."))
-
-// 	case *reference:
-// 		if kind.file != nil {
-// 			enc.AddString("imports", kind.file.uri.Filename())
-// 		}
-// 		if kind.path != nil {
-// 			enc.AddString("references", strings.Join(kind.path, "."))
-// 		}
-// 		if kind.seeTypeOf != nil {
-// 			err = enc.AddObject("see_type_of", kind.seeTypeOf)
-// 			if err != nil {
-// 				return err
-// 			}
-// 		}
-// 	}
-
-// 	return nil
-// }
+func (s *symbol) LogValue() slog.Value {
+	attrs := []slog.Attr{slog.String("file", s.file.uri.Filename())}
+
+	// pos converts an ast.SourcePos into a slog.Value.
+	pos := func(pos ast.SourcePos) slog.Value {
+		return slog.GroupValue(
+			slog.Int("line", pos.Line),
+			slog.Int("col", pos.Col),
+		)
+	}
+
+	attrs = append(attrs, slog.Any("start", pos(s.info.Start())))
+	attrs = append(attrs, slog.Any("end", pos(s.info.End())))
+
+	switch kind := s.kind.(type) {
+	case *builtin:
+		attrs = append(attrs, slog.String("builtin", kind.name))
+
+	case *import_:
+		if kind.file != nil {
+			attrs = append(attrs, slog.String("imports", kind.file.uri.Filename()))
+		}
+
+	case *definition:
+		attrs = append(attrs, slog.String("defines", strings.Join(kind.path, ".")))
+
+	case *reference:
+		if kind.file != nil {
+			attrs = append(attrs, slog.String("imports", kind.file.uri.Filename()))
+		}
+		if kind.path != nil {
+			attrs = append(attrs, slog.String("references", strings.Join(kind.path, ".")))
+		}
+		if kind.seeTypeOf != nil {
+			attrs = append(attrs, slog.Any("see_type_of", kind.seeTypeOf))
+		}
+	}
+
+	return slog.GroupValue(attrs...)
+}
 
 // FormatDocs finds appropriate documentation for the given s and constructs a Markdown
 // string for showing to the client.