missing `{}` after printing option

[japersik]

Hi,
I'm facing a problem that I don't know how to fix..
when parsing and printing the same file containing option, the file becomes invalid

A simple example:

syntax = "proto2";
package pkg;
option go_package = "some.pkg";
import "google/protobuf/descriptor.proto";
message Options {
    optional bool some_option_value = 1;
}
extend google.protobuf.MessageOptions {
    optional Options my_some_option = 11964;
}
message SomeMessage {
    option (.pkg.my_some_option) = {some_option_value : true};
}

I'm using ParseFilesButDoNotLink(...) because the extension may be unknown

after parsing and printing this file, I got this

syntax = "proto2";

package pkg;

import "google/protobuf/descriptor.proto";

option go_package = "some.pkg";

message Options {
  optional bool some_option_value = 1;
}

message SomeMessage {
  option (.pkg.my_some_option) = some_option_value : true;
}

extend .google.protobuf.MessageOptions {
  optional .pkg.Options my_some_option = 11964;
}

option option (.pkg.my_some_option) = some_option_value : true; is invalid cause curly braces are missing

I tried to use bufbuild compiller instead of protoparse, but I got the same result

[japersik]

code exapmle

package main

import (
	"bytes"
	_ "embed"
	"fmt"
	"io"
	"os"
	"strings"

	"github.com/jhump/protoreflect/desc"
	"github.com/jhump/protoreflect/desc/protoparse"
	"github.com/jhump/protoreflect/desc/protoprint"
	"google.golang.org/protobuf/reflect/protodesc"
	"google.golang.org/protobuf/reflect/protoreflect"
	"google.golang.org/protobuf/types/descriptorpb"
)

//go:embed example.proto
var textProtoFile string
var parsedFileName = "example.proto"

func main() {
	//create builtins map
	var builtins = map[string]protoreflect.FileDescriptor{
		"google/protobuf/descriptor.proto": descriptorpb.File_google_protobuf_descriptor_proto,
	}
	var builtInDeps = make(map[string]string)
	{
		var fds []*descriptorpb.FileDescriptorProto
		for _, value := range builtins {
			fd := protodesc.ToFileDescriptorProto(value)
			fds = append(fds, fd)
		}
		fdMap, err := desc.CreateFileDescriptors(fds)
		if err != nil {
			panic(err)
		}
		printer := protoprint.Printer{OmitComments: protoprint.CommentsAll}
		for key, value := range fdMap {
			var writer strings.Builder
			err = printer.PrintProtoFile(value, &writer)
			if err != nil {
				panic(err)
			}
			builtInDeps[key] = writer.String()
		}
	}
// parse
	var p = protoparse.Parser{
		ValidateUnlinkedFiles: true,
		Accessor: func(filename string) (io.ReadCloser, error) {
			var contents string
			if filename == parsedFileName {
				contents = textProtoFile
			}
			if contents == "" {
				contents = builtInDeps[filename]
			}
			if contents == "" {
				return nil, os.ErrNotExist
			}
			return io.NopCloser(strings.NewReader(contents)), nil
		},
	}
	parsed, err := p.ParseFilesButDoNotLink(parsedFileName, "google/protobuf/descriptor.proto")
	if err != nil {
		panic(err)
	}
	var writer bytes.Buffer
	var printer = protoprint.Printer{
		SortElements:             true,
		OmitComments:             protoprint.CommentsAll,
		ForceFullyQualifiedNames: true,
	}
	files, err := protodesc.NewFiles(&descriptorpb.FileDescriptorSet{File: parsed})
	if err != nil {
		panic(err)
	}
	descriptorFile, err := files.FindFileByPath(parsedFileName)
	if err != nil {
		panic(err)
	}
	descriptorWrapped, err := desc.WrapFile(descriptorFile)
	if err != nil {
		panic(err)
	}
// print
	err = printer.PrintProtoFile(descriptorWrapped, &writer)
	if err != nil {
		panic(err)
	}
	fmt.Println(writer.String())
}

[jhump]

@japersik, nice job finding all of the bugs in this package! This is indeed a tiny bug related to uninterpreted options, which is what you usually get when you're not linking (because the link step is needed for it to interpret most options, including all custom options).

Thanks for the repro case! I will use that to make a new test and can open a PR with the fix soon.

[jhump]

I'm using ParseFilesButDoNotLink(...) because the extension may be unknown

I don't quite follow this. Do you mean the source might be importing a file that is unknown? That is the only way for the file to be valid source and the extension to be unknown. And in that case (if you don't have access to the imports), I don't see how you could create a desc.Descriptor to pass to the protoprint package.

In any event, I've opened a PR with the fix, since it was intended that this could work with uninterpreted options -- there just wasn't adequate test coverage to verify it was right. In practice though, you should really be using ParseFiles. If you had problems getting that to work, open another issue and I may be able to help.

[japersik]

Thanks
I probably wrote something wrong (I've been working with protobuf not so long ago and might not understand some things). But I found proto2 files that use extended options that are defined in another file, but are not imported. In this case, I can't use ParseFiles because I get an error message like that: field Full.Path.To.field: unknown extension .Full.Path.To.extension

[jhump]

But I found proto2 files that use extended options that are defined in another file, but are not imported.

Hmm, that's quite suspicious. If they use custom options that are not imported then they are not valid source files. No other compiler (particularly the official protoc compiler) will accept that. Is it possible that the extensions used to be defined in one of the files it imports, but something changed in the dependencies? Likely those sources should be fixed to import the custom option definitions.