Skip to content

Commit

Permalink
Fix synthetic sources missing custom options
Browse files Browse the repository at this point in the history
  • Loading branch information
kralicky committed Jun 19, 2024
1 parent bc8ef1a commit 0007177
Show file tree
Hide file tree
Showing 14 changed files with 101 additions and 51 deletions.
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/google/uuid v1.6.0
github.com/kralicky/codegen v0.0.0-20240522012557-5193d3fdbeca
github.com/kralicky/gpkg v0.0.0-20240119195700-64f32830b14f
github.com/kralicky/protocompile v0.0.0-20240515213713-67f2e10cff9b
github.com/kralicky/protocompile v0.0.0-20240619180838-57d9401fcbbc
github.com/kralicky/tools-lite v0.0.0-20240313161632-60bfa88304ff
github.com/mattn/go-tty v0.0.5
github.com/mitchellh/mapstructure v1.5.0
Expand All @@ -33,12 +33,12 @@ require (
github.com/iancoleman/strcase v0.3.0 // indirect
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
github.com/kralicky/go-adaptive-radix-tree v0.0.0-20240619012453-a8f80032ba31 // indirect
github.com/mattn/go-colorable v0.1.4 // indirect
github.com/mattn/go-isatty v0.0.10 // indirect
github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b // indirect
github.com/olebedev/when v1.0.0 // indirect
github.com/pkg/errors v0.8.1 // indirect
github.com/plar/go-adaptive-radix-tree v1.0.5 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/prometheus/client_model v0.6.0 // indirect
github.com/prometheus/common v0.53.0 // indirect
Expand Down
12 changes: 6 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
github.com/kralicky/codegen v0.0.0-20240522012557-5193d3fdbeca h1:fOocXyJC76EbMkhVUU07PWRPj0FdDJMdWxvsDvUfyzo=
github.com/kralicky/codegen v0.0.0-20240522012557-5193d3fdbeca/go.mod h1:JCNwZT8FdjEIv6WsKAHsJ+GTh1/AlBrXWvxg4bOIQbU=
github.com/kralicky/go-adaptive-radix-tree v0.0.0-20240619012453-a8f80032ba31 h1:cOZUoNuv9OuCZKepddeIW87ScJWwveLfLcJMaii6YCA=
github.com/kralicky/go-adaptive-radix-tree v0.0.0-20240619012453-a8f80032ba31/go.mod h1:oJwexVSshEat0E3evyKOH6QzN8GFWrhLvEoh8GiJzss=
github.com/kralicky/gpkg v0.0.0-20240119195700-64f32830b14f h1:MsNe8A51V+7Fu5OMXSl8SK02erPJ40vFs2zDHn89w1g=
github.com/kralicky/gpkg v0.0.0-20240119195700-64f32830b14f/go.mod h1:vOkwMjs49XmP/7Xfo9ZL6eg2ei51lmtD/4U/Az5GTq8=
github.com/kralicky/protocompile v0.0.0-20240515213713-67f2e10cff9b h1:8tk3zYBk0TdyAAF6rb8CzhfG59Zr74bUD2cnIyG8izk=
github.com/kralicky/protocompile v0.0.0-20240515213713-67f2e10cff9b/go.mod h1:8zYpLPSK3LGoz1PCD4OiSLWdlrkBYNSYSv9+pxc4D9w=
github.com/kralicky/protocompile v0.0.0-20240619180838-57d9401fcbbc h1:3TJx3pjKKcYN/VARYk4tgeQQI03m23VneHLk1csdUoE=
github.com/kralicky/protocompile v0.0.0-20240619180838-57d9401fcbbc/go.mod h1:aE9D9loDNucSzYmT854laAVbgGQwGOckiSXYuwAJJSI=
github.com/kralicky/tools-lite v0.0.0-20240313161632-60bfa88304ff h1:akxm/czMYHdr1xIGm6wmddABmc4M9KDcqksdwpIJx8A=
github.com/kralicky/tools-lite v0.0.0-20240313161632-60bfa88304ff/go.mod h1:V8GGYRLr40bvX/W3nZFxG+6S3iDFWn6o5J3NGDClr8U=
github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=
Expand All @@ -64,16 +66,14 @@ github.com/olebedev/when v1.0.0 h1:T2DZCj8HxUhOVxcqaLOmzuTr+iZLtMHsZEim7mjIA2w=
github.com/olebedev/when v1.0.0/go.mod h1:T0THb4kP9D3NNqlvCwIG4GyUioTAzEhB4RNVzig/43E=
github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/plar/go-adaptive-radix-tree v1.0.5 h1:rHR89qy/6c24TBAHullFMrJsU9hGlKmPibdBGU6/gbM=
github.com/plar/go-adaptive-radix-tree v1.0.5/go.mod h1:15VOUO7R9MhJL8HOJdpydR0rvanrtRE6fA6XSa/tqWE=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prometheus/client_model v0.6.0 h1:k1v3CzpSRUTrKMppY35TLwPvxHqBu0bYgxZzqGIgaos=
github.com/prometheus/client_model v0.6.0/go.mod h1:NTQHnmxFpouOD0DpvP4XujX3CdOAGQPoaGhyTchlyt8=
github.com/prometheus/common v0.53.0 h1:U2pL9w9nmJwJDa4qqLQ3ZaePJ6ZTwt7cMD3AG3+aLCE=
github.com/prometheus/common v0.53.0/go.mod h1:BrxBKv3FWBIGXw89Mg1AeBq7FSyRzXWI3l3e7W3RN5U=
github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ=
github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8=
github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM=
github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y=
Expand Down
48 changes: 24 additions & 24 deletions pkg/format/formatter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ message Simple {
extend . google. // identifier broken up strangely should still be accepted
protobuf .
ExtensionRangeOptions {
optional string label = 20000;
optional string label = 20000;
}`[1:],
want: `
extend .google. /* identifier broken up strangely should still be accepted */protobuf.ExtensionRangeOptions {
Expand All @@ -46,16 +46,16 @@ extend .google. /* identifier broken up strangely should still be accepted */pro
2: {
input: `
message Test {
optional string foo = 1 [json_name = "|foo|"];
repeated int32 array = 2;
optional Simple s = 3;
repeated Simple r = 4;
map<string, int32> m = 5;
optional string foo = 1 [json_name = "|foo|"];
repeated int32 array = 2;
optional Simple s = 3;
repeated Simple r = 4;
map<string, int32> m = 5;
optional bytes b = 6 [default = "\0\1\2\3\4\5\6\7fubar!"];
repeated float floats = 7;
repeated bool bools = 8;
repeated Test.Nested._NestedNested.EEE enums = 9;
optional bytes b = 6 [default = "\0\1\2\3\4\5\6\7fubar!"];
repeated float floats = 7;
repeated bool bools = 8;
repeated Test.Nested._NestedNested.EEE enums = 9;
}`[1:],
want: `
message Test {
Expand All @@ -74,21 +74,21 @@ message Test {
3: {
input: `
message Another {
option (.foo.bar.rept) = {
foo: "abc" s < name: "foo", id: 123 >, array: [1, 2 ,3], r:[<name:"f">, {name:"s"}, {id:456} ],
};
option (foo.bar.rept) = {
foo: "def" s { name: "bar", id: 321 }, array: [3, 2 ,1], r:{name:"g"} r:{name:"s"}};
option (rept) = { foo: "def" };
option (eee) = V1;
option (.foo.bar.rept) = {
foo: "abc" s < name: "foo", id: 123 >, array: [1, 2 ,3], r:[<name:"f">, {name:"s"}, {id:456} ],
};
option (foo.bar.rept) = {
foo: "def" s { name: "bar", id: 321 }, array: [3, 2 ,1], r:{name:"g"} r:{name:"s"}};
option (rept) = { foo: "def" };
option (eee) = V1;
option (a) = { fff: OK };
option (a).test = { m { key: "foo" value: 100 } m { key: "bar" value: 200 }};
option (a).test.foo = "m&m";
option (a).test.s.name = "yolo";
option (a).test.s.id = 98765;
option (a).test.array = 1;
option (a).test.array = 2;
option (a).test.(.foo.bar.Test.Nested._NestedNested._garblez2) = "whoah!";
option (a).test.s.id = 98765;
option (a).test.array = 1;
option (a).test.array = 2;
option (a).test.(.foo.bar.Test.Nested._NestedNested._garblez2) = "whoah!";
}`[1:],
want: `
message Another {
Expand Down Expand Up @@ -128,7 +128,7 @@ option (.foo.bar.rept) = {r: [<name: "f">, {name: "s"}, {id: 456}]};`[1:],
},
6: {
input: `option (.foo.bar.rept) = {
foo: "abc" s < name: "foo", id: 123 >, array: [1, 2 ,3], };`,
foo: "abc" s < name: "foo", id: 123 >, array: [1, 2 ,3], };`,
want: `
option (.foo.bar.rept) = {
foo: "abc",
Expand All @@ -138,8 +138,8 @@ option (.foo.bar.rept) = {
},
7: {
input: `option (.foo.bar.rept) = {
foo: "abc" s < name: "foo", id: 123 >, array: [
1, 2 ,3], };`,
foo: "abc" s < name: "foo", id: 123 >, array: [
1, 2 ,3], };`,
want: `
option (.foo.bar.rept) = {
foo: "abc",
Expand Down
5 changes: 4 additions & 1 deletion pkg/lsp/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,13 @@ func (c *Cache) compileLocked(protos ...string) {
}
c.partialResultsMu.Unlock()

syntheticFiles := c.resolver.CheckIncompleteDescriptors(res.Files)
syntheticFiles := c.resolver.CheckIncompleteDescriptors(c.results)
if len(syntheticFiles) == 0 {
return
}
if err != nil {
slog.Debug("error checking incomplete descriptors", "err", err)
}
slog.Debug("building new synthetic sources", "sources", len(syntheticFiles))
c.compileLocked(syntheticFiles...)
}
3 changes: 3 additions & 0 deletions pkg/lsp/documents.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ func (c *Cache) DidModifyFiles(ctx context.Context, modifications []file.Modific
c.resolver.UpdateURIPathMappings(modifications)

for _, m := range modifications {
if m.Action == file.Open && m.LanguageID != "protobuf" {
continue
}
path, err := c.resolver.URIToPath(m.URI)
if err != nil {
slog.With(
Expand Down
7 changes: 0 additions & 7 deletions pkg/lsp/editions.go

This file was deleted.

12 changes: 11 additions & 1 deletion pkg/lsp/generated.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func GoIdent(desc protoreflect.Descriptor) string {
}

func tryResolvePathToGeneratedImport(files []ParsedGoFile, unresolvedPath string, whence protocompile.ImportContext) (string, error) {
// heuristic 1: if there is a unique go import path which suffix-matches `path.Dir(path)`, use it
// if there is a unique go import path which suffix-matches `path.Dir(path)`, use it
pathSuffix := path.Dir(unresolvedPath)
var candidates []string
for _, f := range files {
Expand All @@ -179,5 +179,15 @@ func tryResolvePathToGeneratedImport(files []ParsedGoFile, unresolvedPath string
return path.Join(candidates[0], path.Base(unresolvedPath)), nil
}

// I wrote the comment below late at night a while ago but can't remember
// what exactly the situation was or why i came to that conclusion, because
// this does not seem like an issue. keeping it here for posterity though

// handle files that import other files in the same package - the go code generator
// would not generate import statements for these files, since they are in the same package.
// example cloud.google.com/go/monitoring/apiv3/v2/monitoringpb/metric.proto imports
// common.proto but it's in the same package, so the generated go code does not import it.
// however, we need it to resolve some type references.

return "", fmt.Errorf("could not determine previously-generated import path for %s", unresolvedPath)
}
5 changes: 4 additions & 1 deletion pkg/lsp/golang.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ func (s *GoLanguageDriver) RefreshModules() {
}

func (s *GoLanguageDriver) HasGoModule() bool {
return s.localModDir != "" && s.localModName != ""
if s == nil {
return false
}
return s.localModDir != "" && s.localModName != "" && s.moduleResolver != nil
}

type ParsedGoFile struct {
Expand Down
6 changes: 6 additions & 0 deletions pkg/lsp/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ func (c *Cache) TryFindPackageReferences(params protocol.TextDocumentPositionPar
}

fileNode := parseRes.AST()
if fileNode == nil {
return nil
}

tokenAtOffset, comment := fileNode.ItemAtOffset(offset)
if tokenAtOffset == ast.TokenError || comment.IsValid() {
Expand Down Expand Up @@ -150,6 +153,9 @@ func (c *Cache) tryHoverPackageNode(params protocol.TextDocumentPositionParams)
}

fileNode := parseRes.AST()
if fileNode == nil {
return nil
}

tokenAtOffset, comment := fileNode.ItemAtOffset(offset)
if tokenAtOffset == ast.TokenError || comment.IsValid() {
Expand Down
30 changes: 28 additions & 2 deletions pkg/lsp/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/kralicky/tools-lite/gopls/pkg/cache"
"github.com/kralicky/tools-lite/gopls/pkg/file"
"github.com/kralicky/tools-lite/gopls/pkg/protocol"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protodesc"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/reflect/protoregistry"
Expand Down Expand Up @@ -217,9 +218,27 @@ func (r *Resolver) CheckIncompleteDescriptors(results linker.Files) []string {
if res == nil {
continue
}
resolver := linker.ResolverFromFile(res)
fdp := res.(linker.Result).FileDescriptorProto()
data, err := proto.Marshal(fdp)
if err != nil {
slog.With(
"uri", string(uri),
"error", err,
).Error("failed to generate synthetic file descriptor")
continue
}
if err := (proto.UnmarshalOptions{Resolver: resolver}).Unmarshal(data, fdp); err != nil {
slog.With(
"uri", string(uri),
"error", err,
).Error("failed to generate synthetic file descriptor")
continue
}

newFile, err := protodesc.FileOptions{
AllowUnresolvable: true,
}.New(protodesc.ToFileDescriptorProto(res), linker.ResolverFromFile(res))
}.New(fdp, resolver)
if err != nil {
slog.With(
"uri", string(uri),
Expand Down Expand Up @@ -320,7 +339,7 @@ func (r *Resolver) findFileByPathLocked(path string, whence protocompile.ImportC
if result, err := r.checkGoModule(path, whence); err == nil {
lg.Debug("resolved to go module")
return result, nil
} else if !errors.Is(err, os.ErrNotExist) {
} else if !errors.Is(err, os.ErrNotExist) && !errors.Is(err, ErrNoModule) {
lg.Debug("failed to check go module")
return protocompile.SearchResult{}, err
}
Expand Down Expand Up @@ -375,6 +394,9 @@ func (r *Resolver) checkFS(path string, whence protocompile.ImportContext) (prot
}

func (r *Resolver) checkGoModule(path string, whence protocompile.ImportContext) (protocompile.SearchResult, error) {
if !r.goLanguageDriver.HasGoModule() {
return protocompile.SearchResult{}, ErrNoModule
}
if strings.HasPrefix(path, "github.com/gogo/googleapis/") {
// these are vendored in the gogo/protobuf repo, so we need to special case them
// to avoid conflicting symbols
Expand Down Expand Up @@ -613,6 +635,10 @@ func FastLookupGoModule(f io.Reader) (string, error) {
}

func (r *Resolver) tryReverseLookupLocked(path string, whence protocompile.ImportContext) (string, error) {
if !r.goLanguageDriver.HasGoModule() {
return "", ErrNoModule
}

fd := whence.FileDescriptorProto()
uri, ok := r.fileURIsByPath[fd.GetName()]
if !ok {
Expand Down
5 changes: 3 additions & 2 deletions sdk/codegen/generators/x/python/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ go 1.21
require (
github.com/flosch/pongo2/v6 v6.0.0
github.com/iancoleman/strcase v0.3.0
google.golang.org/protobuf v1.32.0
google.golang.org/protobuf v1.34.2
)

require (
github.com/google/go-cmp v0.6.0 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/kr/pretty v0.3.1 // indirect
github.com/rogpeppe/go-internal v1.12.0 // indirect
)
6 changes: 3 additions & 3 deletions sdk/codegen/generators/x/python/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ github.com/flosch/pongo2/v6 v6.0.0/go.mod h1:CuDpFm47R0uGGE7z13/tTlt1Y6zdxvr2RLT
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/iancoleman/strcase v0.3.0 h1:nTXanmYxhfFAMjZL34Ov6gkzEsSJZ5DbhxWjvSASxEI=
github.com/iancoleman/strcase v0.3.0/go.mod h1:iwCmte+B7n89clKwxIoIXy/HfoL7AsD47ZCWhYzw7ho=
github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI=
github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
google.golang.org/protobuf v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8=
google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg=
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
5 changes: 5 additions & 0 deletions test/marker/marker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"go/types"
"io/fs"
"log"
"log/slog"
"os"
"path"
"path/filepath"
Expand Down Expand Up @@ -90,6 +91,10 @@ func Test(t *testing.T) {
t.Skip("golang/go#64473: skipping with -short: this test is too slow on darwin and solaris builders")
}
}
slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{
AddSource: true,
Level: slog.LevelDebug,
})))
// The marker tests must be able to run go/packages.Load.
testenv.NeedsGoPackages(t)

Expand Down
4 changes: 2 additions & 2 deletions test/marker/testdata/hover/refs.txt
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ package bar;
import "foo.proto";

message BarMsg { //@loc(defBarMsg, "BarMsg"), refs("BarMsg", defBarMsg, refBarMsgFieldType)
option (foo.fooEnumMsgOpt) = FooEnum0; //@loc(refFooEnum0OptVal, "FooEnum0"), loc(refQualifiedFooEnumMsgOpt, "(foo.fooEnumMsgOpt)")
option (foo.fooMsgOpt).fooEnum = FooEnum1; //@loc(refFooEnum1FieldRefVal, "FooEnum1"), loc(refQualifiedFooMsgOpt, "(foo.fooMsgOpt)"), loc(refFooEnumOptFieldRef, "fooEnum")
option (foo.fooEnumMsgOpt) = FooEnum0; //@loc(refFooEnum0OptVal, "FooEnum0"), loc(refQualifiedFooEnumMsgOpt, "foo.fooEnumMsgOpt")
option (foo.fooMsgOpt).fooEnum = FooEnum1; //@loc(refFooEnum1FieldRefVal, "FooEnum1"), loc(refQualifiedFooMsgOpt, "foo.fooMsgOpt"), loc(refFooEnumOptFieldRef, "fooEnum")

foo.FooMsg fooMsg = 1 [ //@loc(refFooMsgQualifiedFieldType, "foo.FooMsg"), loc(defBarMsgFooMsgField, "fooMsg"), refs("fooMsg", defBarMsgFooMsgField)
(foo.fooEnumFieldOpt) = FooEnum0 //@loc(refFooEnum0FieldLitVal, "FooEnum0")
Expand Down

0 comments on commit 0007177

Please sign in to comment.