Refactor #[derive(Filter)] for better parsing and readability

[ilchu]

As mentioned in #2431 PR discussion, there are still a few improvements that could be made to the Filter derive macro in data_model/derive:

• More modular functions
• Custom structs with Parse to make invalid enums fail more predictably
• Better naming conventions
• Less nested mapping

[0x009922]

I made some research about the Filter macro. I am confused by its documentation.

There is an example of how macro expands from this:

use iroha_data_model_derive::{Filter, IdEqOrdHash};
use iroha_data_model::prelude::{HasOrigin, Identifiable};
use serde::{Deserialize, Serialize};


#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Filter, Deserialize, Serialize)]
pub enum LayerEvent {
    SubLayer(SubLayerEvent),
    Created(LayerId),
}

pub enum SubLayerEvent {
    Created(SubLayerId),
}

pub struct LayerId {
    name: u32,
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct SubLayerId {
    name: u32,
    parent_id: LayerId,
}

#[derive(Debug, Clone, IdEqOrdHash)]
pub struct Layer {
    id: <Self as Identifiable>::Id,
}

#[derive(Debug, Clone, IdEqOrdHash)]
pub struct SubLayer {
    id: <Self as Identifiable>::Id,
}

impl HasOrigin for LayerEvent {
    type Origin = Layer;

    fn origin_id(&self) -> &<Layer as Identifiable>::Id {
        match self {
            Self::SubLayer(sub_layer) => &sub_layer.origin_id().parent_id,
            Self::Created(id) => id,
        }
    }
}

impl HasOrigin for SubLayerEvent {
    type Origin = SubLayer;

    fn origin_id(&self) -> &<SubLayer as Identifiable>::Id {
        match self {
            Self::Created(id) => id,
        }
    }
}

To this:

#[doc = " Filter for LayerEvent entity"]
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, derive_more::Constructor, Decode, Encode, Deserialize, Serialize, IntoSchema)]
pub struct LayerFilter {
    origin_filter:
        crate::prelude::FilterOpt<crate::prelude::OriginFilter<crate::prelude::LayerEvent>>,
    event_filter: crate::prelude::FilterOpt<LayerEventFilter>,
}
impl LayerFilter {
    #[doc = " Construct new LayerFilter"]
    pub const fn new(
        origin_filter: crate::prelude::FilterOpt<
            crate::prelude::OriginFilter<crate::prelude::LayerEvent>,
        >,
        event_filter: crate::prelude::FilterOpt<LayerEventFilter>,
    ) -> Self {
        Self {
            origin_filter,
            event_filter,
        }
    }
    #[doc = r" Get `origin_filter`"]
    #[inline]
    pub const fn origin_filter(
        &self,
    ) -> &crate::prelude::FilterOpt<crate::prelude::OriginFilter<crate::prelude::LayerEvent>> {
        &self.origin_filter
    }
    #[doc = r" Get `event_filter`"]
    #[inline]
    pub const fn event_filter(&self) -> &crate::prelude::FilterOpt<LayerEventFilter> {
        &self.event_filter
    }
}
impl crate::prelude::Filter for LayerFilter {
    type EventType = crate::prelude::LayerEvent;
    fn matches(&self, event: &Self::EventType) -> bool {
        self.origin_filter.matches(event) && self.event_filter.matches(event)
    }
}
#[doc = " Event filter for LayerEvent entity"]
#[allow(clippy::enum_variant_names, missing_docs)]
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Decode, Encode, Deserialize, Serialize, IntoSchema)]
pub enum LayerEventFilter {
    ByCreated,
    BySubLayer(crate::prelude::FilterOpt<SubLayerFilter>),
}
impl crate::prelude::Filter for LayerEventFilter {
    type EventType = crate::prelude::LayerEvent;
    fn matches(&self, event: &crate::prelude::LayerEvent) -> bool {
        match (self, event) {
            (Self::ByCreated, crate::prelude::LayerEvent::Created(_)) => true,
            (Self::BySubLayer(filter_opt), crate::prelude::LayerEvent::SubLayer(event)) => {
                filter_opt.matches(event)
            }
            _ => false,
        }
    }
}

However, when I expand it with cargo expand, it seems that there is no any output from Filter macro. You can see a repro in my fork.

@ilchu, could you comment on it?

Also, I think that the example is cumbersome and docstring should contain something shorter and more expressive.

[0x009922]

Several ideas I have to improve the macro:

• Due to name scoping, the macro currently properly expands only from within the iroha_data_model crate as it relies on a few of crate::prelude imports.

  It relies not only on crate::prelude. Here is the complete set of imports required to use Filter properly:

  use iroha_data_model::prelude;
  use iroha_data_model_derive::Filter;
  use iroha_schema::IntoSchema;
  use parity_scale_codec::{Decode, Encode};
  use serde::{Deserialize, Serialize};
  
  #[derive(Filter)]
  enum AccountEvent {
      Registered(u32),
  }
  
  fn main() {}

  It is a self-contained integration test.

  In order to not rely on such a convention, I propose to declare a submodule for macro with essential imports, like iroha_data_model::derive::util:

  pub use parity_scale_codec::{Decode, Encode};
  // etc

  So that the macro could rely on imports from that module instead of assuming that something is imported in the scope.

• This macro also depends on the naming conventions adopted so far, such as that
  Event enums always have tuple variants with either some sort of Id or another Event inside of them, as well as that all Event inner fields precede Id fields in the enum definition.

  As far as I can see, the macro doesn't emit any errors if this convention is violated.

Apart from this, I don't see what else to refactor in the impl_filter proc macro. Its code is quite straightforward and doesn't contain much logic. Maybe it makes sense to go away from manual impl Parse and rely on darling though, which is relevant to other macros in iroha_data_model_derive as well.

[DCNick3]

All of these were addressed in #3897 I think?