Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: type shadowing in proto file #481

Merged
merged 2 commits into from
Sep 3, 2024
Merged

fix: type shadowing in proto file #481

merged 2 commits into from
Sep 3, 2024

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Sep 3, 2024

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.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good!

I am not sure I understand why this error is happening though. What is shadowing "account.AccountInfo"? As far as I can tell, there is nothing special about this reference as compared to other references in the same file.

@tomyrd
Copy link
Collaborator Author

tomyrd commented Sep 3, 2024

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
Copy link
Contributor

bobbinth commented Sep 3, 2024

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
Copy link
Collaborator Author

tomyrd commented Sep 3, 2024

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
Copy link
Contributor

bobbinth commented Sep 3, 2024

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 tomyrd force-pushed the tomyrd-fix-shadowing branch from 5bac7a8 to ed3ba1b Compare September 3, 2024 19:53
@tomyrd
Copy link
Collaborator Author

tomyrd commented Sep 3, 2024

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

@tomyrd tomyrd force-pushed the tomyrd-fix-shadowing branch from ed3ba1b to 571830d Compare September 3, 2024 19:59
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! Could you also update the changelog since now this is a breaking change?

@tomyrd tomyrd merged commit 424a18c into next Sep 3, 2024
9 checks passed
@tomyrd tomyrd deleted the tomyrd-fix-shadowing branch September 3, 2024 20:40
SantiagoPittella pushed a commit that referenced this pull request Sep 6, 2024
* fix: change field name in `GetAccountDetailsResponse`

* update: CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants