Store lifecycle hooks dynamically

[9999years]

Instead of having a field for each event (before_restart_ghci, test_ghci, after_startup_shell, etc.), we generate and store the arguments dynamically. This simplifies the running of hooks and will hopefully make it easier to extend the lifecycle hooks in the future.

[Gabriella439]

I think rather than check at runtime that we don't have an invalid combination of LifecycleEvent and When we should enforce that statically by combining them into a single data structure using something like this:

pub enum Bounded {
    Before,
    After,
}

pub enum LifecycleEvent {
    Test,
    StartUp{ when: When },
    Reload{ when: When },
    Restart{ when: When },
}

[9999years]

I did consider this, but it gets kind of clumsy; for one, I'd have to add an explicit list of each possible event:

fn events() -> Vec<LifecycleEvent> {
  vec![
    LifecycleEvent::Test,
    LifecycleEvent::StartUp(When::Before),
    LifecycleEvent::StartUp(When::After),
    // ...
  ]
}

With that approach, when I add a new lifecycle event, nothing will tell me to add the new event to the list, and I also have to make sure to include every possible variant.

[Gabriella439]

Wouldn't that be an issue even for the existing approach?

[evanrelf]

supported_when (the function you highlighted) matches on a value of type LifecycleEvent. If you added another event, the pattern match would become non-exhaustive.

[9999years]

And I have a proc macro that generates a function which iterates over all the enum variants, which I can only use if they don't contain data.

[Gabriella439]

it is true that in that one case you might lose exhaustiveness, but on the balance I feel like you gain better safety/exhaustiveness by making invalid states unrepresentable

[9999years]

Consider that the impetus of this system is that I actually already forgot to fill in new entries in the old lifecycle hooks list! This system prevents that. If there's a way to implement this data type that makes invalid states unrepresentable and doesn't require me to explicitly list all the valid states, I'll gladly implement it.

[Gabriella439]

Can you clarify which list you're referring to? I understand what you're asking for in the abstract but couldn't place where in each pull request this corresponds to

[evanrelf]

Suggested change

	fn supported_when(&self) -> Vec<When> {
	fn supported_when(&self) -> HashSet<When> {

[evanrelf]

Suggested change

	fn supported_kind(&self, when: When) -> Vec<CommandKind> {
	fn supported_kind(&self, when: When) -> HashSet<CommandKind> {

[evanrelf]

Is the C2 necessary? C isn't constrained in any way, so I think it would always unify with whatever the type of command is.

[9999years]

Yeah, because this lets you change the generic type (i.e. it lets you go from a Hook<CommandKind> to a Hook<Command>). That's also why I can't use the ..self record update syntax here — the types need to match exactly.

[evanrelf]

This is 100% overkill but

You could replace HashSet<T> (where T is When or CommandKind) with FlagSet<T> from the flagset crate (nicer alternative to bitflags if you're familiar with that).

In this case it's so unnecessary, but the flagset crate is actually really nice, so I recommend considering it for future stuff.

[github-actions]

A PR to release these changes has been created, bumping the version from 0.5.1 to 0.5.2.