http icon indicating copy to clipboard operation
http copied to clipboard

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

Open WhyNotHugo opened this issue 2 years ago • 8 comments

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).

WhyNotHugo avatar Feb 01 '23 18:02 WhyNotHugo

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

seanmonstar avatar Feb 01 '23 18:02 seanmonstar

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 avatar Feb 02 '23 12:02 WhyNotHugo

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:

  • Use Bytes type for Method and merge those two variants. This should be quite efficient for const instances.
  • Create a new const constructor for method that can only create methods up to InlineExtension::MAX in length.

WhyNotHugo avatar Mar 20 '23 19:03 WhyNotHugo

@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.

WhyNotHugo avatar Mar 21 '23 17:03 WhyNotHugo

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.

seanmonstar avatar Mar 24 '23 18:03 seanmonstar

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 avatar Mar 27 '23 16:03 WhyNotHugo

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 avatar Mar 27 '23 16:03 WhyNotHugo

I guess I'll try the InlineExtension variant then.

WhyNotHugo avatar Mar 27 '23 17:03 WhyNotHugo