search

0xPolygonMiden / miden-node

Reference implementation of the node for the Polygon Miden rollup
MIT License
52 stars 37 forks source link

fix: type shadowing in proto file #481

Closed tomyrd closed 4 weeks ago

tomyrd commented 4 weeks ago

When updating the dependencies in the client we started getting this error when generating the rust types from proto files:

Error:   × name 'account.AccountInfo' is shadowed
       ╭─[responses.proto:164:5]
   163 │     // Account info (with details for on-chain accounts)
   164 │     account.AccountInfo account = 1;
       ·     ─────────┬─────────
       ·              ╰── found here
   165 │ }
       ╰────
    help: 'account.AccountInfo' is is resolved to
          'responses.GetAccountDetailsResponse.account.AccountInfo',
          which is not defined. The innermost scope is searched first
          in name resolution. Consider using a leading '.'(i.e.,
          '.account.AccountInfo') to start from the outermost scope.

It seems the name of the field account is shadowing the name of the import account. As it is recommended in the message, this can be fixed by specifying the root.

tomyrd commented 4 weeks ago

The account name is used in two places in this context:

  1. For the name of the first field GetAccountDetailsResponse
  2. For the name of the module that is imported that contains the type AccountInfo

From what I understood, the first name is overwriting (also called shadowing) the second name in this case, so the type AccountInfo is being searched inside the field when it should be searched inside the module. By specifying the root with the '.' we are removing this ambiguity and the type will be searched in .account.AccountInfo.

This name collision doesn't happen in the other messages and that's why it's only needed here.

bobbinth commented 4 weeks ago

This name collision doesn't happen in the other messages and that's why it's only needed here.

Ah! Thank you! So, if we renamed the field to something like details, it would solve the problem as well, right? (not that we should do it, but just wanted to confirm)

tomyrd commented 4 weeks ago

Yes, that would also work but the name should also be changed in the code in that case. I just looked into it and it's not that much different, maybe we prefer the name change instead of the explicit root.

bobbinth commented 4 weeks ago

I just looked into it and it's not that much different, maybe we prefer the name change instead of the explicit root.

I'm fine either way.

tomyrd commented 4 weeks ago

At the end I decided to change the name and keep the type definition consistent.