Support for Option<T> and Vec<T> with T as a struct/enum

[romanoji]

Hello maintainers! 👋 The crate seems to lack of support for Option<T>, where T is a struct or an enum and for Vec<T>.

Currently, it's not possible to sanitize a struct as a whole which relies on generic types other than Option<T>, with T as a primitive.

Here's an example:

use sanitizer::prelude::*;

#[derive(Sanitize, Debug, PartialEq)]
struct Person {
    #[sanitize(trim)]
    first_name: Option<String>,
    // Optional nested struct should be sanitized if it's present
    #[sanitize]
    personal_details: Option<PersonalDetails>,
    // Optional nested enum should be sanitized if it's present
    #[sanitize]
    job: Option<Job>,
    // Vec should be sanitized if it's present.
    #[sanitize]
    pets: Vec<Pet>,
}

#[derive(Sanitize, Debug, PartialEq)]
struct PersonalDetails {
    #[sanitize(trim)]
    last_name: Option<String>,
    #[sanitize(trim)]
    address: Option<Option<String>>,
}

#[derive(Sanitize, Debug, PartialEq)]
enum Job {
    Plumber,
    Scientist,
    #[sanitize(trim)]
    Other(String),
}

#[derive(Sanitize, Debug, PartialEq)]
enum Pet {
    Dog,
    Cat,
    #[sanitize(trim)]
    Other(String)
}

#[test]
fn optional_nested_fields_are_sanitized_if_present_test() {
    let mut instance = Person {
        first_name: Some(String::from("  Gordon  ")),
        personal_details: Some(PersonalDetails {
            last_name: Some(String::from("  Freeman  ")),
            address: Some(Some(String::from("  unknown  "))),
        }),
        job: Some(Job::Other(String::from("  Resistance leader  "))),
        pets: vec![Pet::Other(String::from("  Headcrab  "))],
    };
    let expected = Person {
        first_name: Some(String::from("Gordon")),
        personal_details: Some(PersonalDetails {
            last_name: Some(String::from("Freeman")),
            address: Some(Some(String::from("unknown"))),
        }),
        job: Some(Job::Other(String::from("Resistance leader"))),
        pets: vec![Pet::Other(String::from("Headcrab"))],
    };

    instance.sanitize();

    assert_eq!(instance, expected);
}

When running such code it fails with the following errors:

> cargo test --test example

   Compiling sanitizer_macros v0.2.2 (/Users/owner/Projects/sanitizer/sanitizer-macros)
error[E0308]: mismatched types
 --> sanitizer-macros/tests/example.rs:3:10
  |
3 | #[derive(Sanitize, Debug, PartialEq)]
  |          ^^^^^^^^ expected enum `Job`, found enum `Option`
  |
  = note: expected mutable reference `&mut Job`
             found mutable reference `&mut Option<Job>`
  = note: this error originates in the derive macro `Sanitize` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
 --> sanitizer-macros/tests/example.rs:3:10
  |
3 | #[derive(Sanitize, Debug, PartialEq)]
  |          ^^^^^^^^ expected struct `PersonalDetails`, found enum `Option`
  |
  = note: expected mutable reference `&mut PersonalDetails`
             found mutable reference `&mut Option<PersonalDetails>`
  = note: this error originates in the derive macro `Sanitize` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0107]: missing generics for struct `Vec`
   --> sanitizer-macros/tests/example.rs:15:11
    |
15  |     pets: Vec<Pet>,
    |           ^^^ expected at least 1 generic argument
    |
note: struct defined here, with at least 1 generic parameter: `T`
   --> /Users/owner/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:398:12
    |
398 | pub struct Vec<T, #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global> {
    |            ^^^ -
help: add missing generic argument
    |
15  |     pets: Vec<T><Pet>,
    |           ~~~~~~

Some errors have detailed explanations: E0107, E0308.
For more information about an error, try `rustc --explain E0107`.
error: could not compile `sanitizer_macros` due to 3 previous errors

I came up with a simple workaround for now, but it's quite bothersome to handle for more complex structs. It also requires to drop #[sanitize] proc macro to make it work.
Here's a rough idea, based on an example shared above:

fn optional_nested_fields_are_sanitized_manually_if_present_test() {
  // (...)

  instance.sanitize();
  instance.personal_details.as_mut().map(sanitizer::Sanitize::sanitize);
  instance.job.as_mut().map(sanitizer::Sanitize::sanitize);
  instance.pets.iter_mut().map(sanitizer::Sanitize::sanitize).collect::<Vec<_>>();

  assert_eq!(instance, expected);
}

Based on my initial research it seems that handling Option<T> for structs/enums might be not that hard to implement.
However, I'm not so sure about Vec<T> (and perhaps other iterables too).

Looking forward to hear your thoughts about it. 🙂

[Daksh14]

Looking into this

[Daksh14]

@romanoji I can add a implementation for Vector, the code generated is different for each data structure, so I don't think we can support this for every generic T<X>

[romanoji]

@Daksh14, sounds good to me. 👍

If you could handle it also for Option<T> that'd be awesome. I suppose it'd be sufficient for most of the cases.

[misl-smlz]

We are running into the same Problem. @romanoji what workaround did you use? @Daksh14 anything we can help in implementing this?

[romanoji]

@misl-smlz, as I mentioned in my initial comment, I dropped #[sanitize] annotations from the top-level struct (for Vec<T> and Option<T>, except for Option<String>) and run sanitization for each of those attributes manually. I don't remember all the details right now but that was the most straightforward solution.

[Expurple]

I can add a implementation for Vector, the code generated is different for each data structure, so I don't think we can support this for every generic T<X>

You can be generic over something like fmap::FunctorMut - an existing functor abstraction that's already implemented for the standard data structures, including Option and Vec. I think, this is the best way to approach this.

[Daksh14]

I can add a implementation for Vector, the code generated is different for each data structure, so I don't think we can support this for every generic T<X>

You can be generic over something like fmap::FunctorMut - an existing functor abstraction that's already implemented for the standard data structures, including Option and Vec. I think, this is the best way to approach this.

Sorry for being inactive, The library uses syn so what type would that correspond to there?

I would also have to update the syn version because we use 1.x and 2.x is out

[Expurple]

My initial idea was about providing a blanket impl Sanitize in sanitizer, without changing the code generated by the proc macro in dependent crates:

/// A blanket impl that allows sanitizing values inside of common data structures like [Option] and [Vec].
///
/// ## Example
///
/// ```
/// use sanitizer::Sanitize;
///
/// #[derive(Debug, PartialEq)]
/// struct MyValue(i32);
///
/// impl Sanitize for MyValue {
///     /// If the inner value is `0`, change it to `1`.
///     fn sanitize(&mut self) {
///         if self.0 == 0 {
///             self.0 = 1;
///         }
///     }
/// }
///
/// let mut wrapped_value = Some(MyValue(0));
/// wrapped_value.sanitize();
/// assert_eq!(wrapped_value, Some(MyValue(1)));
/// ```
impl<'a, A, T> Sanitize for T
where
    A: 'a,
    T: FunctorMut<'a, A>,
    T::Inner: Sanitize,
{
    fn sanitize(&mut self) {
        self.fmap_mut(Sanitize::sanitize)
    }
}

But I tried making a PR and I get

error[E0207]: the type parameter `A` is not constrained by the impl trait, self type, or predicates

which I'm not sure how to resolve.

If we won't be able to resolve this, then I'd just provide similar generic impls for Option and Vec directly: #15