Refactor the macro code

[jplatte]

In ruma/ruma-api#34, I once again discovered that the macro code could use some refactoring. I will try to work on this soon-ish, although if somebody else wants to look into this, I'd happily mentor that person as well.

[jplatte]

Set the effort/hard label, because this requires a non-trivial amount of knowledge about proc macros.

[jplatte]

One thing I just recalled again I would potentially like to see in this area is refactor the request / response deserialization functions to contain an inner function where ? can be used for error propagation, where the outer function then adds the raw http request / response in the error case (having to add that is why there's currently lots of explicit matches with Err(e) => return Err(…) in the generated code).

[DevinR528]

This is about this and the few other times it's generated for the Request struct?

[jplatte]

@DevinR528 yes, my latest comment was about that.

[DevinR528]

What was the function signature you had in mind? Or maybe the better question is how to handle passing the request/response variable around owned in order to create the error (I'm assuming this relates to "where the outer function then adds the raw http request / response in the error case"). Is the goal is to have an inner function that does the matching but somehow hands the error creation back to the TryFrom::try_from method?

[jplatte]

Without relating this closely to the code (would have to look it up again), this is what I'm thinking of:

fn try_from(...) -> Result<T, OuterError> {
    let try_from_impl = move || {
        fallible_operation_1()?;
        fallible_operation_2()?;
        fallible_operation_3()
    };

    try_from_impl().map_err(|e| OuterError {
        msg: e,
        context: ...,
    })
}

Does that make sense or is it too abstract? 😅

[DevinR528]

I think the example was good enough. I was trying it out in the expanded code and this is what I came up with.

Ok(Self {
    room_id: {
        let segment = path_segments.get(4usize).unwrap().as_bytes();
        let try_from_impl =
            move || -> Result<_, ruma_api::error::DeserializationError> {
                let decoded =
                    ruma_api::exports::percent_encoding::percent_decode(segment)
                        .decode_utf8()
                        .map_err(ruma_api::error::DeserializationError::from)?;

                std::convert::TryFrom::try_from(decoded.deref()).map_err(Into::into)
            };
        match try_from_impl() {
            Ok(val) => val,
            Err(err) => {
                return Err(RequestDeserializationError::new(err, request).into())
            }
        }
    },
    event_id: {
        let segment = path_segments.get(6usize).unwrap().as_bytes();
        let try_from_impl =
            move || -> Result<_, ruma_api::error::DeserializationError> {
                let decoded =
                    ruma_api::exports::percent_encoding::percent_decode(segment)
                        .decode_utf8()
                        // Into::into needs type anotations here
                        .map_err(ruma_api::error::DeserializationError::from)?;

                std::convert::TryFrom::try_from(decoded.deref()).map_err(Into::into)
            };
        // this is the only field that can use `.map_err` in this example since it's the last
        try_from_impl().map_err(|err| RequestDeserializationError::new(err, request))?
    },
})

All but the last field has to use match and return or compiler complains about request being moved. This is an improvement over what's there currently.

[jplatte]

Why one closure per request field? Is it not possible to have just one try_from_impl for the whole TryFrom::try_from?

[jplatte]

I don't care much whether it's map_err or needs to be match for borrow-checking reasons.

[DevinR528]

Why one closure per request field? Is it not possible to have just one try_from_impl for the whole TryFrom::try_from?

Type inference sets the type of the closure I'm guessing as the event_id field try_from_impl fails

error[E0308]: try expression alternatives have incompatible types
   --> ruma-client-api/src/zzz.rs:111:21
    |
111 | /                     try_from_impl(segment)
112 | |                         .map_err(|err| RequestDeserializationError::new(err, request))?
    | |^ expected struct `ruma_identifiers::event_id::EventId`, found struct `ruma_identifiers::room_id::RoomId`
    |
    = note: expected struct `ruma_identifiers::event_id::EventId<std::boxed::Box<str>>`
               found struct `ruma_identifiers::room_id::RoomId<std::boxed::Box<str>>`

[DevinR528]

That was from this code

let try_from_impl = move |segment| -> Result<_, ruma_api::error::DeserializationError> {
    let decoded = ruma_api::exports::percent_encoding::percent_decode(segment)
        .decode_utf8()
        .map_err(ruma_api::error::DeserializationError::from)?;

    std::convert::TryFrom::try_from(decoded.deref()).map_err(Into::into)
};

Ok(Self {
    room_id: {
        let segment = path_segments.get(4usize).unwrap().as_bytes();
        match try_from_impl(segment) {
            Ok(val) => val,
            Err(err) => {
                return Err(RequestDeserializationError::new(err, request).into())
            }
        }
    },
    event_id: {
        let segment = path_segments.get(6usize).unwrap().as_bytes();
        try_from_impl(segment)
            .map_err(|err| RequestDeserializationError::new(err, request))?
    },
})

[jplatte]

@DevinR528 You're still misunderstanding something. I want the closure to return Result<Self, DeserializationError>. The whole

#extract_request_path
#extract_request_query
#extract_request_headers
#extract_request_body

Ok(Self {
    #parse_request_path
    #parse_request_query
    #parse_request_headers
    #parse_request_body
})

should be moved into the closure, with the remainder of the try_from implementation being a call to the closure + map_err. Does that make sense? Did you try this before?

[DevinR528]

It works! Now I just gotta make the code-gen changes and open the PR.

[jplatte]

Since there are no more specific refactoring suggestions in here that haven't been implemented or found impossibe, I'll close this.