Integrate all (dummy) subsystems with the Overseer

[ordian]

Fixes #1317.

Please bear with me while I make these baby steps.
This boilerplate looks ugly so as an experiment I looked at a type map approach 2e934c3 (instead of storing a bunch of fields, we can store a HashMap<Type, Box<SomeTrait>>) but then reverted this change for now. Let me know if it's worth exploring.

[coriolinus]

You're definitely right that the repetition here is annoying. I think that you're also right that the runtime typemap is not the right way to handle it, because it prevents things like exhaustive matching.

I'm imagining a pair of macros which work like this:

// declare a list of types; produces some sort of private module? const item? not sure.
declare_types!(subsystem_messages, CandidateBackingMessage, CandidateSelectionMessage, ...);

// iterate over the types declared earlier. Produce a new ident from each closure provided.
// final item has appropriate idents substituted.
for_each_type_in!(
  subsystem_messages, 
  |type_name| to_snake_case(type_name.truncate(type_name.len() - "Message".len())),
  |type_name| to_snake_case(type_name.truncate(type_name.len() - "Message".len())) + "_subsystem",
  {
    let $1 = spawn (
      &mut s,
      &mut running_subsystems,
      &mut running_subsystems_rx,
      $0,
    )?;
  }
);

or

for_each_type_in!(
  subsystem_messages, 
  |type_name| to_snake_case(type_name.truncate(type_name.len() - "Message".len())) + "_subsystem",
  {
    if let (ref mut s) = self.$0.instance {
      let _ = s.tx.send(FromOverseer::Signal(OverseerSignal::Conclude)).await;
    }
  }
);

There would need to be an explicit match generator as well:

for_each_type_in!(
  subsystem_messages, 
  |type_name| type_name.truncate(type_name.len() - "Message".len()),
  |type_name| to_snake_case(type_name.truncate(type_name.len() - "Message".len())) + "_subsystem",
  match msg {
    AllMessage::$0(msg) => {
      if let Some(ref mut s) = self.$1.instance {
        let _ = s.tx.send(FromOverseer::Communication{ msg }).await
      }
    }
  }
);

It would mean two fairly complicated procedural macros, but it would also eliminate quite a lot of manual duplication of code... provided that we can come up with a good way to store a bunch of type idents in declare_types.

Alternately, we could skip declare_types entirely and just paste in the full type list for each invocation of for_each_type_in, but then we have the problem of copy-paste synchronization errors again.

[ordian]

@coriolinus thanks for the detailed feedback!

I was hesitant to throw macros at the problem, because in my experience while it can solve some problems, it inevitably create others, like code readability (now you need to read the source of the macro/docs/tests), maintenance, longer compile times, IDE-unfriendliness. But given the love of macros at Parity, I might reconsider :)

I think that you're also right that the runtime typemap is not the right way to handle it, because it prevents things like exhaustive matching.

Actually, I don't think it does prevent exhaustive matching, but to be fair I haven't explored this avenue fully. It also can guarantee exhaustive listing of subsystem fields via iteration.

[coriolinus]

You're right that there are a bunch of downsides to throwing macros at a problem, but they do come with one key upside: they make correctness a lot easier. If we use macros set up as I suggested, then adding a new subsystem means that, beyond implementing the subsystem itself, you just need to add it to the declare_types list. Without it, you have to add a copy-paste case in a bunch of different places, none of which are easy to grep for. IMO that makes maintenance easier, not harder.

[montekki]

I don't think it's a good idea to add macros in this situation where the code is quite straightforward (but bloated). One of the most frequent complaints I've heard is our code being very macro-heavy.

[ordian]

@coriolinus makes a good point about correctness though. It's not currently guaranteed at compile time that we didn't forget to check a certain subsystem e.g. in broadcast_signal. OTOH, the tests should catch it and I don't know if we we'll be adding any new subsystem anytime soon.

[rphmeier]

Thanks