Skip to content

Introduce type-based linking of requests and replies#86

Merged
nickray merged 3 commits into
mainfrom
reply-request-bijection
Feb 5, 2023
Merged

Introduce type-based linking of requests and replies#86
nickray merged 3 commits into
mainfrom
reply-request-bijection

Conversation

@sosthene-nitrokey
Copy link
Copy Markdown
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Jan 31, 2023

This ensures that PollClient::request always matches the correct Request and Reply

This patch also fixes #84 and replaces the From that used unreachable_unchecked with TryFrom.

The main additions are the traits:

pub trait RequestData: Into<Request> + TryFrom<Request, Error = crate::Error> {
    type CorrespondingReply: ReplyData<CorrespondingRequest = Self>;
}

pub trait ReplyData: Into<Reply> + TryFrom<Reply, Error = crate::Error> {
    type CorrespondingRequest: RequestData<CorrespondingReply = Self>;
}

And PollClient's request becomes

// old
fn request<T: From<Reply>>(&mut self, req: impl Into<Request>) -> ClientResult<'_, T, Self>;
 // new
fn request<Rq: RequestData>(
      &mut self,
      req: Rq,
) -> ClientResult<'_, Rq::CorrespondingReply, Self>;

This also showed that the reply type for the RemoveDir was incorrect (it returned RemoveDirAll) which actually triggered UB is mentionned in #84

This ensures that `PollClient::request` always matches the correct Request and Reply

This patch also fixes #84 and replaces the
`From` that used `unreachable_unchecked` with `TryFrom`.
@sosthene-nitrokey
Copy link
Copy Markdown
Contributor Author

I wonder if this checking could also be used on the backend side.

@robin-nitrokey
Copy link
Copy Markdown
Member

What do you think about dropping the Corresponding prefix? Would be a bit easier to read.

@sosthene-nitrokey
Copy link
Copy Markdown
Contributor Author

sosthene-nitrokey commented Jan 31, 2023

It gets confusing to have multiple types named Request and Reply, but I agree it is a bit verbose. I initially added it because as I was dealing with the errors that came with the refactor it was confusing.

I removed it in 79ccc04

@robin-nitrokey
Copy link
Copy Markdown
Member

I generally agree, but for associated types I think re-using existing names is fine (e. g. TryFrom::Error).

@nickray
Copy link
Copy Markdown
Member

nickray commented Feb 1, 2023

This is super cool, it always bugged me that these types were not matched on a type level.

Comment thread src/api.rs Outdated
@nickray nickray merged commit 3a847d5 into main Feb 5, 2023
@nickray nickray deleted the reply-request-bijection branch February 5, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dangerous unsafe in impl_reply

3 participants