Various refactorings (#2281)

Co-authored-by: Rob Ede <robjtede@icloud.com>

.gitignore

@@ -16,3 +16,6 @@ guide/build/
 
 # Configuration directory generated by CLion
 .idea
+
+# Configuration directory generated by VSCode
+.vscode

CHANGES.md

@@ -12,6 +12,7 @@
 * Deprecate `App::data` and `App::data_factory`. [#2271]
 * Smarter extraction of `ConnectionInfo` parts. [#2282]
 
+
 ### Fixed
 * Scope and Resource middleware can access data items set on their own layer. [#2288]
 

actix-files/src/named.rs

@@ -355,8 +355,8 @@ impl NamedFile {
         } else if let (Some(ref m), Some(header::IfUnmodifiedSince(ref since))) =
             (last_modified, req.get_header())
         {
-            let t1: SystemTime = m.clone().into();
+            let t1: SystemTime = (*m).into();
-            let t2: SystemTime = since.clone().into();
+            let t2: SystemTime = (*since).into();
 
             match (t1.duration_since(UNIX_EPOCH), t2.duration_since(UNIX_EPOCH)) {
                 (Ok(t1), Ok(t2)) => t1.as_secs() > t2.as_secs(),
@@ -374,8 +374,8 @@ impl NamedFile {
         } else if let (Some(ref m), Some(header::IfModifiedSince(ref since))) =
             (last_modified, req.get_header())
         {
-            let t1: SystemTime = m.clone().into();
+            let t1: SystemTime = (*m).into();
-            let t2: SystemTime = since.clone().into();
+            let t2: SystemTime = (*since).into();
 
             match (t1.duration_since(UNIX_EPOCH), t2.duration_since(UNIX_EPOCH)) {
                 (Ok(t1), Ok(t2)) => t1.as_secs() <= t2.as_secs(),

actix-http/benches/uninit-headers.rs

@@ -78,12 +78,12 @@ impl HeaderIndex {
 // test cases taken from:
 // https://github.com/seanmonstar/httparse/blob/master/benches/parse.rs
 
-const REQ_SHORT: &'static [u8] = b"\
+const REQ_SHORT: &[u8] = b"\
 GET / HTTP/1.0\r\n\
 Host: example.com\r\n\
 Cookie: session=60; user_id=1\r\n\r\n";
 
-const REQ: &'static [u8] = b"\
+const REQ: &[u8] = b"\
 GET /wp-content/uploads/2010/03/hello-kitty-darth-vader-pink.jpg HTTP/1.1\r\n\
 Host: www.kittyhell.com\r\n\
 User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 Pathtraq/0.9\r\n\
@@ -119,6 +119,8 @@ mod _original {
     use std::mem::MaybeUninit;
 
     pub fn parse_headers(src: &mut BytesMut) -> usize {
+        #![allow(clippy::uninit_assumed_init)]
+
         let mut headers: [HeaderIndex; MAX_HEADERS] =
             unsafe { MaybeUninit::uninit().assume_init() };
 

actix-http/src/client/connector.rs

@@ -85,7 +85,7 @@ impl Connector<()> {
         use bytes::{BufMut, BytesMut};
 
         let mut alpn = BytesMut::with_capacity(20);
-        for proto in protocols.iter() {
+        for proto in &protocols {
             alpn.put_u8(proto.len() as u8);
             alpn.put(proto.as_slice());
         }
@@ -290,8 +290,7 @@ where
                         let h2 = sock
                             .ssl()
                             .selected_alpn_protocol()
-                            .map(|protos| protos.windows(2).any(|w| w == H2))
+                            .map_or(false, |protos| protos.windows(2).any(|w| w == H2));
-                            .unwrap_or(false);
                         if h2 {
                             (Box::new(sock), Protocol::Http2)
                         } else {
@@ -325,8 +324,7 @@ where
                             .get_ref()
                             .1
                             .get_alpn_protocol()
-                            .map(|protos| protos.windows(2).any(|w| w == H2))
+                            .map_or(false, |protos| protos.windows(2).any(|w| w == H2));
-                            .unwrap_or(false);
                         if h2 {
                             (Box::new(sock), Protocol::Http2)
                         } else {

actix-http/src/client/h2proto.rs

@@ -168,14 +168,13 @@ where
 
                 if let Err(e) = send.send_data(bytes, false) {
                     return Err(e.into());
-                } else {
-                    if !b.is_empty() {
-                        send.reserve_capacity(b.len());
-                    } else {
-                        buf = None;
-                    }
-                    continue;
                 }
+                if !b.is_empty() {
+                    send.reserve_capacity(b.len());
+                } else {
+                    buf = None;
+                }
+                continue;
             }
             Some(Err(e)) => return Err(e.into()),
         }

actix-http/src/config.rs

@@ -152,8 +152,8 @@ impl ServiceConfig {
         }
     }
 
-    #[inline]
     /// Return keep-alive timer delay is configured.
+    #[inline]
     pub fn keep_alive_timer(&self) -> Option<Sleep> {
         self.keep_alive().map(|ka| sleep_until(self.now() + ka))
     }
@@ -365,11 +365,11 @@ mod tests {
         let clone3 = service.clone();
 
         drop(clone1);
-        assert_eq!(false, notify_on_drop::is_dropped());
+        assert!(!notify_on_drop::is_dropped());
         drop(clone2);
-        assert_eq!(false, notify_on_drop::is_dropped());
+        assert!(!notify_on_drop::is_dropped());
         drop(clone3);
-        assert_eq!(false, notify_on_drop::is_dropped());
+        assert!(!notify_on_drop::is_dropped());
 
         drop(service);
         assert!(notify_on_drop::is_dropped());

actix-http/src/error.rs

@@ -125,7 +125,7 @@ impl fmt::Display for Error {
 
 impl StdError for Error {
     fn source(&self) -> Option<&(dyn StdError + 'static)> {
-        self.inner.cause.as_ref().map(|err| err.as_ref())
+        self.inner.cause.as_ref().map(Box::as_ref)
     }
 }
 

actix-http/src/h1/decoder.rs

@@ -102,7 +102,7 @@ pub(crate) trait MessageType: Sized {
                     }
                     // transfer-encoding
                     header::TRANSFER_ENCODING => {
-                        if let Ok(s) = value.to_str().map(|s| s.trim()) {
+                        if let Ok(s) = value.to_str().map(str::trim) {
                             chunked = s.eq_ignore_ascii_case("chunked");
                         } else {
                             return Err(ParseError::Header);
@@ -110,7 +110,7 @@ pub(crate) trait MessageType: Sized {
                     }
                     // connection keep-alive state
                     header::CONNECTION => {
-                        ka = if let Ok(conn) = value.to_str().map(|conn| conn.trim()) {
+                        ka = if let Ok(conn) = value.to_str().map(str::trim) {
                             if conn.eq_ignore_ascii_case("keep-alive") {
                                 Some(ConnectionType::KeepAlive)
                             } else if conn.eq_ignore_ascii_case("close") {
@@ -125,7 +125,7 @@ pub(crate) trait MessageType: Sized {
                         };
                     }
                     header::UPGRADE => {
-                        if let Ok(val) = value.to_str().map(|val| val.trim()) {
+                        if let Ok(val) = value.to_str().map(str::trim) {
                             if val.eq_ignore_ascii_case("websocket") {
                                 has_upgrade_websocket = true;
                             }

actix-http/src/h1/dispatcher.rs

@@ -515,14 +515,13 @@ where
         cx: &mut Context<'_>,
     ) -> Result<(), DispatchError> {
         // Handle `EXPECT: 100-Continue` header
+        let mut this = self.as_mut().project();
         if req.head().expect() {
             // set dispatcher state so the future is pinned.
-            let mut this = self.as_mut().project();
             let task = this.flow.expect.call(req);
             this.state.set(State::ExpectCall(task));
         } else {
             // the same as above.
-            let mut this = self.as_mut().project();
             let task = this.flow.service.call(req);
             this.state.set(State::ServiceCall(task));
         };

actix-http/src/h1/payload.rs

@@ -186,8 +186,7 @@ impl Inner {
         if self
             .task
             .as_ref()
-            .map(|w| !cx.waker().will_wake(w))
+            .map_or(true, |w| !cx.waker().will_wake(w))
-            .unwrap_or(true)
         {
             self.task = Some(cx.waker().clone());
         }
@@ -199,8 +198,7 @@ impl Inner {
         if self
             .io_task
             .as_ref()
-            .map(|w| !cx.waker().will_wake(w))
+            .map_or(true, |w| !cx.waker().will_wake(w))
-            .unwrap_or(true)
         {
             self.io_task = Some(cx.waker().clone());
         }

actix-http/src/header/map.rs

@@ -249,7 +249,7 @@ impl HeaderMap {
     /// assert!(map.get("INVALID HEADER NAME").is_none());
     /// ```
     pub fn get(&self, key: impl AsHeaderName) -> Option<&HeaderValue> {
-        self.get_value(key).map(|val| val.first())
+        self.get_value(key).map(Value::first)
     }
 
     /// Returns a mutable reference to the _first_ value associated a header name.
@@ -280,8 +280,8 @@ impl HeaderMap {
     /// ```
     pub fn get_mut(&mut self, key: impl AsHeaderName) -> Option<&mut HeaderValue> {
         match key.try_as_name(super::as_name::Seal).ok()? {
-            Cow::Borrowed(name) => self.inner.get_mut(name).map(|v| v.first_mut()),
+            Cow::Borrowed(name) => self.inner.get_mut(name).map(Value::first_mut),
-            Cow::Owned(name) => self.inner.get_mut(&name).map(|v| v.first_mut()),
+            Cow::Owned(name) => self.inner.get_mut(&name).map(Value::first_mut),
         }
     }
 

actix-http/src/message.rs

@@ -152,15 +152,16 @@ impl RequestHead {
 
     /// Connection upgrade status
     pub fn upgrade(&self) -> bool {
-        if let Some(hdr) = self.headers().get(header::CONNECTION) {
+        self.headers()
-            if let Ok(s) = hdr.to_str() {
+            .get(header::CONNECTION)
-                s.to_ascii_lowercase().contains("upgrade")
+            .map(|hdr| {
-            } else {
+                if let Ok(s) = hdr.to_str() {
-                false
+                    s.to_ascii_lowercase().contains("upgrade")
-            }
+                } else {
-        } else {
+                    false
-            false
+                }
-        }
+            })
+            .unwrap_or(false)
     }
 
     #[inline]
@@ -308,13 +309,11 @@ impl ResponseHead {
     /// Get custom reason for the response
     #[inline]
     pub fn reason(&self) -> &str {
-        if let Some(reason) = self.reason {
+        self.reason.unwrap_or_else(|| {
-            reason
-        } else {
             self.status
                 .canonical_reason()
                 .unwrap_or("<unknown status code>")
-        }
+        })
     }
 
     #[inline]
@@ -356,7 +355,7 @@ pub struct Message<T: Head> {
 impl<T: Head> Message<T> {
     /// Get new message from the pool of objects
     pub fn new() -> Self {
-        T::with_pool(|p| p.get_message())
+        T::with_pool(MessagePool::get_message)
     }
 }
 

actix-web-codegen/src/route.rs

@@ -6,7 +6,7 @@ use std::convert::TryFrom;
 use proc_macro::TokenStream;
 use proc_macro2::{Span, TokenStream as TokenStream2};
 use quote::{format_ident, quote, ToTokens, TokenStreamExt};
-use syn::{parse_macro_input, AttributeArgs, Ident, NestedMeta};
+use syn::{parse_macro_input, AttributeArgs, Ident, LitStr, NestedMeta};
 
 enum ResourceType {
     Async,
@@ -227,8 +227,7 @@ impl Route {
                 format!(
                     r#"invalid service definition, expected #[{}("<some path>")]"#,
                     method
-                        .map(|it| it.as_str())
+                        .map_or("route", |it| it.as_str())
-                        .unwrap_or("route")
                         .to_ascii_lowercase()
                 ),
             ));
@@ -298,7 +297,7 @@ impl ToTokens for Route {
         } = self;
         let resource_name = resource_name
             .as_ref()
-            .map_or_else(|| name.to_string(), |n| n.value());
+            .map_or_else(|| name.to_string(), LitStr::value);
         let method_guards = {
             let mut others = methods.iter();
             // unwrapping since length is checked to be at least one

awc/src/request.rs

@@ -486,7 +486,9 @@ impl ClientRequest {
                 let mut encoding = vec![];
 
                 #[cfg(feature = "compress-brotli")]
-                encoding.push("br");
+                {
+                    encoding.push("br");
+                }
 
                 #[cfg(feature = "compress-gzip")]
                 {

awc/src/ws.rs

@@ -517,7 +517,7 @@ mod tests {
             "test-origin"
         );
         assert_eq!(req.max_size, 100);
-        assert_eq!(req.server_mode, true);
+        assert!(req.server_mode);
         assert_eq!(req.protocols, Some("v1,v2".to_string()));
         assert_eq!(
             req.head.headers.get(header::CONTENT_TYPE).unwrap(),

benches/responder.rs

@@ -1,6 +1,5 @@
 use std::{future::Future, time::Instant};
 
-use actix_http::Response;
 use actix_utils::future::{ready, Ready};
 use actix_web::http::StatusCode;
 use actix_web::test::TestRequest;
@@ -24,11 +23,11 @@ struct StringResponder(String);
 
 impl FutureResponder for StringResponder {
     type Error = Error;
-    type Future = Ready<Result<Response, Self::Error>>;
+    type Future = Ready<Result<HttpResponse, Self::Error>>;
 
     fn future_respond_to(self, _: &HttpRequest) -> Self::Future {
         // this is default builder for string response in both new and old responder trait.
-        ready(Ok(Response::build(StatusCode::OK)
+        ready(Ok(HttpResponse::build(StatusCode::OK)
             .content_type("text/plain; charset=utf-8")
             .body(self.0)))
     }
@@ -37,7 +36,7 @@ impl FutureResponder for StringResponder {
 impl<T> FutureResponder for OptionResponder<T>
 where
     T: FutureResponder,
-    T::Future: Future<Output = Result<Response, Error>>,
+    T::Future: Future<Output = Result<HttpResponse, Error>>,
 {
     type Error = Error;
     type Future = Either<T::Future, Ready<Result<HttpResponse, Self::Error>>>;
@@ -52,7 +51,7 @@ where
 
 impl Responder for StringResponder {
     fn respond_to(self, _: &HttpRequest) -> HttpResponse {
-        Response::build(StatusCode::OK)
+        HttpResponse::build(StatusCode::OK)
             .content_type("text/plain; charset=utf-8")
             .body(self.0)
     }
@@ -62,7 +61,7 @@ impl<T: Responder> Responder for OptionResponder<T> {
     fn respond_to(self, req: &HttpRequest) -> HttpResponse {
         match self.0 {
             Some(t) => t.respond_to(req),
-            None => Response::from_error(error::ErrorInternalServerError("err")),
+            None => HttpResponse::from_error(error::ErrorInternalServerError("err")),
         }
     }
 }

benches/service.rs

@@ -51,9 +51,8 @@ where
                     fut.await.unwrap();
                 }
             });
-            let elapsed = start.elapsed();
             // check that at least first request succeeded
-            elapsed
+            start.elapsed()
         })
     });
 }
@@ -93,9 +92,8 @@ fn async_web_service(c: &mut Criterion) {
                     fut.await.unwrap();
                 }
             });
-            let elapsed = start.elapsed();
             // check that at least first request succeeded
-            elapsed
+            start.elapsed()
         })
     });
 }

src/app_service.rs

@@ -131,9 +131,9 @@ where
             let service = endpoint_fut.await?;
 
             // populate app data container from (async) data factories.
-            async_data_factories.iter().for_each(|factory| {
+            for factory in &async_data_factories {
                 factory.create(&mut app_data);
-            });
+            }
 
             Ok(AppInitService {
                 service,

src/http/header/content_disposition.rs

@@ -410,41 +410,33 @@ impl ContentDisposition {
 
     /// Return the value of *name* if exists.
     pub fn get_name(&self) -> Option<&str> {
-        self.parameters.iter().filter_map(|p| p.as_name()).next()
+        self.parameters.iter().find_map(DispositionParam::as_name)
     }
 
     /// Return the value of *filename* if exists.
     pub fn get_filename(&self) -> Option<&str> {
         self.parameters
             .iter()
-            .filter_map(|p| p.as_filename())
+            .find_map(DispositionParam::as_filename)
-            .next()
     }
 
     /// Return the value of *filename\** if exists.
     pub fn get_filename_ext(&self) -> Option<&ExtendedValue> {
         self.parameters
             .iter()
-            .filter_map(|p| p.as_filename_ext())
+            .find_map(DispositionParam::as_filename_ext)
-            .next()
     }
 
     /// Return the value of the parameter which the `name` matches.
     pub fn get_unknown(&self, name: impl AsRef<str>) -> Option<&str> {
         let name = name.as_ref();
-        self.parameters
+        self.parameters.iter().find_map(|p| p.as_unknown(name))
-            .iter()
-            .filter_map(|p| p.as_unknown(name))
-            .next()
     }
 
     /// Return the value of the extended parameter which the `name` matches.
     pub fn get_unknown_ext(&self, name: impl AsRef<str>) -> Option<&ExtendedValue> {
         let name = name.as_ref();
-        self.parameters
+        self.parameters.iter().find_map(|p| p.as_unknown_ext(name))
-            .iter()
-            .filter_map(|p| p.as_unknown_ext(name))
-            .next()
     }
 }
 

src/middleware/compress.rs

@@ -200,8 +200,7 @@ impl AcceptEncoding {
         let mut encodings = raw
             .replace(' ', "")
             .split(',')
-            .map(|l| AcceptEncoding::new(l))
+            .filter_map(|l| AcceptEncoding::new(l))
-            .flatten()
             .collect::<Vec<_>>();
 
         encodings.sort();

src/request.rs

@@ -111,11 +111,7 @@ impl HttpRequest {
     /// E.g., id=10
     #[inline]
     pub fn query_string(&self) -> &str {
-        if let Some(query) = self.uri().query().as_ref() {
+        self.uri().query().unwrap_or_default()
-            query
-        } else {
-            ""
-        }
     }
 
     /// Get a reference to the Path parameters.

src/resource.rs

@@ -465,7 +465,7 @@ impl Service<ServiceRequest> for ResourceService {
     actix_service::always_ready!();
 
     fn call(&self, mut req: ServiceRequest) -> Self::Future {
-        for route in self.routes.iter() {
+        for route in &self.routes {
             if route.check(&mut req) {
                 return route.call(req);
             }

src/response/builder.rs

@@ -406,7 +406,7 @@ impl HttpResponseBuilder {
             return None;
         }
 
-        self.res.as_mut().map(|res| res.head_mut())
+        self.res.as_mut().map(Response::head_mut)
     }
 }
 

src/server.rs

@@ -292,15 +292,15 @@ where
                     let c = cfg.lock().unwrap();
                     let host = c.host.clone().unwrap_or_else(|| format!("{}", addr));
 
-                    let svc = HttpService::build()
+                    let mut svc = HttpService::build()
                         .keep_alive(c.keep_alive)
                         .client_timeout(c.client_timeout)
                         .local_addr(addr);
 
-                    let svc = if let Some(handler) = on_connect_fn.clone() {
+                    if let Some(handler) = on_connect_fn.clone() {
-                        svc.on_connect_ext(move |io: &_, ext: _| (handler)(io as &dyn Any, ext))
+                        svc = svc.on_connect_ext(move |io: &_, ext: _| {
-                    } else {
+                            (handler)(io as &dyn Any, ext)
-                        svc
+                        })
                     };
 
                     let fac = factory()
@@ -461,17 +461,15 @@ where
             }
         }
 
-        if !success {
+        if success {
-            if let Some(e) = err.take() {
-                Err(e)
-            } else {
-                Err(io::Error::new(
-                    io::ErrorKind::Other,
-                    "Can not bind to address.",
-                ))
-            }
-        } else {
             Ok(sockets)
+        } else if let Some(e) = err.take() {
+            Err(e)
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::Other,
+                "Can not bind to address.",
+            ))
         }
     }
 
@@ -537,15 +535,14 @@ where
             );
 
             fn_service(|io: UnixStream| async { Ok((io, Protocol::Http1, None)) }).and_then({
-                let svc = HttpService::build()
+                let mut svc = HttpService::build()
                     .keep_alive(c.keep_alive)
                     .client_timeout(c.client_timeout);
 
-                let svc = if let Some(handler) = on_connect_fn.clone() {
+                if let Some(handler) = on_connect_fn.clone() {
-                    svc.on_connect_ext(move |io: &_, ext: _| (&*handler)(io as &dyn Any, ext))
+                    svc = svc
-                } else {
+                        .on_connect_ext(move |io: &_, ext: _| (&*handler)(io as &dyn Any, ext));
-                    svc
+                }
-                };
 
                 let fac = factory()
                     .into_factory()

src/service.rs

@@ -167,11 +167,7 @@ impl ServiceRequest {
     /// E.g., id=10
     #[inline]
     pub fn query_string(&self) -> &str {
-        if let Some(query) = self.uri().query().as_ref() {
+        self.uri().query().unwrap_or_default()
-            query
-        } else {
-            ""
-        }
     }
 
     /// Peer socket address.

src/types/form.rs

@@ -1,6 +1,7 @@
 //! For URL encoded form helper documentation, see [`Form`].
 
 use std::{
+    borrow::Cow,
     fmt,
     future::Future,
     ops,
@@ -384,7 +385,7 @@ where
                 } else {
                     let body = encoding
                         .decode_without_bom_handling_and_without_replacement(&body)
-                        .map(|s| s.into_owned())
+                        .map(Cow::into_owned)
                         .ok_or(UrlencodedError::Encoding)?;
 
                     serde_urlencoded::from_str::<T>(&body).map_err(UrlencodedError::Parse)

src/types/path.rs

@@ -103,8 +103,7 @@ where
     fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future {
         let error_handler = req
             .app_data::<Self::Config>()
-            .map(|c| c.ehandler.clone())
+            .and_then(|c| c.ehandler.clone());
-            .unwrap_or(None);
 
         ready(
             de::Deserialize::deserialize(PathDeserializer::new(req.match_info()))

src/types/payload.rs

@@ -1,6 +1,7 @@
 //! Basic binary and string payload extractors.
 
 use std::{
+    borrow::Cow,
     future::Future,
     pin::Pin,
     str,
@@ -190,7 +191,7 @@ fn bytes_to_string(body: Bytes, encoding: &'static Encoding) -> Result<String, E
     } else {
         Ok(encoding
             .decode_without_bom_handling_and_without_replacement(&body)
-            .map(|s| s.into_owned())
+            .map(Cow::into_owned)
             .ok_or_else(|| ErrorBadRequest("Can not decode body"))?)
     }
 }

src/types/query.rs

@@ -119,8 +119,7 @@ where
     fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future {
         let error_handler = req
             .app_data::<Self::Config>()
-            .map(|c| c.err_handler.clone())
+            .and_then(|c| c.err_handler.clone());
-            .unwrap_or(None);
 
         serde_urlencoded::from_str::<T>(req.query_string())
             .map(|val| ok(Query(val)))