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

prost-build 0.8.0 only adds final type_attribute #502

Closed
tommilligan opened this issue Jul 9, 2021 · 8 comments
Closed

prost-build 0.8.0 only adds final type_attribute #502

tommilligan opened this issue Jul 9, 2021 · 8 comments

Comments

@tommilligan
Copy link

In prost-build 0.7.0, it was possible to add any number of type attributes, and all would be added to the generated struct. In the following minimal example:

// src/example.proto
syntax = "proto3";
package example;

message Example {
  string example_field = 1;
}
// build.rs
use prost_build::Config;

fn main() {
    let mut config = Config::new();
    config.type_attribute(".", "#[derive(Display)]");
    config.type_attribute(".", "#[derive(PartialOrd)]");
    config
        .compile_protos(&["./src/example.proto"], &["./src"])
        .unwrap();
}

Generates the following on prost-build 0.7.0:

#[derive(Display)]
#[derive(PartialOrd)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Example {
    #[prost(string, tag="1")]
    pub example_field: ::prost::alloc::string::String,
}

Note that all configured type_attributes are added in the order they are configured. However, with prost-build 0.8.0, the generated output is:

#[derive(PartialOrd)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Example {
    #[prost(string, tag="1")]
    pub example_field: ::prost::alloc::string::String,
}

Note that only the most recent type_attribute, PartialOrd, has been added.

This appears to be an unintentional breaking change, I expect introduced here: 79f0dfd#diff-5283c009c22e833ce65e3b8a6d7f57c026e0562f79225a9375f8e738ad4080c2R252-R260

I'll investigate further and see if I can submit a fix today.

@tommilligan
Copy link
Author

Added a minimal example that fails to compile with cargo build here: https://github.com/tommilligan/prost-502-reproduction

@tommilligan
Copy link
Author

tommilligan commented Jul 9, 2021

After investigation, there appear two be two distinct changes introduced by the PathMap refactoring in 79f0dfd. They are:

  • calls to type_attribute and field_attribute are no longer cumulative. Calling again with the same pattern key will overwrite any previously inserted attributes. This is in contradiction to the docs which state:

The calls to this method are cumulative. They don’t overwrite previous calls and if a type is matched by multiple calls of the method, all relevant attributes are added to it.

  • only attributes from one of the matched patterns are applied. For instance with the following config and the example.proto file shown above:
use prost_build::Config;

fn main() {
    let mut config = Config::new();
    config.type_attribute(".", "#[derive(PartialOrd)]");
    config.type_attribute("Example", "#[derive(Ord)]");
    config
        .compile_protos(&["./src/example.proto"], &["./src"])
        .unwrap();
}

The following rust code is generated:

#[derive(Ord)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Example {
    #[prost(string, tag="1")]
    pub example_field: ::prost::alloc::string::String,
}

Note that only the attribute matched by the Example key is applied, even though the . should still match and be applied to all messages. Again this doesn't match what the docs indicate.

thanethomson added a commit to informalsystems/tendermint-rs that referenced this issue Jul 13, 2021
This temporarily reverts commit
1efe42c. We'll reintroduce these
changes when tokio-rs/prost#502 is fixed.

Signed-off-by: Thane Thomson <[email protected]>
thanethomson added a commit to informalsystems/tendermint-rs that referenced this issue Jul 13, 2021
This temporarily reverts commit
1efe42c. We'll reintroduce these
changes when tokio-rs/prost#502 is fixed.

Signed-off-by: Thane Thomson <[email protected]>
daviddrysdale added a commit to daviddrysdale/tink-rust that referenced this issue Jul 17, 2021
prost_build::Config::type_attribute() doesn't appear to support
repeated attributes any more, so fold together.
(tokio-rs/prost#502)

Also upgrade tonic* to 0.5
daviddrysdale added a commit to project-oak/tink-rust that referenced this issue Jul 19, 2021
prost_build::Config::type_attribute() doesn't appear to support
repeated attributes any more, so fold together.
(tokio-rs/prost#502)

Also upgrade tonic* to 0.5
@Veetaha
Copy link
Contributor

Veetaha commented Jul 25, 2021

@danburkert, this seems like a serious regression that blocks us from updating to the new version and fixing the RUSTSEC advisory...

@kalzoo
Copy link

kalzoo commented Jul 26, 2021

^ sorry to +1 this, but same - can't use 0.8 with this issue (and #507), which means no security patch, no latest tonic, etc. of course, it's open-source and free, and I appreciate all you've made! just a friendly heads up.

@horacimacias
Copy link
Contributor

just in case this helps anybody else, in my case I was able to use a single attribute with line break to achieve two attributes

.type_attribute(".", "// first attribute\n// second attribute")

not ideal and likely not covering all scenarios but at least does the trick in certain cases.

@ryankurte
Copy link

if you need a temporary workaround (probably wouldn't ship it...) it seems you can also revert just the commit (7940dfd) introducing the regression (i got part way through combining type attributes but, we have a lot). something like this.

@cab
Copy link
Contributor

cab commented Aug 13, 2021

I think this is either a duplicate of or related to #490. If so, I think this issue is more comprehensive, so it might be worth folding into this one.

subashch6 added a commit to subashch6/prost that referenced this issue Aug 20, 2021
Guiguiprim pushed a commit to Guiguiprim/prost that referenced this issue Aug 27, 2021
Guiguiprim pushed a commit to Guiguiprim/prost that referenced this issue Aug 30, 2021
Guiguiprim pushed a commit to Guiguiprim/prost that referenced this issue Sep 7, 2021
Guiguiprim pushed a commit to Guiguiprim/prost that referenced this issue Sep 8, 2021
LucioFranco added a commit that referenced this issue Sep 15, 2021
This address #490 #502 #507

Co-authored-by: Guilhem Vallat <[email protected]>
Co-authored-by: Lucio Franco <[email protected]>
@LucioFranco
Copy link
Member

Fixed in #522

ajguerrer pushed a commit to ajguerrer/prost that referenced this issue Sep 17, 2021
This address tokio-rs#490 tokio-rs#502 tokio-rs#507

Co-authored-by: Guilhem Vallat <[email protected]>
Co-authored-by: Lucio Franco <[email protected]>
lklimek pushed a commit to lklimek/tenderdash-abci-rs that referenced this issue Feb 28, 2023
This temporarily reverts commit
1efe42c8625045fd99072718faf96e81aeb9c6e6. We'll reintroduce these
changes when tokio-rs/prost#502 is fixed.

Signed-off-by: Thane Thomson <[email protected]>
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 a pull request may close this issue.

7 participants