Support custom derives on Request and Response

[ShaneMurphy2]

Replace the bespoke "derive_serde" with a more flexible "derive = [, , ...]" form.

Deprecate the old "derive_serde" form, and emit deprecation warnings.

Add trybuild tests in a separate test crate to ensure the deprecation, features, and error messages all behave as expected. This technique of a separate trybuild test crate is borrowed from tokio/tests-build. Update CI to run this new crate's tests under the serde matrix.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ShaneMurphy2]

Not sure if I did the CI modification correctly, please take a look.

[ShaneMurphy2]

Addresses #436

[ShaneMurphy2]

Looks like one thing I still need to iron out (besides the CLA 😬) is the situation where serde1 is enabled but the user explicitly writes #[service(derive = [Serialize, Deserialize])]. That currently causes a conflicting implementation error. I guess I would need to scan through the paths in the meta list and look for Serialize or Deserialize. Of course, that wouldn't fix someone importing Serialize under a different name.

[tikue]

the situation where serde1 is enabled but the user explicitly writes #[service(derive = [Serialize, Deserialize])]

Oh, this is what I meant to address by my 4th bullet of #436 (comment): only implicitly derive serialize when derive = [...] is not present.

[ShaneMurphy2]

the situation where serde1 is enabled but the user explicitly writes #[service(derive = [Serialize, Deserialize])]

Oh, this is what I meant to address by my 4th bullet of #436 (comment): only implicitly derive serialize when derive = [...] is not present.

Gotcha, will work on it.

[ShaneMurphy2]

Should be good now, I think I incorrectly updated the CI file though. Either way, do we wanna bikeshed the name a bit? Is derive clear? I should also update the documentation for the macro.

[ShaneMurphy2]

Cleaned up the implementation a bit.

[ShaneMurphy2]

Added some docs and should fix CI 🤞

[tikue]

Instead of adding this package, could the tests go in the existing tarpc/tests/compile_fail?

[ShaneMurphy2]

Ah, assumed trybuild tests would be in the plugins crate, my bad. Yes, I believe they can go there.

[ShaneMurphy2]

Done!

[tikue]

this isn't needed now, right?

[ShaneMurphy2]

Yup, will remove.

[tikue]

Still seeing some test failures:

[ShaneMurphy2]

Still seeing some test failures:

Hm, tests passed locally, and those outputs look the same except for the number of candidate traits. Interesting.

[tikue]

Still seeing some test failures:

Hm, tests passed locally, and those outputs look the same except for the number of candidate traits. Interesting.

sometimes I've seen these changes based on whether the tests were run with nightly vs stable rustc?

[ShaneMurphy2]

Still seeing some test failures:

Hm, tests passed locally, and those outputs look the same except for the number of candidate traits. Interesting.

sometimes I've seen these changes based on whether the tests were run with nightly vs stable rustc?

I think it's that the test matrix in CI is enabling features that bring in dependencies that provide a serialize method under a trait. Because of that, the compiler is suggesting more items, causing the output to be different. I could re-generate the stderr files with all feature enabled, but I'm not sure if that would cause CI to fail in a different spot when it toggles the feature off. I think I'll try refactoring the test to use fully qualified syntax to the serialize method so that the error message is not ambiguous.

[ShaneMurphy2]

That should fix it, I'm a little worried about how brittle these tests are though. Let me know if you think they're more trouble than they're worth.

[tikue]

Oh looks like the CLA hasn't been signed, either? Is that something you can take a look at?

[ShaneMurphy2]

Oh looks like the CLA hasn't been signed, either? Is that something you can take a look at?

Yeah, legal is looking at it. Should just be due diligence and not a problem.

[ShaneMurphy2]

That should fix it, I'm a little worried about how brittle these tests are though. Let me know if you think they're more trouble than they're worth.

Forgot to overwrite the .stderr files 🤦

[ShaneMurphy2]

Green!

[ShaneMurphy2]

Re: CLA, the legal team is either at a conference or out sick, so it might be a few more days.

[ShaneMurphy2]

Re: CLA, the legal team is either at a conference or out sick, so it might be a few more days.

Approved, just waiting for wheels to turn.

[ShaneMurphy2]

Should be good to re-run the pipeline!

[tikue]

Thanks a lot! This is a great generalization.