reznikmm / protobuf

The Google Protocol Buffers implementation in Ada
MIT License
32 stars 5 forks source link

"Namespace" of message is not taken into account resulting in incorrect Ada code #10

Closed mgrojo closed 2 years ago

mgrojo commented 2 years ago

The following valid proto3 file generates an incorrect Ada file, due to losing the "namespace" opened by "message".

syntax = "proto3";

package Test_Package;

message Parent {
  message T  {
     string value = 1;
  }
}

message T {
  int32 value = 1;
}

The generated Ada file has errors:

test_package-test.ads:16:09: error: invalid completion of private type "T_Vector" defined at line 12
test_package-test.ads:17:11: error: Indexing aspect requires a function that applies to type "T_Vector"
test_package-test.ads:18:06: error: Indexing aspect requires a function that applies to type "T_Vector"
test_package-test.ads:150:14: error: "Read_T" conflicts with declaration at line 96
test_package-test.ads:154:14: error: "Write_T" conflicts with declaration at line 100
test_package-test.ads:158:08: error: "Read" attribute already defined at line 104
test_package-test.ads:162:09: error: "T_Array" conflicts with declaration at line 108
test_package-test.ads:164:09: error: "T_Array_Access" conflicts with declaration at line 110
test_package-test.ads:166:09: error: "T_Vector" conflicts with declaration at line 112
reznikmm commented 2 years ago

Hello!

What source code organization do you propose for this proto3?

mgrojo commented 2 years ago

Hi!

In my proto files the direct equivalence would be a nested package named Parent, nesting is only used in that sense, as a namespace for some messages. But I know proto3 files could also be defined in this way:

message SearchResponse {
  message Result {
    string url = 1;
    string title = 2;
    repeated string snippets = 3;
  }
  repeated Result results = 1;
}

In that case, a nested package might not be enough. I don't have any of this pattern, though.

mgrojo commented 2 years ago

I was thinking about that last example. One code organization that would preserve the namespaces for proto and still support those nested messages with fields in the parent could be:

package Filename is

   package SearchResponse is
      type Result is ...;
      type Message is ...;
   end Search_Response;

end Filename;

Since message is a keyword in protobuf, there's no risk of name collision to anything defined in the proto file.

In fact, that pattern could be extended to any message in the proto file (not only to nested messages with fields in parent), that is, all messages could generate a package with the name of the message, and a type with message as name (plus operations) but that it's a too radical change, I guess. I mean:

package Filename is

   package SearchResponse is
      package Result is
         type Message is ...;
      end Result;
      type Message is ...;
   end Search_Response;

end Filename;
reznikmm commented 2 years ago

This should work now.

mgrojo commented 2 years ago

Thanks for all the corrections, @reznikmm. With this and the pull request that I've made, all my compilation issues with the generated code are fixed. The alternate implementation has also worked for me up to the compilation phase (haven't gone further yet). By the way, I've forwarded your patronage suggestion to the interested party.