Coder Social home page Coder Social logo

educe's Issues

Debug for unions is unsound, exposes uninitialized memory

Using #[derive(Educe) and #[educe(Debug)] on a union that may contain uninitialized bytes produces a debug implementation that has undefined behavior from safe code (excluding the unsafe block produced by the macro itself).

Here is a minimum reproduction:

use std::mem::MaybeUninit;
use educe::Educe;

#[derive(Educe)]
#[educe(Debug)]
union U {
    field: MaybeUninit<[u8; 256]>,
}

fn main() {
    let u = U { field: MaybeUninit::uninit() };
    println!("{:?}", u);
}

Reading uninitialized bytes, even as [u8], is UB, which can be verified with Miri and in the docs for MaybeUninit. This will happily print out uninitialized bytes from the stack on my machine.

Unless I'm missing something this also has the potential to be a security issue since leaking uninitialized bytes can be dangerous, for example leaking passwords in freed memory, although I don't know how often people are exposing Debug representations for unions.

For reference here is the macro output:

impl core::fmt::Debug for U {
    #[inline]
    fn fmt(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
        let mut builder = formatter.debug_tuple("U");
        let size = core::mem::size_of::<Self>();
        let data = unsafe {
            { core::slice::from_raw_parts(self as *const Self as *const u8, size) }
        };
        builder.field(&data);
        builder.finish()
    }
}

Wrong bounds applied to derived impls

The documentation says:

Generic parameters will be automatically bound to the Clone trait if necessary.

This suggests a precise bound. In fact the bound seems to be ad-hoc and sometimes wrong. For example:

#[derive(Educe)]
#[educe(Clone)]
pub struct Outer<T>(Option<T>);

This expands to:

impl<T> ::core::clone::Clone for Outer<T> {

(ie, without any bound) and obviously the implementation doesn't compile:

3 | #[derive(Educe)]
  |          ^^^^^ the trait `Clone` is not implemented for `T`
  |
  = note: required for `Option<T>` to implement `Clone`
  = note: this error originates in the derive macro `Educe` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `T`
  |
5 | pub struct Outer<T: std::clone::Clone>(Option<T>);
  |                   +++++++++++++++++++

I think there are about three reasonable behaviours:

  1. Never add bounds; ask the user to supply bound if one is needed. This is the default behaviour in educe 0.4.x. It has the virtues that the user has precise control, and if the user forgets, it doesn't compile.
  2. Add bounds for every generic parameter. This is the behaviour of std's derives, and of educe 0.4.x with #[educe(Clone(bound))].
  3. Add precise bounds for every field, ie in the above, you'd end up with something like ... where Option<T>: Clone.

I haven't looked at the source code in educe 0.5 but I think it may be adding bounds for a generic parameter T iff the T appears as such in a field, but not if it is inside some other type. That is quite a surprising behaviour.

Where the user wants to supply precise bounds, or none at all, with educe 0.5, the user can write #[educe(Clone(bound()))] or #[educe(Clone(bound(.....)))]. But a call site that previously used #[educe(Clone(bounds))], with educe 0.4, is not straightforward to convert. I think at the very least it would be nice to have a syntax that requested behaviour (2) - adding bounds for every generic parameter.

I thini ideally the default would be precise bounds ((3) in my list).

I've also noticed that the default bounds can be influenced by whether a method is supplied. For example:

#[derive(Educe, Debug)]
#[educe(Clone)]
pub struct Manual<T> {
//  whether Manual<T>: Clone has a T: Clone bound seems to depend on this
//    #[educe(Clone(method = "our_clone"))]
    field: T,
}

fn our_clone<T>(_input: &T) -> T {
    panic!()
}

This seems quite surprising.

If we do suppress the automatic bounds in this case, we should ideally provide a way to supplement the automatic bounds, as otherwise the user must write all the correct automatic bounds out again.

I think that would probably be a breaking change.

bug(Debug): error[E0275]: overflow evaluating the requirement `engine::tests::borsh_diff::Sequence2TupleEntryChange<'_, &engine::tests::borsh_diff::BorshView, &engine::tests::borsh_diff::BorshView>: std::marker::Sized`

with 0.6 release got this:

    = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`engine`)
    = note: required for `Vec<Sequence2TupleEntryChange<'_, &BorshView, &BorshView>>` to implement `std::fmt::Debug`
    = note: the full type name has been written to '/home/dz/github.com/Layer-N/nord/target/debug/deps/engine-1c44dcde9cb6e8a5.long-type-14849732555947787797.txt'
    = note: 126 redundant requirements hidden
    = note: required for `&engine::tests::borsh_diff::BorshDiffs<'_>` to implement `std::fmt::Debug`
    = note: required for the cast from `&&engine::tests::borsh_diff::BorshDiffs<'_>` to `&dyn std::fmt::Debug`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

Help with Default with generics not working.

#[derive(Educe, Debug)]
#[educe(Default(bound(T: Default), expression = Self { data: vec![1, 2, 3, 4, 5] }))]
struct Dummy<T> {
  data: T,
}
39 | #[educe(Default(bound(T: Default), expression = Self { data: vec![1, 2, 3, 4, 5] }))]
   |                                                              ^^^^^^^^^^^^^^^^^^^ expected type parameter `T`, found `Vec<{integer}>`
#[educe(Default)]
#[derive(Educe, Debug)]
struct DummyStructMusliGeneric<T> {
  #[educe(Default = true)]
  data: T,
}
43 |   #[educe(Default = true)]
   |                     ^^^^ the trait `From<bool>` is not implemented for `T`, which is required by `bool: std::convert::Into<_>`

`#[derive(Serde)]` shorthand for `#[derive(Serialize, Deserialize)]`

I have a few types with so many derives that rustfmt overflows them:

#[derive(
    Default,
    Clone,
    Copy,
    PartialEq,
    Eq,
    PartialOrd,
    Ord,
    Hash,
    EnumIter,
    Serialize,
    Deserialize,
    Debug
)]

It occurred to me that it might be worth a shorthand to combine Serialize and Deserialize. So my derives will look a lot nicer. Until there are even more and they overflow again...

#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, EnumIter, Serde, Debug)]

This crate, which I love for minimising boilerplate, occurred to me as a potential home for a Serde derive that does this, so I thought I'd suggest it here.

Handle recursive enum

First of all thank you for such an awesome crate. It's used in sea-query https://github.com/SeaQL/sea-query/blob/f4a5172b9493f355567ee01613e05b2886ed2673/src/value.rs#L132

I discovered upon 0.5.12 the code fails to compile, presumably due to the change introduced in 'ijackson-precise-bounds'.

Here is the testcase #26

May be recursive types is an undocumented feature that we somehow depended on? Or is this a bug introduced during refactoring?

Would love to hear your suggestions.

Ref: SeaQL/sea-query#781 (to reproduce there do cargo b --features=hashable-value,postgres-array

Namespace problems with MyDebug, RawString, and possibly others

While looking into #17, I discovered that educe's macro-generated output contains structs named things like MyDebug and RawString.

proc macro output must be extremely careful with identifiers. Everything is completely unhygienic, so generally names must at the very least be qualified.

Consider this test program:

use educe::Educe;
use std::fmt;

#[derive(Debug)]
pub struct Inner;

impl Inner {
    fn our_fmt(&self, _f: &mut fmt::Formatter) -> fmt::Result {
        Ok(())
    }
}

#[derive(Educe)]
#[educe(Debug(name = false))]
pub struct Thing {
    #[educe(Debug(method = "Inner::our_fmt"))]
    field: Inner,
    #[educe(Debug(name(Q)))]
    other: u32,
}

fn main() {
    let a = Thing {
        field: Inner,
        other: 6,
    };
    println!("{:?}", &a);
}

If search-and-replace Inner to RawString I get this:


  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
   |          |
   |          expected `<Thing as Debug>::fmt::RawString`, found `RawString`
   |          arguments to this struct are incorrect
   |
   = note: `RawString` and `<Thing as Debug>::fmt::RawString` have similar names, but are actually distinct types
note: `RawString` is defined in module `crate` of the current crate
  --> src/main.rs:5:1
   |
5  | pub struct RawString;
   | ^^^^^^^^^^^^^^^^^^^^
note: `<Thing as Debug>::fmt::RawString` is defined in module `crate` of the current crate
  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
note: tuple struct defined here
  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
   = note: this error originates in the derive macro `Educe` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no function or associated item named `our_fmt` found for struct `<Thing as Debug>::fmt::RawString` in the current scope
  --> src/main.rs:16:28
   |
13 | #[derive(Educe)]
   |          ----- function or associated item `our_fmt` not found for this struct
...
16 |     #[educe(Debug(method = "RawString::our_fmt"))]
   |                            ^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `RawString`

If I call it MyDebug I get this:

-*- mode: compilation; default-directory: "~/Rustup/Arti/experiments/" -*-
Compilation started at Mon Feb 19 19:41:22

nailing-cargo check --workspace --all-features
nailing-cargo: out-of-tree, git, building in: `/home/ian/Rustup/Arti/Build/experiments'
nailing-cargo: using really to run as user `rustcargo'
nailing-cargo: *WARNING* cwd is not in Cargo.nail thbough it has Cargo.toml!
nailing-cargo: nailed (0 manifests, 0 packages)
nailing-cargo: invoking: cargo check --locked --offline --workspace --all-features
From /home/ian/Rustup/Arti/experiments
   ec056cb..7f619ec             -> refs/nailing-cargo/src
    Checking foo v0.1.0 (/volatile/rustcargo/Rustup/Arti/experiments)
error[E0106]: missing lifetime specifier
  --> src/main.rs:17:12
   |
17 |     field: MyDebug,
   |            ^^^^^^^ expected named lifetime parameter
   |
help: consider using the `'a` lifetime
   |
17 |     field: MyDebug<'a>,
   |                   ++++

error[E0308]: mismatched types
  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
   |          |
   |          expected `&MyDebug<'_>`, found `&MyDebug`
   |          arguments to this struct are incorrect
   |
   = note: `MyDebug` and `<Thing as Debug>::fmt::MyDebug<'_>` have similar names, but are actually distinct types
note: `MyDebug` is defined in module `crate` of the current crate
  --> src/main.rs:5:1
   |
5  | pub struct MyDebug;
   | ^^^^^^^^^^^^^^^^^^
note: `<Thing as Debug>::fmt::MyDebug<'_>` is defined in module `crate` of the current crate
  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
note: tuple struct defined here
  --> src/main.rs:13:10
   |
13 | #[derive(Educe)]
   |          ^^^^^
   = note: this error originates in the derive macro `Educe` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no function or associated item named `our_fmt` found for struct `` in the current scope
  --> src/main.rs:16:28
   |
13 | #[derive(Educe)]
   |          ----- function or associated item `our_fmt` not found for this struct
...
16 |     #[educe(Debug(method = "MyDebug::our_fmt"))]
   |                            ^^^^^^^^^^^^^^^^^^ function or associated item not found in `MyDebug<'_>`

I will send an MR.

Panic when other attributes contain non-literal tokens

Hello, thank you for this crate. I was trying it out and I found this sample program fails with a compile error:

Cargo.toml:

[package]
name = "educetest"
version = "0.1.0"
edition = "2021"

[dependencies]
clap = { version = "4.2.3", features = ["derive"] }
educe = { version = "0.4.21", default-features = false, features = ["Default"] }

src/main.rs:

#[derive(clap::Parser, educe::Educe)]
#[educe(Default)]
#[command(author, version, about, long_about = None)]
struct Cli {
    /// Network port to use
    #[educe(Default = 1000)]
    #[arg(value_parser = clap::value_parser!(u16).range(1..))]
    port: u16,
}

fn main() {}

Output:

error: proc-macro derive panicked
 --> src/main.rs:1:24
  |
1 | #[derive(clap::Parser, educe::Educe)]
  |                        ^^^^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Error("expected literal")

However it goes away when I remove the #[arg(...)] attribute. It seems Educe is trying to parse all other attributes. Should it skip them and only parse #[educe(...)] attributes?

Multiple bounds are confused

Hey @magiclen, thanks for the handy crate!

Handling multiple (different) bounds seems to be broken. This:

#[derive(Educe)]
#[educe(Clone(bound = "T: Clone"), Default(bound = "T: Default"))]
struct Type<T>(T);

Expands to:

impl<T> core::default::Default for Type<T>
where
    T: Clone,
{
    #[inline]
    fn default() -> Self {
        Type(<T as core::default::Default>::default())
    }
}
impl<T> core::clone::Clone for Type<T>
where
    T: Default,
{
    #[inline]
    fn clone(&self) -> Self {
        Type(core::clone::Clone::clone(&self.0))
    }
    #[inline]
    fn clone_from(&mut self, _source: &Self) {
        core::clone::Clone::clone_from(&mut self.0, &_source.0);
    }
}

Which gives a bunch of errors:

error[E0277]: the trait bound `T: std::default::Default` is not satisfied
 --> src/main.rs:3:10
  |
3 | #[derive(Educe)]
  |          ^^^^^ the trait `std::default::Default` is not implemented for `T`
  |
  = note: required by `std::default::Default::default`
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
  |
3 | #[derive(Educe + std::default::Default)]
  |                ^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `T: std::clone::Clone` is not satisfied
 --> src/main.rs:3:10
  |
3 | #[derive(Educe)]
  |          ^^^^^ the trait `std::clone::Clone` is not implemented for `T`
  |
  = note: required by `std::clone::Clone::clone`
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
  |
3 | #[derive(Educe + std::clone::Clone)]
  |                ^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `T: std::clone::Clone` is not satisfied
 --> src/main.rs:3:10
  |
3 | #[derive(Educe)]
  |          ^^^^^ the trait `std::clone::Clone` is not implemented for `T`
  |
  = note: required by `std::clone::Clone::clone_from`
  = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
  |
3 | #[derive(Educe + std::clone::Clone)]
  |                ^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

Missing white spaces in default expressions

Hi and thanks for the library!

I found a bug: White spaces in expression are automatically removed from string literals.

Minimal example:

fn main() {
    println!("{:#?}", Struct::default());
}

#[derive(educe::Educe, Debug)]
#[educe(Default)]
struct Struct {
    #[educe(Default(expression = "\"as df\".to_owned()"))]
    a: String,
    #[educe(Default(expression = r#""as df".to_owned()"#))]
    b: String,
    #[educe(Default(expression = r#""as df""#))]
    c: &'static str,
    #[educe(Default("as df"))]
    d: &'static str,
}

Output (notice missing between s and d) in the fields a-c:

Struct {
    a: "asdf",
    b: "asdf",
    c: "asdf",
    d: "as df",
}

Tested with educe 0.4.22 in the browser Wasm.
Let me know when you need more info or help. Thanks.

Name collision if an enum field is named `f` or `builder`

One way to produce a bizarre error:

#[derive(Educe)]
#[educe(Debug)]
pub enum Enum {
    Variant { f: i32 },
}

Actual output:

error[E0599]: no method named `debug_struct` found for reference `&i32` in the current scope
 --> src/main.rs:3:10
  |
3 | #[derive(Educe)]
  |          ^^^^^ method not found in `&i32`

Expected output: it compiles.

The reason is that the expansion looks like this (excerpt from cargo expand):

    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        match self {
            Self::Variant { f } => {
                let mut builder = f.debug_struct("Variant");
                builder.field("f", f);
                builder.finish()
            }
        }
    }

So f was rebound by the pattern.

IMO the correct way to fix this is to bind each field to a local variable with a prefix:

            Self::Variant { f: v_f } => {
                let mut builder = f.debug_struct("Variant");
                builder.field("f", v_f);

This used to work in educe 0.4

Set default per type

I have some https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html and similar types used deeply within my own structs that I would like to add Default to. The structs are all generated.

I could generate literal values as mentioned at https://github.com/magiclen/educe#the-default-values-for-specific-fields, i.e #[educe(Default = 1)], but then I need the generator to know what field type it is, and in doing that I am almost at the point I dont need educe any more.

I am wondering whether it might be possible to have a mapping of type-name to default value, which educe uses as needed to get default literal values if there is none explicitly provided for each member.

e.g. something a bit like

    #[derive(Clone, Debug, Deserialize, Serialize)]
    #[educe(Default(literals = (Ipv4Addr = [0,0,0,0], Ipv6Addr = [0,0,0,0,...])))]
    pub struct Ipv4Range {
        #[educe(Default)]
        pub first: std::net::Ipv4Addr,
        pub last: std::net::Ipv4Addr,
    }

Explicitly defining bounds with multiple traits leads to spurious bounds

The following code:

#[derive(Educe)]
#[educe(Clone, Debug(bound(T: Debug)))]
pub struct Obj<T: Object, LocalDb: ClientSideDb> {
    ptr: DbPtr<T>,
    data: Arc<T>,

    #[educe(Debug(ignore))]
    db: Arc<ClientDb<LocalDb>>,
}

Leads to the Clone impl having a T: Debug bound, while I'd expect only the Debug implementation to have this bound.

Unfortunately I guess fixing this will require bumping educe to 0.6, as it's technically a breaking change, though it's also a bugfix.

PS: sorry for the non-minimized example, I'm in a hurry and just copy-pasted my current code; hopefully it'd also reproduce with an empty struct as expected! If not please let me know and I'll try to make a minimized example.

Suggestions for reducing implementation code duplication

Thanks for merging #15 and fixing up the rustfmt and the tests. (I'll see if I can manage to do the latter myself properly next time.)

While working on that I noticed that the implementation contains a lot of rather similar code which might profitably be refactored. I would like to propose concrete changesas MR(s). I'm filing this ticket to get your feedback and opinion about what you'd like to see.

I hope to get some steer from you and then make some code change(s) as MRs; then you can decide if you like them or want to stick with the existing approach, or do something different.

The first suggestion is specific to educe and relates to the comparison traits. The 2nd and 3rd suggestions are to adopt two patterns enabled by less-used Rust language features but commonly used in Rust derive macros to unify parts of their implementations. They have a natural ordering: if we're going to do all three, we should probably do them in this order.

Thanks for your crate, and for your consideration.

Similarity between Ord, PartialOrd and PartialEq code

These implementations are extremely similar. I think they could be combined into a single implementation, taking a small amount of target-trait-specific information.

To be more concrete:

pub trait ComparisonTraitHandler {
    /// Returns `cmp` or `partial_cmp` or `eq`
    fn method_name() -> proc_macro2::TokenStream;
    /// Returns `Some(::core::cmp::Ordering::Equal)` or `..::Equal` or `true`
    fn equal_value() -> proc_macro2::TokenStream;
    ....
}
impl<T: ComparisonTraitHandler> TraitHandler for T {
   ... most of the existing three impls goes here, combined into one ...
}

impl ComparisonTraitHandler for CloneEnumHandler { ... }

Always using match, unifying enum and struct code

Taking clone as an example, currently for enums we generate roughly this:

    fn clone(&self) -> Self {
        match self {
            Self::Variant1 { f: _s_f } => Self::Variant1 { ...

That code would be fine for structs too. We'd just need to leave out ::Variant1.

The result would be to abolish most of the *_struct.rs files.

Probably the easiest way to implement this would be to provide a helper function which takes a Data and returns an iterator of "variants" which might be either actual enum variants, or just the one struct variant:

struct VariantInfo<'d_> {
    field: &'d Fields,
    variant_name: Option<&'d Ident>,
}
fn variants_for_struct_or_enum(data: &'d Data) -> impl Iterator<Item = VariantInfo<'d>> {...

If you like this idea in principle, I think my next step would be to make an RFC MR demonstrating it for one of the trait impls. That would let you critique the details of the pattern before anyone does a lot of work on all the impls.

Always using braced syntax

In Rust, it's legal to match and construct unit and tuple structs and variants with the braced syntax:

struct Unit;
struct Tuple(i32);

let Unit { } = Unit { };
let Tuple { 0: x } = Tuple { 0: 42 };

This means that the code for Fields::Named can, more or less, be used for tuples and units too. The only wrinkle is the need to handle the field name being either an index or an identifier. A small amount of utility code for obtaining the "field name" and derived field names would allow us to remove the different handling for Fields::Unnamed and Fields::Unit.

Again, if you like this idea, the next thing to do would be try this out for one of the implementations.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.