Add PrintDecorator hooks for Waffle's IR

[michelledaviest]

Added trait PrintDecorator to add hooks to print information after inst, before and after blocks and before and after functions. Added default implementations for these hooks so Waffle's display function output doesn't change with this addition. I had all internal waffle logging calls to display take in a NOPPrintDecorator that does not print anything.

[cfallin]

Thanks for this! This is basically what I had imagined, with one detail changed: I don't think we'll want a "no-op decorator", but instead let's make the distinction at the API level (and leave the existing methods unchanged). Otherwise looks great. Thanks!

[cfallin]

It's a tiny bit silly in this case (since the purpose is fairly apparent) but I like to stick to a style of mandatory doc comments on all items that are pub -- could we add those here too? Something like:

/// Print arbitrary text after an instruction.
///
/// Invoked after every instruction in a block. The instruction has already been printed on its own line; this method can print content on the line below the operator, if desired.

and likewise below.

[michelledaviest]

For clarity, the after_inst instruction prints information on the same line as the operator after the operator has been printed.

[cfallin]

Here, Option<&'a PD> if we do as above

[cfallin]

any particular reason to split out the newline like this?

[cfallin]

(looking below: ah, is a decorator call missing? or is this for symmetry with the below?)

[michelledaviest]

This was a change leftover from refactoring. I've changed it to a writeln! call.

[cfallin]

Thanks! This is close -- just a few more tweaks!

[cfallin]

Spurious comma here? (Syntactically valid but probably an artifact of refactor)

[cfallin]

I just realized there's a cleaner way to do this that doesn't require allocating a HashMap up front and passing ownership -- could we instead parameterize also on F: FnMut(Func) -> PD and take f: F?

In other words I think it might be good to make this generic with a callback that the caller provides to get the decorator for any given function; FnMut so it can lazily create it if needed. And would prefer to return a PD by value but feel free to do Cow<'b, PD> instead to give the caller the option of either by-value or by-borrow; I don't want the API to require one to create and store these elsewhere though.

[michelledaviest]

I can't have it be a FnMut since Display only takes in a &self.
So my ModuleDisplay now looks like below. I made the function return an Option so I could pass in None for all functions in the normal display function.

pub struct ModuleDisplay<'a, PD: PrintDecorator, F: Fn(Func) -> Option<PD>> {
    pub(crate) module: &'a Module<'a>,
    pub(crate) decorators: F,
}

However, I'm having trouble implementing the display function for Module in a way that doesn't expose these type parameters to the user. Ideally I am able to have both the PrintDecorator and function returning the decorator be impl, like so:

pub fn display<'b>(
    &'b self,
) -> ModuleDisplay<'b, impl PrintDecorator, impl Fn(Func) -> Option<impl PrintDecorator>>
where
    'b: 'a,
{
    ModuleDisplay::<NOPPrintDecorator, dyn Fn(Func) -> Option<NOPPrintDecorator>> {
        module: self,
        decorators: |x: Func| None,
    }
}

However, this doesn't work since the trait Sized is not implemented for dyn Fn(ir::Func) -> std::option::Option<display::NOPPrintDecorator>. I am able to get no type errors by exposing the generics to the user but ideally we don't want that...

[michelledaviest]

The display_with_decorator looks like this:

pub fn display_with_decorator<'b, PD: PrintDecorator, F: Fn(Func) -> Option<PD>>(
    &'b self,
    decorators: F,
) -> ModuleDisplay<'b, PD, F>
where
   'b: 'a,
{
    ModuleDisplay {
        module: self,
        decorators: decorators,
    }
}

[cfallin]

Does the first variant above work without the type ascription (ModuleDisplay { ... } expression only)? Or alternately, impl Fn rather than dyn Fn?

We're definitely getting into the weeds of the Rust type system here but there has to be a way to make this work!

[michelledaviest]

So impl Fn doesn't work because impl isn't allowed in paths (? idk what a path is). impl Trait is only allowed in arguments and return types of functions and methods.

Removing the type ascription works, however, we now get the error expected Option<impl PrintDecorator>, found Option<dyn PrintDecorator>. I think the problem is that it doesn't know that the function we're passing in will return a PrintDecorator which we have already impl'd.

pub fn display<'b>(
    &'b self,
) -> ModuleDisplay<'b, impl PrintDecorator, impl Fn(Func) -> Option<PrintDecorator>>
where
    'b: 'a,
{
    ModuleDisplay {
        module: self,
        decorators: |x: Func| -> Option<NOPPrintDecorator> { None },
    }
}  

[cfallin]

OK, one more idea: can we use a Box<dyn Fn(...) -> ...> type for decorators and avoid the type parameter in ModuleDisplay altogether?

[cfallin]

(or Option<Box<dyn Fn...>> if we want to avoid the allocation in the common case)

[michelledaviest]

It works!! Or at least it type checks. I'll push after I test it against my solver.

[michelledaviest]

Verified this works with my generators.

[cfallin]

@michelledaviest do you need a point-release for this or are you working off a fork or repo internally? (It's easy to do, just curious about urgency)

[michelledaviest]

I'm working off this fork so no rush