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

cleanup std usages / procedural macro hygiene #417

Merged
merged 5 commits into from Jan 21, 2024
Merged

Conversation

OMGeeky
Copy link
Contributor

@OMGeeky OMGeeky commented Jan 13, 2024

Directly using things like Result and Option without namespace inside the generated code without using the full namespace can lead to problems when the caller has renamed those usages and provided his own with different signatures (often done with the Result type).

See the Rust Reference on this.

One example that would create compiler errors without this fix:

use std::result::Result as OldResult;
type Result<T> = OldResult<T,u16>;
use std::option::Option as OldOption;
type Option = OldOption<u16>;

#[tarpc::service]
pub trait World {
    async fn hello(name: String) -> String;
}

@tikue
Copy link
Collaborator

tikue commented Jan 18, 2024

Thank you!

@OMGeeky
Copy link
Contributor Author

OMGeeky commented Jan 19, 2024

I Added some more and I think I found everything that I missed in the previous one.

@OMGeeky
Copy link
Contributor Author

OMGeeky commented Jan 19, 2024

I did find some more, like the unreachable macro. I found an easy way to check for this and will create a test that checks for any external usages.

@OMGeeky
Copy link
Contributor Author

OMGeeky commented Jan 19, 2024

One thing I also changed was all the usages of tarpc to ::tarpc because if you rename your import you can still access it with those two :: in front of the crate name, by the name it was under originally.

@rtbo rtbo mentioned this pull request Jan 21, 2024
@tikue tikue merged commit 40d2d16 into google:master Jan 21, 2024
5 checks passed
@tikue
Copy link
Collaborator

tikue commented Jan 21, 2024

Thanks so much for fixing this!

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.

None yet

2 participants