Skip to content

Convert tarpc::service! function macro to #[tarpc::service] attribute macro #247

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

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

tikue
Copy link
Collaborator

@tikue tikue commented Jul 23, 2019

Fixes #244

I am leaning toward making this change. I think, overall, it's a net positive. The proc macro is also much easier to read than the macro_rules code (not that that couldn't have been changed without making it an attribute).

@tikue tikue added the feature label Jul 23, 2019
@tikue tikue requested a review from vorot93 July 23, 2019 05:04
@tikue tikue self-assigned this Jul 23, 2019
@@ -11,3 +11,4 @@ os:
script:
- cargo test --all-targets --all-features
- cargo test --doc --all-features
- cargo run --example 2>&1 | grep ' ' | awk '{print $1}' | xargs -L 1 cargo run --all-features --example
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I keep forgetting to update examples before pushing code. This will make that harder. I could make this change outside this pr if you'd like.

@@ -11,6 +12,9 @@ categories = ["asynchronous", "network-programming"]
readme = "../README.md"
description = "Proc macros for tarpc."

[features]
serde1 = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be enabled by default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to get this to work. Unfortunately, it presents a footgun for users who don't need serde but don't opt out of the default features. They'll get the following errors if they don't have serde in their Cargo.toml dependencies:

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:11:1
   |
11 | #[tarpc::service]
   | ^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:11:1
   |
11 | #[tarpc::service]
   | ^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:11:1
   |
11 | #[tarpc::service]
   | ^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:11:1
   |
11 | #[tarpc::service]
   | ^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:14:20
   |
14 |     async fn hello(name: String) -> String;
   |                    ^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
  --> example-service/src/lib.rs:14:37
   |
14 |     async fn hello(name: String) -> String;
   |                                     ^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/27812
   = help: add `#![feature(rustc_private)]` to the crate attributes to enable

error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0658`.
error: Could not compile `tarpc-example-service`.

This comment has more context.

@@ -13,25 +13,43 @@ readme = "../README.md"
description = "An RPC framework for Rust with a focus on ease of use."

[features]
serde1 = ["rpc/serde1", "serde", "serde/derive"]
serde1 = ["rpc/serde1", "tarpc-plugins/serde1", "serde", "serde/derive"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be enabled by default

@vorot93
Copy link
Contributor

vorot93 commented Jul 29, 2019

One thing to note is that serde integration should be always enabled by default. This is what users expect and it is extremely confusing when they get compilation errors having followed the API to a letter.

@tikue
Copy link
Collaborator Author

tikue commented Jul 29, 2019

Let me experiment with making serde default enabled. One issue is that the feature currently requires users to depend on the serde crate with the derive feature enabled. While it's not a great experience right now if they want to use serde and it's not enabled, it's arguably worse if they don't care about serde at all but get a compile error because they are missing a crate they aren't even going to use.

@tikue
Copy link
Collaborator Author

tikue commented Jul 29, 2019

If I can make it work by reexporting serde from tarpc, then I'm in favor of that.

@tikue tikue force-pushed the service-attribute branch from da59872 to c3b5a10 Compare July 30, 2019 04:12
@googlebot
Copy link
Collaborator

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tikue tikue force-pushed the service-attribute branch from c3b5a10 to 605f2dd Compare July 30, 2019 04:14
@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@tikue tikue force-pushed the service-attribute branch from 605f2dd to 188e89b Compare July 30, 2019 04:38
@tikue tikue merged commit abb0b5b into google:master Jul 30, 2019
@tikue tikue deleted the service-attribute branch August 9, 2019 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider switching service macro to be attribute based
3 participants