hyperium / http

Rust HTTP types
Apache License 2.0
1.12k stars 283 forks source link

A `method!` macro to define methods at build time #587

Open WhyNotHugo opened 1 year ago

WhyNotHugo commented 1 year ago

Using HTTP verbs / methods that are not defined in this crate has very ugly semantics, e.g.:

Method::from_bytes(b"PROPFIND").expect("PROPFIND is a valid method")

This expect adds a lot of noise all over the place, but it also can't be defined as a const. The following in invalid:

const PROPFIND: Method = Method::from_bytes(b"PROPFIND")
  .expect("PROPFIND is a valid method")

At the same time, these verbs are something that can be validated at build time (rather than runtime). Do you think a macro to create methods would be reasonable? I'm thinking something along the lines of:

method!("PROPFIND");

It would simply be replaced by the correct method, but a proc_macro could allow validating that the string is an acceptable verb at build time (hence, avoiding having a Result for an operation that is known to be infallible).

seanmonstar commented 1 year ago

I imagine a from_static could just be made a const fn, like done for headers.

WhyNotHugo commented 1 year ago

I tried making it const fn. I also had to make functions that it calls const fn and replace some ? with match {...}.

Finally, I reached AllocatedExtension::new, where I'm not sure how to continue:

diff --git a/src/method.rs b/src/method.rs
index b7b3b35..a795208 100644
--- a/src/method.rs
+++ b/src/method.rs
@@ -97,7 +96,7 @@ impl Method {
     pub const TRACE: Method = Method(Trace);

     /// Converts a slice of bytes to an HTTP method.
-    pub fn from_bytes(src: &[u8]) -> Result<Method, InvalidMethod> {
+    pub const fn from_bytes(src: &[u8]) -> Result<Method, InvalidMethod> {
         match src.len() {
             0 => Err(InvalidMethod::new()),
             3 => match src {
@@ -128,7 +127,10 @@ impl Method {
                 if src.len() < InlineExtension::MAX {
                     Method::extension_inline(src)
                 } else {
-                    let allocated = AllocatedExtension::new(src)?;
+                    let allocated = match AllocatedExtension::new(src) {
+                        Ok(a) => a,
+                        Err(e) => return Err(e),
+                    };

                     Ok(Method(ExtensionAllocated(allocated)))
                 }
@@ -136,8 +138,11 @@ impl Method {
         }
     }

-    fn extension_inline(src: &[u8]) -> Result<Method, InvalidMethod> {
-        let inline = InlineExtension::new(src)?;
+    const fn extension_inline(src: &[u8]) -> Result<Method, InvalidMethod> {
+        let inline = match InlineExtension::new(src) {
+            Ok(i) => i,
+            Err(err) => return Err(err),
+        };

         Ok(Method(ExtensionInline(inline)))
     }
@@ -288,7 +293,7 @@ impl FromStr for Method {
 }

 impl InvalidMethod {
-    fn new() -> InvalidMethod {
+    const fn new() -> InvalidMethod {
         InvalidMethod { _priv: () }
     }
 }
@@ -325,10 +330,12 @@ mod extension {
         // Method::from_bytes() assumes this is at least 7
         pub const MAX: usize = 15;

-        pub fn new(src: &[u8]) -> Result<InlineExtension, InvalidMethod> {
-            let mut data: [u8; InlineExtension::MAX] = Default::default();
+        pub const fn new(src: &[u8]) -> Result<InlineExtension, InvalidMethod> {
+            let mut data: [u8; InlineExtension::MAX] = [0; 15];

-            write_checked(src, &mut data)?;
+            if let Err(err) = write_checked(src, &mut data) {
+                return Err(err);
+            };

             // Invariant: write_checked ensures that the first src.len() bytes
             // of data are valid UTF-8.
@@ -339,15 +346,17 @@ mod extension {
             let InlineExtension(ref data, len) = self;
             // Safety: the invariant of InlineExtension ensures that the first
             // len bytes of data contain valid UTF-8.
-            unsafe {str::from_utf8_unchecked(&data[..*len as usize])}
+            unsafe { str::from_utf8_unchecked(&data[..*len as usize]) }
         }
     }

     impl AllocatedExtension {
-        pub fn new(src: &[u8]) -> Result<AllocatedExtension, InvalidMethod> {
+        pub const fn new(src: &[u8]) -> Result<AllocatedExtension, InvalidMethod> {
             let mut data: Vec<u8> = vec![0; src.len()];

-            write_checked(src, &mut data)?;
+            if let Err(err) = write_checked(src, &mut data) {
+                return Err(err);
+            };

             // Invariant: data is exactly src.len() long and write_checked
             // ensures that the first src.len() bytes of data are valid UTF-8.

error[E0277]: the trait bound `Vec<u8>: DerefMut` is not satisfied
   --> src/method.rs:357:50
    |
357 |             if let Err(err) = write_checked(src, &mut data) {
    |                                                  ^^^^^^^^^ the trait `~const DerefMut` is not implemented for `Vec<u8>`
    |
note: the trait `DerefMut` is implemented for `Vec<u8>`, but that implementation is not `const`
   --> src/method.rs:357:50
    |
357 |             if let Err(err) = write_checked(src, &mut data) {
    |                                                  ^^^^^^^^^
``
WhyNotHugo commented 1 year ago

Okay, my previous approach doesn't seem to be feasible.

HeaderValue uses the Bytes type under the hood, which might be a good choice here too. It has an overhead of 4usize per instance. Given that methods are usually 3-8 characters, the overhead sounds like a lot, but in the const use case, they'll usually point to the same 'static str, so at least there wouldn't be heap allocations.

Current Method has two variants: one that holds the data inline, and another that holds a heap allocated string.

I see two possible options:

WhyNotHugo commented 1 year ago

@seanmonstar Any feedback on the above comment? I want to give this a shot, but both approaches are non-trivial so I'd rather hear from you before sinking time into the wrong one.

seanmonstar commented 1 year ago

I think keeping Method small is worthwhile. And with custom methods being so uncommon, I think it makes sense to double box the weird ones, so we only pay a single pointer in the struct.

WhyNotHugo commented 1 year ago

And with custom methods being so uncommon

I'm not using custom methods; I'm using standardized methods like PROPFIND and REPORT, which are part of rfc4791 (webdav).

Whether they're uncommon depends on your field of work. Personally I find TRACE to be much more niche (I've never seen it used in the wild).

I think it makes sense to double box the weird ones, so we only pay a single pointer in the struct.

You mean the Method::ExtensionAllocated variant, right? I'll give this a try, I'm pretty sure that I have all the required bits in sight.

WhyNotHugo commented 1 year ago

Darn, I missed that const cannot be used in a const fn. Or rather, it's still unstable:

https://github.com/rust-lang/rust/issues/92521

WhyNotHugo commented 1 year ago

I guess I'll try the InlineExtension variant then.