🐞 [Bug]: Explicit lifetime annotations are lost

[jkneer]

What happened?

impl and functions that are annotated by the state-shift macros lose the definitions of the explicit lifetimes.

Expected behavior

Well lifetime definitions should be preserved.

[ozgunozerk]

Great! If you have an example code to share which is not working, would be much convenient for me the address the problem. If you want to contribute and solve this, you are more than welcome as well!

Let me know :)

[jkneer]

I have a use case for the typestate pattern outside of builders - managing hardware devices. If we take your complex example the equivalent to my case would be the PlayerBuilder having a lifetime:

#[type_state(state_slots = 3, default_state = Initial)]
struct PlayerBuilder<'forgiggles> {
    race: Option<Race>,
    level: Option<u8>,
    skill_slots: Option<u8>,
    spell_slots: Option<u8>,
    _lifetime: PhantomData<&'forgiggles ()>,
}

The type_state macro will strip the struct of that explicit lifetime. I haven't thought about the implications, but I think just conserving the lifetime is all it needs.

I'm sorry, I found this while playing with the pattern during a refactor. I would prefer not to delve into the depths of this crate :), though I'd be very happy to support the testing.

[ozgunozerk]

Thanks a lot! Yeah no worries I can fix it, already grateful to you for the issue report and the example code! You can track the updates from this issue 🚀

[jkneer]

Just tried the pattern in another section, where I would only need to hand a struct with a lifetime to the builders constructer. That also does not work, because it requires the Builder itself to define the lifetime, and that is currently not possible.

#[type_state(state_slots = 1, default_state = Initial)]
struct PlayerBuilder<'forgiggles> {
    myobject: MyObject<&'forgiggles ()>,
}

impl<'a> PlayerBuilder<'a> {
    #[require(Initial)]
    #[switch_to()...]
    fn new (obj: MyObject<'a>) {
        ...
    }
}

[ozgunozerk]

@jkneer, I just had the time to take a look at this.

I'll note my thought process here to not forget. And if you or anybody else have some better suggestions, I can take them into account while implementing the solution.

Problems:

• a struct may have multiple lifetimes

  struct Foo<'a, 'b, State> {
    value_a: &'a str,
    value_b: Option<&'b str>,
    _state: PhantomData<State>,
  }

• a method of this struct may not deal with all the lifetimes of the struct, and can concern itself with only a subset of the lifetimes associated with the struct (since the fields of the struct will be wrapped in Option due to type-state-pattern

  
  #[require(Initial)]
  #[switch_to(ValueB)]
  fn new(value_a: &'a str) -> Self {
      Foo {
          value_a: value_a,
          value_b: None,
          _state: PhantomData,
      }
  }

• another note on the methods: it is reasonable to expect the user of this library to be consistent with the lifetime annotations between the struct and its associated methods. For example, while the above code snippet is ok, the below is not:

  #[require(Initial)]
  #[switch_to(ValueB)]
  fn new(value_a: &'b str) -> Self { // NOTICE HERE: lifetime `'b` is associated with `value_b`. It is user's responsibility to use the correct lifetime
      Foo {
          value_a: value_a,
          value_b: None,
          _state: PhantomData,
      }
  }

  because it is implied that this method will be placed inside of an impl block with the necessary lifetime annotations, and it is only reasonable to expect this library to deduce the required lifetimes by matching the letters between the struct and the method.

So, for the solution, I have laid out the below:

1. #[states] macro should keep a track of how many lifetimes are present in the impl block (written by the user)
   • trivial to do
2. #[states] macro should relay this information to #[require] macro (it already relay's the struct's name to #[require] macro
   • trivial to do

3. #[require] macro should parse the function's parameters' lifetimes, and match them with the provided lifetime list by #[states] macro. The unmatched/unused lifetimes from the list will be translated into '_ for unused lifetimes to not enforce unnecessary lifetime boundaries.

   • an example would be good:

     impl<'a, 'b> Foo<'a, 'b> {
     #[require(Initial)]
     #[switch_to(ValueB)]
     fn new(value_a: &'a str) -> Self {
       Foo {
           value_a: value_a,
           value_b: None,
           _state: PhantomData,
       }
     }
     }

   this will turn into (eliminating the changes that are not related to lifetimes):

   impl<'a> Foo<'a, '_, Initialized> { // NOTICE `'b` is unused for this method, so the `impl` block replaces `'b` with `'_`
     fn new(value: &'a str) -> Self {
         Foo {
             value_a: value,
             value_b: None,
             _state: PhantomData,
         }
     }
   }

Hope it all makes sense, I will start implementing the solution soon (but probably not today)

[jkneer]

Generally sounds like the right thing to do.

I'm a bit worried about "it is reasonable to expect the user of this library to be consistent with the lifetime annotations between the struct and its associated methods." I agree that it is reasonable, but it is an additional requirement on the lifetime names, that is kind of arbitrary to the uninformed user of the lib. This has the potential to waste a lot of hours for users. Will the user get meaningfull error messages, or can it only be "fixed" by documentation? Yet I have no better idea on how to handle this.

[ozgunozerk]

I'm a bit worried about "it is reasonable to expect the user of this library to be consistent with the lifetime annotations between the struct and its associated methods." I agree that it is reasonable, but it is an additional requirement on the lifetime names, that is kind of arbitrary to the uninformed user of the lib. This has the potential to waste a lot of hours for users. Will the user get meaningfull error messages, or can it only be "fixed" by documentation? Yet I have no better idea on how to handle this.

Totally agreed, will search for additional ways to provide meaningful error messages if possible. Otherwise it seems like documentation is our only bet

[ozgunozerk]

@jkneer great news, I think I implemented the requested feature and now it works.

If you have time and still willing to test, could you please use the generic-support branch? You probably know this but you can easily select the branch of this crate in your cargo toml file using the git version.

state-shift = { git = "https://github.com/ozgunozerk/state-shift.git", branch = "generic-support" }

should be something like that.

Please let me know if there are any errors or potential improvements. Planning to open a PR and merge this feature soon

[jkneer]

Hi,

The lints on the lifetime seem to be gone, but two new things popped up:

• where clauses also seem to be lost. Fix is probably very similar to the lifetime fix.
• I have a 'type Result = std::result::Result<T, MyError>' alias. On methods marked with switch_to these get linted with type alias takes 1 generic argument but 2 generic arguments were supplied expected 1 generic argument. Looking at the expanded code it seems like the state is simply added as a generic to the Result; I think this would always break when a generic type is returned. The return probably needs to be wrapped instead of just expanding it?

Those two issues seem to be seperate issues and I would advise to move forward with this PR and create new issues for the return type and the where clause.

[ozgunozerk]

This is unexpected. I've tested where clauses in both the struct and the impl block. See this test file, which includes:

• lifetime
• generic
• where clause

and it compiles fine: https://github.com/ozgunozerk/state-shift/blob/generic-support/tests/lifetime_example.rs.

Reproducible example would be much appreciated if you can share the broken case and the code.

[jkneer]

Hi,

Ah I jumped right into my non-buildpattern application. Unfortunately that is in a proprietary FFI hardware driver wrapper... that I cannot share. It was a bug in my code where the lints were less than helpful... so that seems to work. I've prepared an example of that I wanted to achieve. I'll try to get the PR for that ready this afternoon.

There is another bug though. Visibility is lost #9 .

I put the exampe code (see PR #10) inside a module, to show case the problem mentioned in #9 also.

[ozgunozerk]

Wonderful! Let's continue from #9 as the next step 🚀