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

Allow flattening of a struct through derive(EncodeLabelSet) at any position #135

Open
drbrain opened this issue Apr 15, 2023 · 2 comments
Open
Labels
help wanted Extra attention is needed

Comments

@drbrain
Copy link

drbrain commented Apr 15, 2023

Currently when deriving EncodeLabelSet and flattening a struct the flattened struct must appear last, and there must be only one flattened struct, from the test:

#[test]
fn flatten() {
    #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    struct CommonLabels {
        a: u64,
        b: u64,
    }
    #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    struct Labels {
        unique: u64,
        #[prometheus(flatten)]
        common: CommonLabels,
    }

    // …

If you place common before unique in struct Labels like this:

#[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
struct Labels {
    #[prometheus(flatten)]
    common: CommonLabels,
    unique: u64,
}

Compilation fails:

error[E0382]: borrow of moved value: `encoder`
   --> derive-encode/tests/lib.rs:147:14
    |
147 |     #[derive(EncodeLabelSet, Hash, Clone, Eq, PartialEq, Debug)]
    |              ^^^^^^^^^^^^^^
    |              |
    |              value borrowed here after move
    |              move occurs because `encoder` has type `LabelSetEncoder<'_>`, which does not implement the `Copy` trait
    |

This occurs because ownership of encoder is given to the flattened struct

I think this would be a breaking change to fix, but fixing it would allow fields to appear in any order, or allow flattening of multiple structs without nesting them all one inside the other in tail position.

@mxinden
Copy link
Member

mxinden commented May 9, 2023

Thanks for debugging and the detailed issue.

I think this would be a breaking change to fix, but fixing it would allow fields to appear in any order, or allow flattening of multiple structs without nesting them all one inside the other in tail position.

Worth exploring passing &mut LabelSetEncoder. Not sure why I chose to require ownership.

Another alternative is to provide a better error message in the derive macro when the flattened property is not the last.

@olix0r
Copy link

olix0r commented Dec 7, 2023

Worth exploring passing &mut LabelSetEncoder.

My initial impression is that this would be a welcome API change. If we could impl<A: EncodeLabelSet, B: EncodeLabelSet> EncodeLabelSet for (A, B), that would generally make label composition more flexible.

@mxinden mxinden added the help wanted Extra attention is needed label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants