Auto merge of #7224 - Manishearth:clippy, r=Ms2ger

Integrate clippy into Servo; cleanup some of script

The integration is off by default for now. You can try it out with `./mach build --features "script/plugins/clippy"`.

We're using a branch of clippy with some of the lints changed to Allow, either because they don't apply to us, or because they're noisy and dwarf other warnings (but still should be fixed)

After going through the rest of Servo's warnings I'll figure out which lints we should be keeping.

There's a cargo bug with optional deps that makes it hard for this to work with Cargo.lock -- so this PR contains no changes to lockfiles (and running the build with clippy on may dirty the lockfile, though it gets fixed later)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7224)
<!-- Reviewable:end -->

components/devtools_traits/lib.rs

@@ -192,8 +192,8 @@ impl Decodable for Modification {
     fn decode<D: Decoder>(d: &mut D) -> Result<Modification, D::Error> {
         d.read_struct("Modification", 2, |d|
             Ok(Modification {
-                attributeName: try!(d.read_struct_field("attributeName", 0, |d| Decodable::decode(d))),
+                attributeName: try!(d.read_struct_field("attributeName", 0, Decodable::decode)),
-                newValue: match d.read_struct_field("newValue", 1, |d| Decodable::decode(d)) {
+                newValue: match d.read_struct_field("newValue", 1, Decodable::decode) {
                     Ok(opt) => opt,
                     Err(_) => None
                 }

components/plugins/Cargo.toml

@@ -11,3 +11,10 @@ plugin = true
 [dependencies.tenacious]
 git = "https://github.com/servo/rust-tenacious"
 
+[dependencies.clippy]
+git = "https://github.com/Manishearth/rust-clippy"
+branch = "servo"
+optional = true
+
+[features]
+default = []

components/plugins/lib.rs

@@ -21,6 +21,8 @@ extern crate syntax;
 extern crate rustc;
 
 extern crate tenacious;
+#[cfg(feature = "clippy")]
+extern crate clippy;
 
 use rustc::lint::LintPassObject;
 use rustc::plugin::Registry;
@@ -53,10 +55,19 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box lints::unrooted_must_root::UnrootedPass::new() as LintPassObject);
     reg.register_lint_pass(box lints::privatize::PrivatizePass as LintPassObject);
     reg.register_lint_pass(box lints::inheritance_integrity::InheritancePass as LintPassObject);
-    reg.register_lint_pass(box lints::str_to_string::StrToStringPass as LintPassObject);
     reg.register_lint_pass(box lints::ban::BanPass as LintPassObject);
     reg.register_lint_pass(box tenacious::TenaciousPass as LintPassObject);
     reg.register_attribute("must_root".to_string(), Whitelisted);
     reg.register_attribute("servo_lang".to_string(), Whitelisted);
     reg.register_attribute("allow_unrooted_interior".to_string(), Whitelisted);
+    register_clippy(reg);
+}
+
+#[cfg(feature = "clippy")]
+fn register_clippy(reg: &mut Registry) {
+    ::clippy::plugin_registrar(reg);
+}
+#[cfg(not(feature = "clippy"))]
+fn register_clippy(reg: &mut Registry) {
+    reg.register_lint_pass(box lints::str_to_string::StrToStringPass as LintPassObject);
 }

components/script/dom/node.rs

@@ -264,7 +264,7 @@ impl LayoutDataRef {
 
     /// Borrows the layout data immutably. This function is *not* thread-safe.
     #[inline]
-    pub fn borrow<'a>(&'a self) -> Ref<'a, Option<LayoutData>> {
+    pub fn borrow(&self) -> Ref<Option<LayoutData>> {
         debug_assert!(task_state::get().is_layout());
         self.data_cell.borrow()
     }
@@ -275,7 +275,7 @@ impl LayoutDataRef {
     /// prevent CSS selector matching from mutably accessing nodes it's not supposed to and racing
     /// on it. This has already resulted in one bug!
     #[inline]
-    pub fn borrow_mut<'a>(&'a self) -> RefMut<'a, Option<LayoutData>> {
+    pub fn borrow_mut(&self) -> RefMut<Option<LayoutData>> {
         debug_assert!(task_state::get().is_layout());
         self.data_cell.borrow_mut()
     }
@@ -908,7 +908,7 @@ impl<'a> NodeHelpers for &'a Node {
         // Step 1.
         match parse_author_origin_selector_list_from_str(&selectors) {
             // Step 2.
-            Err(()) => return Err(Syntax),
+            Err(()) => Err(Syntax),
             // Step 3.
             Ok(ref selectors) => {
                 let root = self.ancestors().last();
@@ -1326,7 +1326,7 @@ impl Iterator for FollowingNodeIterator {
             }
         }
         self.current = None;
-        return None
+        None
     }
 }
 
@@ -1372,7 +1372,7 @@ impl Iterator for PrecedingNodeIterator {
         }
 
         self.current = None;
-        return None
+        None
     }
 }
 
@@ -1663,7 +1663,7 @@ impl Node {
         Node::insert(node, parent, reference_child, SuppressObserver::Unsuppressed);
 
         // Step 6.
-        return Ok(Root::from_ref(node))
+        Ok(Root::from_ref(node))
     }
 
     // https://dom.spec.whatwg.org/#concept-node-insert
@@ -2088,25 +2088,14 @@ impl<'a> NodeMethods for &'a Node {
 
     // https://dom.spec.whatwg.org/#dom-node-nodevalue
     fn GetNodeValue(self) -> Option<DOMString> {
-        match self.type_id {
+        CharacterDataCast::to_ref(self).map(|c| c.Data())
-            NodeTypeId::CharacterData(..) => {
-                let chardata: &CharacterData = CharacterDataCast::to_ref(self).unwrap();
-                Some(chardata.Data())
-            }
-            _ => {
-                None
-            }
-        }
     }
 
     // https://dom.spec.whatwg.org/#dom-node-nodevalue
     fn SetNodeValue(self, val: Option<DOMString>) {
-        match self.type_id {
+        if let NodeTypeId::CharacterData(..) = self.type_id {
-            NodeTypeId::CharacterData(..) => {
             self.SetTextContent(val)
         }
-            _ => {}
-        }
     }
 
     // https://dom.spec.whatwg.org/#dom-node-textcontent
@@ -2565,7 +2554,7 @@ pub fn window_from_node<T: NodeBase + Reflectable>(derived: &T) -> Root<Window>
 }
 
 impl<'a> VirtualMethods for &'a Node {
-    fn super_type<'b>(&'b self) -> Option<&'b VirtualMethods> {
+    fn super_type(&self) -> Option<&VirtualMethods> {
         let eventtarget: &&EventTarget = EventTargetCast::from_borrowed_ref(self);
         Some(eventtarget as &VirtualMethods)
     }

components/script/dom/nodeiterator.rs

@@ -138,7 +138,7 @@ impl<'a> NodeIteratorMethods for &'a NodeIterator {
             }
         }
 
-        return Ok(None);
+        Ok(None)
     }
 
     // https://dom.spec.whatwg.org/#dom-nodeiterator-previousnode
@@ -183,7 +183,7 @@ impl<'a> NodeIteratorMethods for &'a NodeIterator {
             }
         }
 
-        return Ok(None);
+        Ok(None)
     }
 
     // https://dom.spec.whatwg.org/#dom-nodeiterator-detach

components/script/dom/range.rs

@@ -122,7 +122,7 @@ impl Range {
             return Err(HierarchyRequest);
         }
 
-        return Ok((first_contained_child, last_contained_child, contained_children));
+        Ok((first_contained_child, last_contained_child, contained_children))
     }
 }
 
@@ -338,17 +338,11 @@ impl<'a> RangeMethods for &'a Range {
         let end = &inner.end;
         let end_node = end.node();
         let end_offset = end.offset;
-        match (bp_position(parent.r(), offset + 1, start_node.r(), start_offset).unwrap(),
-               bp_position(parent.r(), offset, end_node.r(), end_offset).unwrap()) {
-            (Ordering::Greater, Ordering::Less) => {
         // Step 5.
-                true
+        Ordering::Greater == bp_position(parent.r(), offset + 1,
-            },
+                                         start_node.r(), start_offset).unwrap() &&
-            _ => {
+        Ordering::Less == bp_position(parent.r(), offset,
-                // Step 6.
+                                      end_node.r(), end_offset).unwrap()
-                false
-            }
-        }
     }
 
     // https://dom.spec.whatwg.org/#dom-range-clonecontents

components/script/dom/servohtmlparser.rs

@@ -268,7 +268,7 @@ impl ServoHTMLParser {
     }
 
     #[inline]
-    pub fn tokenizer<'a>(&'a self) -> &'a DOMRefCell<Tokenizer> {
+    pub fn tokenizer(&self) -> &DOMRefCell<Tokenizer> {
         &self.tokenizer
     }
 }

components/script/dom/textencoder.rs

@@ -65,7 +65,7 @@ impl TextEncoder {
             }
             _ => {
                 debug!("Encoding Not UTF");
-                return Err(Range("The encoding must be utf-8, utf-16le, or utf-16be.".to_owned()))
+                Err(Range("The encoding must be utf-8, utf-16le, or utf-16be.".to_owned()))
             }
         }
     }
@@ -87,7 +87,7 @@ impl<'a> TextEncoderMethods for &'a TextEncoder {
 
             let js_object_data: *mut uint8_t = JS_GetUint8ArrayData(js_object, ptr::null());
             ptr::copy_nonoverlapping(encoded.as_ptr(), js_object_data, length as usize);
-            return js_object;
+            js_object
         }
     }
 }

components/script/dom/treewalker.rs

@@ -104,12 +104,9 @@ impl<'a> TreeWalkerMethods for &'a TreeWalker {
                     node = n;
                     // "2. If node is not null and filtering node returns FILTER_ACCEPT,
                     //     then set the currentNode attribute to node, return node."
-                    match try!(self.accept_node(node.r())) {
+                    if NodeFilterConstants::FILTER_ACCEPT == try!(self.accept_node(node.r())) {
-                        NodeFilterConstants::FILTER_ACCEPT => {
                         self.current_node.set(JS::from_rooted(&node));
                         return Ok(Some(node))
-                        },
-                        _ => {}
                     }
                 },
                 None => break,
@@ -195,12 +192,9 @@ impl<'a> TreeWalkerMethods for &'a TreeWalker {
             }
             // "5. Filter node and if the return value is FILTER_ACCEPT, then
             //     set the currentNode attribute to node and return node."
-            match try!(self.accept_node(node.r())) {
+            if NodeFilterConstants::FILTER_ACCEPT == try!(self.accept_node(node.r())) {
-                NodeFilterConstants::FILTER_ACCEPT => {
                 self.current_node.set(JS::from_rooted(&node));
                 return Ok(Some(node))
-                },
-                _ => {}
             }
         }
         // "6. Return null."
@@ -217,9 +211,8 @@ impl<'a> TreeWalkerMethods for &'a TreeWalker {
         loop {
             // "1. While result is not FILTER_REJECT and node has a child, run these subsubsteps:"
             loop {
-                match result {
+                if NodeFilterConstants::FILTER_REJECT == result {
-                    NodeFilterConstants::FILTER_REJECT => break,
+                    break;
-                    _ => {}
                 }
                 match node.r().GetFirstChild() {
                     None => break,
@@ -230,12 +223,9 @@ impl<'a> TreeWalkerMethods for &'a TreeWalker {
                         result = try!(self.accept_node(node.r()));
                         // "3. If result is FILTER_ACCEPT, then
                         //     set the currentNode attribute to node and return node."
-                        match result {
+                        if NodeFilterConstants::FILTER_ACCEPT == result {
-                            NodeFilterConstants::FILTER_ACCEPT => {
                             self.current_node.set(JS::from_rooted(&node));
                             return Ok(Some(node))
-                            },
-                            _ => {}
                         }
                     }
                 }
@@ -251,12 +241,9 @@ impl<'a> TreeWalkerMethods for &'a TreeWalker {
                     result = try!(self.accept_node(node.r()));
                     // "4. If result is FILTER_ACCEPT, then
                     //     set the currentNode attribute to node and return node."
-                    match result {
+                    if NodeFilterConstants::FILTER_ACCEPT == result {
-                        NodeFilterConstants::FILTER_ACCEPT => {
                         self.current_node.set(JS::from_rooted(&node));
                         return Ok(Some(node))
-                        },
-                        _ => {}
                     }
                 }
             }
@@ -391,13 +378,11 @@ impl<'a> PrivateTreeWalkerHelpers for &'a TreeWalker {
                 let result = try!(self.accept_node(node.r()));
                 // "3. If result is FILTER_ACCEPT, then set the currentNode
                 //     attribute to node and return node."
-                match result {
+                if NodeFilterConstants::FILTER_ACCEPT == result {
-                    NodeFilterConstants::FILTER_ACCEPT => {
                     self.current_node.set(JS::from_rooted(&node));
                     return Ok(Some(node))
-                    },
-                    _ => {}
                 }
+
                 // "4. Set sibling to node's first child if type is next,
                 //     and node's last child if type is previous."
                 sibling_op = next_child(node.r());
@@ -418,9 +403,8 @@ impl<'a> PrivateTreeWalkerHelpers for &'a TreeWalker {
                 // "5. Filter node and if the return value is FILTER_ACCEPT, then return null."
                 Some(n) => {
                     node = n;
-                    match try!(self.accept_node(node.r())) {
+                    if NodeFilterConstants::FILTER_ACCEPT == try!(self.accept_node(node.r())) {
-                        NodeFilterConstants::FILTER_ACCEPT => return Ok(None),
+                        return Ok(None)
-                        _ => {}
                     }
                 }
             }

components/script/dom/url.rs

@@ -142,7 +142,7 @@ impl<'a> URLMethods for &'a URL {
     }
 }
 
-fn parser_with_base<'a>(base: Option<&'a Url>) -> UrlParser<'a> {
+fn parser_with_base(base: Option<&Url>) -> UrlParser {
     let mut parser = UrlParser::new();
     if let Some(base) = base {
         parser.base_url(base);

components/script/dom/urlhelper.rs

@@ -84,7 +84,7 @@ impl UrlHelper {
         if urlA.port() != urlB.port() {
             return false
         }
-        return true
+        true
     }
 
     // https://url.spec.whatwg.org/#dom-urlutils-search

components/script/dom/virtualmethods.rs

@@ -48,7 +48,7 @@ use string_cache::Atom;
 pub trait VirtualMethods {
     /// Returns self as the superclass of the implementation for this trait,
     /// if any.
-    fn super_type<'b>(&'b self) -> Option<&'b VirtualMethods>;
+    fn super_type(&self) -> Option<&VirtualMethods>;
 
     /// Called when changing or adding attributes, after the attribute's value
     /// has been updated.

components/script/dom/webglrenderingcontext.rs

@@ -514,15 +514,13 @@ impl<'a> WebGLRenderingContextMethods for &'a WebGLRenderingContext {
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
     fn VertexAttribPointer(self, attrib_id: u32, size: i32, data_type: u32,
                            normalized: bool, stride: i32, offset: i64) {
-        match data_type {
+        if let constants::FLOAT = data_type {
-            constants::FLOAT => {
            let msg = CanvasMsg::WebGL(
                CanvasWebGLMsg::VertexAttribPointer2f(attrib_id, size, normalized, stride, offset));
             self.ipc_renderer.send(msg).unwrap()
+        } else {
+            panic!("VertexAttribPointer: Data Type not supported")
         }
-            _ => panic!("VertexAttribPointer: Data Type not supported")
-        }
-
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.4

components/script/dom/websocket.rs

@@ -277,7 +277,7 @@ impl<'a> WebSocketMethods for &'a WebSocket {
         let mut other_sender = self.sender.borrow_mut();
         let my_sender = other_sender.as_mut().unwrap();
         let _ = my_sender.lock().unwrap().send_message(Message::Text(data.unwrap().0));
-        return Ok(())
+        Ok(())
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-websocket-close

components/script/dom/window.rs

@@ -269,19 +269,19 @@ impl Window {
         (box SendableMainThreadScriptChan(tx), box rx)
     }
 
-    pub fn image_cache_task<'a>(&'a self) -> &'a ImageCacheTask {
+    pub fn image_cache_task(&self) -> &ImageCacheTask {
         &self.image_cache_task
     }
 
-    pub fn compositor<'a>(&'a self) -> &'a IpcSender<ScriptToCompositorMsg> {
+    pub fn compositor(&self) -> &IpcSender<ScriptToCompositorMsg> {
         &self.compositor
     }
 
-    pub fn browsing_context<'a>(&'a self) -> Ref<'a, Option<BrowsingContext>> {
+    pub fn browsing_context(&self) -> Ref<Option<BrowsingContext>> {
         self.browsing_context.borrow()
     }
 
-    pub fn page<'a>(&'a self) -> &'a Page {
+    pub fn page(&self) -> &Page {
         &*self.page
     }
 

components/script/dom/workerglobalscope.rs

@@ -143,11 +143,11 @@ impl WorkerGlobalScope {
         self.runtime.cx()
     }
 
-    pub fn resource_task<'a>(&'a self) -> &'a ResourceTask {
+    pub fn resource_task(&self) -> &ResourceTask {
         &self.resource_task
     }
 
-    pub fn get_url<'a>(&'a self) -> &'a Url {
+    pub fn get_url(&self) -> &Url {
         &self.worker_url
     }
 

components/script/dom/xmlhttprequest.rs

@@ -784,8 +784,8 @@ impl<'a> PrivateXMLHttpRequestHelpers for &'a XMLHttpRequest {
 
     fn process_headers_available(self, cors_request: Option<CORSRequest>,
                                  gen_id: GenerationId, metadata: Metadata) -> Result<(), Error> {
-        match cors_request {
+
-            Some(ref req) => {
+        if let Some(ref req) = cors_request {
             match metadata.headers {
                 Some(ref h) if allow_cross_origin_request(req, h) => {},
                 _ => {
@@ -793,10 +793,8 @@ impl<'a> PrivateXMLHttpRequestHelpers for &'a XMLHttpRequest {
                     return Err(Network);
                 }
             }
-            },
+        }
 
-            _ => {}
-        };
         // XXXManishearth Clear cache entries in case of a network error
         self.process_partial_response(XHRProgress::HeadersReceived(gen_id,
                                                                    metadata.headers,

components/script/dom/xmlhttprequesteventtarget.rs

@@ -28,7 +28,7 @@ impl XMLHttpRequestEventTarget {
     }
 
     #[inline]
-    pub fn eventtarget<'a>(&'a self) -> &'a EventTarget {
+    pub fn eventtarget(&self) -> &EventTarget {
         &self.eventtarget
     }
 }

components/script/script_task.rs

@@ -267,7 +267,7 @@ pub struct SendableMainThreadScriptChan(pub Sender<CommonScriptMsg>);
 impl ScriptChan for SendableMainThreadScriptChan {
     fn send(&self, msg: CommonScriptMsg) -> Result<(), ()> {
         let SendableMainThreadScriptChan(ref chan) = *self;
-        return chan.send(msg).map_err(|_| ());
+        chan.send(msg).map_err(|_| ())
     }
 
     fn clone(&self) -> Box<ScriptChan + Send> {
@@ -550,7 +550,7 @@ unsafe extern "C" fn debug_gc_callback(_rt: *mut JSRuntime, status: JSGCStatus,
 unsafe extern "C" fn shadow_check_callback(_cx: *mut JSContext,
     _object: HandleObject, _id: HandleId) -> DOMProxyShadowsResult {
     // XXX implement me
-    return DOMProxyShadowsResult::ShadowCheckFailed;
+    DOMProxyShadowsResult::ShadowCheckFailed
 }
 
 impl ScriptTask {
@@ -1062,7 +1062,7 @@ impl ScriptTask {
             return ScriptState::DocumentLoading;
         }
 
-        return ScriptState::DocumentLoaded;
+        ScriptState::DocumentLoaded
     }
 
     fn handle_new_layout(&self, new_layout_info: NewLayoutInfo) {
@@ -1406,7 +1406,7 @@ impl ScriptTask {
         if let Some(ref mut child_page) = page.remove(id) {
             shut_down_layout(&*child_page, exit_type);
         }
-        return false;
+        false
     }
 
     /// Handles when layout task finishes all animation in one tick
@@ -1550,7 +1550,7 @@ impl ScriptTask {
                                      DocumentSource::FromParser,
                                      loader);
 
-        let frame_element = frame_element.r().map(|elem| ElementCast::from_ref(elem));
+        let frame_element = frame_element.r().map(ElementCast::from_ref);
         window.r().init_browsing_context(document.r(), frame_element);
 
         // Create the root frame

components/script/textinput.rs

@@ -292,10 +292,11 @@ impl<T: ClipboardProvider> TextInput<T> {
     /// Deal with a newline input.
     pub fn handle_return(&mut self) -> KeyReaction {
         if !self.multiline {
-            return KeyReaction::TriggerDefaultAction;
+            KeyReaction::TriggerDefaultAction
-        }
+        } else {
             self.insert_char('\n');
-        return KeyReaction::DispatchInput;
+            KeyReaction::DispatchInput
+        }
     }
 
     /// Select all text in the input control.

components/script/timers.rs

@@ -222,8 +222,8 @@ impl TimerManager {
         for _ in 0..arguments.len() {
             timer.data.args.push(Heap::default());
         }
-        for i in 0..arguments.len() {
+        for (i, item) in arguments.iter().enumerate() {
-            timer.data.args.get_mut(i).unwrap().set(arguments[i].get());
+            timer.data.args.get_mut(i).unwrap().set(item.get());
         }
         handle
     }