issuu / ocaml-protoc-plugin

ocaml-protoc-plugin
https://issuu.github.io/ocaml-protoc-plugin/
Other
48 stars 19 forks source link

Fix bug in name lookup leading to unusable code generated. #53

Closed andersfugmann closed 8 months ago

andersfugmann commented 8 months ago

The current lookup function (Scope.get_scoped_name) did not account for Ocaml's name resolution when constructing a name. This lead to:

  1. A bug in name resolution even if a possible solution exists
  2. Failed to recognize if no solution exists. (Resulting in compilation errors of generated ml files=

Example:

package X;
message R {
  message A {}
  message R {
    message A {}
    .X.R.A a1 = 1;  
    .X.R.R.A a2 = 2; 
  }
}

In the above case, a1 would resolve to the wrong A, and a2 cannot be resolved if the package decl is removed.

The new solution follows the Ocaml's name resolution to build the shortest symbol reference, and will raise an error if no solution can be found.

The PR also changes package module names to be recursive modules to allow referencing top level from within sub-modules.

It could potentially be possible to inject a new unique module name if the current scoping does no contain usable unique names, but that's for another PR.

andersfugmann commented 8 months ago

The PR is split in three commits.

I don't think a deep review is needed as the code is reasonably tested, but I do appreciate comments on coding style and other comments.