Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
WhyNotHugo opened this issue Feb 1, 2023 · 8 comments · May be fixed by #595
Open

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

WhyNotHugo opened this issue Feb 1, 2023 · 8 comments · May be fixed by #595

Comments

@WhyNotHugo
Copy link
Contributor

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
Copy link
Member

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

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Feb 2, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

rust-lang/rust#92521

@WhyNotHugo
Copy link
Contributor Author

I guess I'll try the InlineExtension variant then.

WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Mar 27, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
@WhyNotHugo WhyNotHugo linked a pull request Mar 27, 2023 that will close this issue
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Mar 27, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Mar 27, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Mar 28, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Mar 28, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Jun 29, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Jun 29, 2023
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Feb 21, 2024
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
WhyNotHugo added a commit to WhyNotHugo/http that referenced this issue Dec 18, 2024
Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
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 a pull request may close this issue.

2 participants