Refactor Host and AdviceProvider

[plafer]

Builds on #1571

A round of refactoring to make the PR that addresses #1457 more self contained.

[bobbinth]

Thank you! Not a full review, but I left a couple of small comments inline.

More generally, I'd like to understand what is the overall plan for the refactoring. Depending on how we go, it seems to me that a subsequent PR may need to reverse many of the changes in this PR.

For example, I'm imagining that Host::on_event() method will handle the actual dispatch of advice injectors. It could look something like:

fn on_event(
    &mut self,
    process: ProcessState,
    event_source: u32,
    event_id: u32
) -> Result<(), ExecutionError> {
    let handler = self.get_event_handler(event_source_id)?;
    handler.process(event_id, process, self.advice_provider_mut())
}

If we go this way, instead of removing individual advice injector functions, we'd remove the corresponding functions from the AdviceProvider trait. This would make the AdviceProvider trait pretty general (i.e., all injector functionality would be defined separately, while the trait will contain mostly "required" methods).

[bobbinth]

Should we change &mut self to just &self here? The thought is that debug decorators should not modify the state of the host. But maybe there is a good reason not to do that.

Also nit: I'd probably name the parameters process and options and add a clippy exception to this method (this way it will look nicer in docs).

[bobbinth]

Same comment as above.

[plafer]

More generally, I'd like to understand what is the overall plan for the refactoring.

I saw this PR as a cleanup that we'd want no matter ultimate direction of the subsequent PR. The main contribution is to remove the AdviceProvider::set_advice() and AdviceProvider::get_advice(). For example, set_advice() shoehorned all possible advice injector functionality into a single method call, and then aggregated all possible return types into this HostResponse (with From implementations that would panic if the wrong type is returned). Basically I changed it so that the caller calls the appropriate AdviceProvider method instead, and get the appropriate return type directly instead of through this HostResponse, removing all that complexity. Similarly for AdviceProvider::get_advice().

Then the Host re-exposed those set_advice() and get_advice() methods, blurring the line between Host and AdviceProvider. In the current approach, the Host exposes the advice provider, and the caller can the interact with it directly. It makes for a cleaner separation where the Host manages the advice provider, rather than "being one". It's also easier to refactor - if we change the AdviceProvider, we don't have to duplicate the same changes on the Host.

Finally I inlined all the AdviceProvider default methods to remove a level of indirection - most of these were just a few lines.

Depending on how we go, it seems to me that a subsequent PR may need to reverse many of the changes in this PR.

For example, I'm imagining that Host::on_event() method will handle the actual dispatch of advice injectors. It could look something like:

That's fine, I don't think this undoes any of the changes here - just more moving things around.

If we go this way, instead of removing individual advice injector functions, we'd remove the corresponding functions from the AdviceProvider trait.

I agree with this direction - I'd like the AdviceProvider trait to look as closely as our high-level description of it as possible. I thought about making the AdviceProvider trait have only 3 methods: return a stack, map, and store. But then this would be a lot of changes and this PR was already decently big.

[bobbinth]

Looks good! Thank you! Let's merge this together with #1581.