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

mumbleskates · 2023-11-25T21:46:25Z

Currently it seems that the "invalid oneof variant" Errs are always discarded because they are being passed directly into flat_map from a closure, rather than actually propagating the error.

This PR also streamlines the checks for duplicated tag numbers and adds some rudimentary tests for the affected code in those proc macros.

prost-derive/src/lib.rs

mumbleskates · 2023-11-25T22:40:51Z

prost-derive/src/lib.rs

+        .sorted_unstable()
+        .tuple_windows()
+        .find(|(a, b)| a == b)


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

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?

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

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

it was not.

…rrors

caspermeijn

This fixes the error reporting for duplicate tags, improves the error message and adds tests. I like it.

prost-derive/src/lib.rs

caspermeijn · 2024-05-22T12:23:05Z

prost-derive/src/lib.rs

+        .sorted_unstable()
+        .tuple_windows()
+        .find(|(a, b)| a == b)


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?

prost-derive/src/lib.rs

caspermeijn · 2024-05-22T12:29:25Z

prost-derive/src/lib.rs

@@ -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


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

i can re-expand this, sure

prost-derive/src/lib.rs

…condition

…o hack check

prost-derive/Cargo.toml

prost-derive/src/lib.rs

caspermeijn · 2024-05-23T08:04:37Z

prost-derive/src/lib.rs

+        .sorted_unstable()
+        .tuple_windows()
+        .find(|(a, b)| a == b)


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

prost-derive/src/lib.rs

Co-authored-by: Casper Meijn <casper@meijn.net>

…cates()

mumbleskates · 2024-05-24T09:22:00Z

incidentally, most (all? probably, all) of my contributions to Prost were discovered/developed while forking and almost completely rewriting it. Downstream of here i have changed the nature of the encoding slightly (not dramatically), but i have also achieved huge strides in usability:

fully trait-based dispatch for encoding and decoding, with derive attributes simple enough to use on existing user types and support for generics and lifetimes on message types
ignored fields
useful functionality on the object-safe traits
oneofs that may bear their own empty variant rather than needing to nest within Option
oneofs in messages must have known matching tags, checked at compile time
greatly increased type support, including nesting values in ways that protobuf schemas cannot even express
novel linear-time encoding via writing the output back-to-front, which can be dramatically faster without caching extra data inside the structs
guaranteed field ordering (protobuf as an encoding has no real use for this)
distinguished decoding of canonical data (protobuf can't do this as easily)
all of this and it's actually faster

if any of these sound useful to upstream it could be a good project for myself or someone to trace back over my steps in achieving ~~all~~ some of these. some of these can probably be upstreamed in a day or two, others could take longer.

caspermeijn

Thanks for your contribution

prost-derive/src/lib.rs

caspermeijn · 2024-05-24T10:00:53Z

if any of these sound useful to upstream it could be a good project for myself or someone to trace back over my steps in achieving all these. some of these can probably be upstreamed in a day or two, others could take longer.

Improvements are welcome. However, review capacity is low, so bringing in a reviewer would be appreciated.

Keep in mind that prost is focused on an unopinionated implementation of protobuf.

mumbleskates commented Nov 25, 2023

View reviewed changes

prost-derive/src/lib.rs Show resolved Hide resolved

mumbleskates force-pushed the deduping branch 2 times, most recently from 31aa6de to b30f84b Compare November 25, 2023 22:40

mumbleskates commented Nov 25, 2023

View reviewed changes

mumbleskates marked this pull request as ready for review November 26, 2023 00:10

mumbleskates force-pushed the deduping branch 2 times, most recently from ef347ff to 01eaa46 Compare May 17, 2024 13:56

better checking of tag duplicates, avoid discarding invalid variant e…

dbd5111

…rrors

mumbleskates force-pushed the deduping branch 3 times, most recently from 169f10c to b235c8b Compare May 17, 2024 14:08

add some simple derive tests :)

4c00d4e

mumbleskates force-pushed the deduping branch from b235c8b to 4c00d4e Compare May 17, 2024 14:22

caspermeijn requested changes May 22, 2024

View reviewed changes

mumbleskates added 6 commits May 22, 2024 16:30

use itertools duplicates()

12e65f3

don't print out unreadable syn junk when encountering unknown attributes

0d0a1dd

clarify and test backstop for the "multiple tags in a oneof variant" …

0b3216a

…condition

use expect_err

6cfd80f

nicer framing around the unknown attribute tokens

02bc132

express higher minimal versions of itertools and proc-macro2 for carg…

be6f140

…o hack check

mumbleskates force-pushed the deduping branch from 30b50fd to be6f140 Compare May 23, 2024 00:04

mumbleskates requested a review from caspermeijn May 23, 2024 00:15

caspermeijn requested changes May 23, 2024

View reviewed changes

mumbleskates and others added 3 commits May 23, 2024 22:21

simplify bounds for proc-macro2

2545542

Co-authored-by: Casper Meijn <casper@meijn.net>

update the other instance of the .tuple_windows() trick to use .dupli…

aa45a14

…cates()

clarify & shorten assertion

af24da1

mumbleskates force-pushed the deduping branch from 1dab3e8 to af24da1 Compare May 24, 2024 05:22

mumbleskates requested a review from caspermeijn May 24, 2024 09:22

caspermeijn approved these changes May 24, 2024

View reviewed changes

prost-derive/src/lib.rs Show resolved Hide resolved

prost-derive/src/lib.rs Outdated Show resolved Hide resolved

caspermeijn added this pull request to the merge queue May 24, 2024

Merged via the queue into tokio-rs:master with commit eec717b May 24, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mumbleskates commented Nov 25, 2023 •

edited

mumbleskates Nov 25, 2023

caspermeijn May 22, 2024

mumbleskates May 22, 2024

caspermeijn May 23, 2024

mumbleskates May 24, 2024

caspermeijn left a comment

caspermeijn May 22, 2024

caspermeijn May 22, 2024

mumbleskates May 22, 2024

caspermeijn May 23, 2024

mumbleskates commented May 24, 2024 •

edited

caspermeijn left a comment

caspermeijn commented May 24, 2024

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

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

Conversation

mumbleskates commented Nov 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspermeijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumbleskates commented May 24, 2024 • edited

caspermeijn left a comment

Choose a reason for hiding this comment

caspermeijn commented May 24, 2024

mumbleskates commented Nov 25, 2023 •

edited

mumbleskates commented May 24, 2024 •

edited