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.

Sign up for GitHub

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

better checking of tag duplicates, avoid discarding invalid variant errs #951

Merged
merged 11 commits into from
May 24, 2024
Merged

better checking of tag duplicates, avoid discarding invalid variant errs #951

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dbd5111
better checking of tag duplicates, avoid discarding invalid variant e…
mumbleskates Nov 25, 2023
4c00d4e
add some simple derive tests :)
mumbleskates Nov 25, 2023
12e65f3
use itertools duplicates()
mumbleskates May 22, 2024
0d0a1dd
don't print out unreadable syn junk when encountering unknown attributes
mumbleskates May 22, 2024
0b3216a
clarify and test backstop for the "multiple tags in a oneof variant" …
mumbleskates May 22, 2024
6cfd80f
use expect_err
mumbleskates May 22, 2024
02bc132
nicer framing around the unknown attribute tokens
mumbleskates May 22, 2024
be6f140
express higher minimal versions of itertools and proc-macro2 for carg…
mumbleskates May 23, 2024
2545542
simplify bounds for proc-macro2
mumbleskates May 24, 2024
aa45a14
update the other instance of the .tuple_windows() trick to use .dupli…
mumbleskates May 24, 2024
af24da1
clarify & shorten assertion
mumbleskates May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 80 additions & 38 deletions prost-derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ extern crate proc_macro;

use anyhow::{bail, Error};
use itertools::Itertools;
use proc_macro::TokenStream;
use proc_macro2::Span;
use proc_macro2::{Span, TokenStream};
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
use quote::quote;
use syn::{
punctuated::Punctuated, Data, DataEnum, DataStruct, DeriveInput, Expr, Fields, FieldsNamed,
Expand All @@ -19,7 +18,7 @@ mod field;
use crate::field::Field;

fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
let input: DeriveInput = syn::parse(input)?;
let input: DeriveInput = syn::parse2(input)?;

let ident = input.ident;

Expand Down Expand Up @@ -91,16 +90,19 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
fields.sort_by_key(|(_, field)| field.tags().into_iter().min().unwrap());
let fields = fields;

let mut tags = fields
if let Some((duplicate_tag, _)) = fields
.iter()
.flat_map(|(_, field)| field.tags())
.collect::<Vec<_>>();
let num_tags = tags.len();
tags.sort_unstable();
tags.dedup();
if tags.len() != num_tags {
bail!("message {} has fields with duplicate tags", ident);
}
.sorted_unstable()
.tuple_windows()
.find(|(a, b)| a == b)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would kinda be preferable to write this as .duplicates().next() using Itertools, but that method unfortunately requires use_std.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I understand prost_derive will run on the host, so it is not subject to no_std rules. The code does also use std::fmt;, so std must be available. So I think it is safe to use_std for Itertools. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah from what i've seen since i published this that is totally possible. i'll update it

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you updated the other case, but not this one. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was not.

{
bail!(
"message {} has multiple fields with tag {}",
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
ident,
duplicate_tag
)
};

let encoded_len = fields
.iter()
Expand Down Expand Up @@ -251,16 +253,16 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
#methods
};

Ok(expanded.into())
Ok(expanded)
}

#[proc_macro_derive(Message, attributes(prost))]
pub fn message(input: TokenStream) -> TokenStream {
try_message(input).unwrap()
pub fn message(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
try_message(input.into()).unwrap().into()
}

fn try_enumeration(input: TokenStream) -> Result<TokenStream, Error> {
let input: DeriveInput = syn::parse(input)?;
let input: DeriveInput = syn::parse2(input)?;
let ident = input.ident;

let generics = &input.generics;
Expand Down Expand Up @@ -359,16 +361,16 @@ fn try_enumeration(input: TokenStream) -> Result<TokenStream, Error> {
}
};

Ok(expanded.into())
Ok(expanded)
}

#[proc_macro_derive(Enumeration, attributes(prost))]
pub fn enumeration(input: TokenStream) -> TokenStream {
try_enumeration(input).unwrap()
pub fn enumeration(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
try_enumeration(input.into()).unwrap().into()
}

fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
let input: DeriveInput = syn::parse(input)?;
let input: DeriveInput = syn::parse2(input)?;

let ident = input.ident;

Expand Down Expand Up @@ -412,23 +414,21 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
}
}

let mut tags = fields
if fields.iter().any(|(_, field)| field.tags().len() > 1) {
panic!("variant with multiple tags"); // Not clear if this is possible, but good to be safe
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message is a regression, right? The old message was: "invalid oneof variant {}::{}: oneof variants may only have a single tag"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can re-expand this, sure

}
if let Some((duplicate_tag, _)) = fields
.iter()
.flat_map(|(variant_ident, field)| -> Result<u32, Error> {
if field.tags().len() > 1 {
bail!(
"invalid oneof variant {}::{}: oneof variants may only have a single tag",
ident,
variant_ident
);
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
}
Ok(field.tags()[0])
})
.collect::<Vec<_>>();
tags.sort_unstable();
tags.dedup();
if tags.len() != fields.len() {
panic!("invalid oneof {}: variants have duplicate tags", ident);
.flat_map(|(_, field)| field.tags())
.sorted_unstable()
.tuple_windows()
.find(|(a, b)| a == b)
{
bail!(
"invalid oneof {}: multiple variants have tag {}",
ident,
duplicate_tag
);
}

let encode = fields.iter().map(|(variant_ident, field)| {
Expand Down Expand Up @@ -519,10 +519,52 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
}
};

Ok(expanded.into())
Ok(expanded)
}

#[proc_macro_derive(Oneof, attributes(prost))]
pub fn oneof(input: TokenStream) -> TokenStream {
try_oneof(input).unwrap()
pub fn oneof(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
try_oneof(input.into()).unwrap().into()
}

#[cfg(test)]
mod test {
use crate::{try_message, try_oneof};
use quote::quote;

#[test]
fn test_rejects_colliding_message_fields() {
let output = try_message(quote!(
struct Invalid {
#[prost(bool, tag = "1")]
a: bool,
#[prost(oneof = "super::Whatever", tags = "4, 5, 1")]
b: Option<super::Whatever>,
}
));
assert!(output.is_err());
assert_eq!(
output.unwrap_err().to_string(),
"message Invalid has multiple fields with tag 1"
);
}

#[test]
fn test_rejects_colliding_oneof_variants() {
let output = try_oneof(quote!(
pub enum Invalid {
#[prost(bool, tag = "1")]
A(bool),
#[prost(bool, tag = "3")]
B(bool),
#[prost(bool, tag = "1")]
C(bool),
}
));
assert!(output.is_err());
assert_eq!(
output.unwrap_err().to_string(),
"invalid oneof Invalid: multiple variants have tag 1"
);
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading