Make missing documentation linting more robust

Add some more cases for warning about missing documentation, and also add a test
to make sure it doesn't die in the future.

Author: alexcrichton

src/librustc/front/intrinsic.rs

@@ -12,6 +12,8 @@
 // and injected into each crate the compiler builds. Keep it small.
 
 pub mod intrinsic {
+    #[allow(missing_doc)];
+
     pub use intrinsic::rusti::visit_tydesc;
 
     // FIXME (#3727): remove this when the interface has settled and the

src/librustc/middle/lint.rs

@@ -95,8 +95,7 @@ pub enum lint {
     unused_mut,
     unnecessary_allocation,
 
-    missing_struct_doc,
-    missing_trait_doc,
+    missing_doc,
 }
 
 pub fn level_to_str(lv: level) -> &'static str {
@@ -268,17 +267,10 @@ static lint_table: &'static [(&'static str, LintSpec)] = &[
         default: warn
     }),
 
-    ("missing_struct_doc",
+    ("missing_doc",
      LintSpec {
-        lint: missing_struct_doc,
-        desc: "detects missing documentation for structs",
-        default: allow
-    }),
-
-    ("missing_trait_doc",
-     LintSpec {
-        lint: missing_trait_doc,
-        desc: "detects missing documentation for traits",
+        lint: missing_doc,
+        desc: "detects missing documentation for public members",
         default: allow
     }),
 ];
@@ -302,6 +294,9 @@ struct Context {
     curr: SmallIntMap<(level, LintSource)>,
     // context we're checking in (used to access fields like sess)
     tcx: ty::ctxt,
+    // Just a simple flag if we're currently recursing into a trait
+    // implementation. This is only used by the lint_missing_doc() pass
+    in_trait_impl: bool,
     // When recursing into an attributed node of the ast which modifies lint
     // levels, this stack keeps track of the previous lint levels of whatever
     // was modified.
@@ -311,7 +306,15 @@ struct Context {
     // Others operate directly on @ast::item structures (or similar). Finally,
     // others still are added to the Session object via `add_lint`, and these
     // are all passed with the lint_session visitor.
-    visitors: ~[visit::vt<@mut Context>],
+    //
+    // This is a pair so every visitor can visit every node. When a lint pass is
+    // registered, another visitor is created which stops at all items which can
+    // alter the attributes of the ast. This "item stopping visitor" is the
+    // second element of the pair, while the original visitor is the first
+    // element. This means that when visiting a node, the original recursive
+    // call can used the original visitor's method, although the recursing
+    // visitor supplied to the method is the item stopping visitor.
+    visitors: ~[(visit::vt<@mut Context>, visit::vt<@mut Context>)],
 }
 
 impl Context {
@@ -429,29 +432,31 @@ impl Context {
     }
 
     fn add_lint(&mut self, v: visit::vt<@mut Context>) {
-        self.visitors.push(item_stopping_visitor(v));
+        self.visitors.push((v, item_stopping_visitor(v)));
     }
 
     fn process(@mut self, n: AttributedNode) {
+        // see comment of the `visitors` field in the struct for why there's a
+        // pair instead of just one visitor.
         match n {
             Item(it) => {
-                for self.visitors.each |v| {
-                    visit::visit_item(it, self, *v);
+                for self.visitors.each |&(orig, stopping)| {
+                    (orig.visit_item)(it, self, stopping);
                 }
             }
             Crate(c) => {
-                for self.visitors.each |v| {
-                    visit::visit_crate(c, self, *v);
+                for self.visitors.each |&(_, stopping)| {
+                    visit::visit_crate(c, self, stopping);
                 }
             }
             // Can't use visit::visit_method_helper because the
             // item_stopping_visitor has overridden visit_fn(&fk_method(... ))
             // to be a no-op, so manually invoke visit_fn.
             Method(m) => {
                 let fk = visit::fk_method(copy m.ident, &m.generics, m);
-                for self.visitors.each |v| {
-                    visit::visit_fn(&fk, &m.decl, &m.body, m.span, m.id,
-                                    self, *v);
+                for self.visitors.each |&(orig, stopping)| {
+                    (orig.visit_fn)(&fk, &m.decl, &m.body, m.span, m.id,
+                                    self, stopping);
                 }
             }
         }
@@ -495,16 +500,16 @@ pub fn each_lint(sess: session::Session,
 // This is used to make the simple visitors used for the lint passes
 // not traverse into subitems, since that is handled by the outer
 // lint visitor.
-fn item_stopping_visitor<E: Copy>(v: visit::vt<E>) -> visit::vt<E> {
+fn item_stopping_visitor<E: Copy>(outer: visit::vt<E>) -> visit::vt<E> {
     visit::mk_vt(@visit::Visitor {
         visit_item: |_i, _e, _v| { },
         visit_fn: |fk, fd, b, s, id, e, v| {
             match *fk {
                 visit::fk_method(*) => {}
-                _ => visit::visit_fn(fk, fd, b, s, id, e, v)
+                _ => (outer.visit_fn)(fk, fd, b, s, id, e, v)
             }
         },
-    .. **(ty_stopping_visitor(v))})
+    .. **(ty_stopping_visitor(outer))})
 }
 
 fn ty_stopping_visitor<E>(v: visit::vt<E>) -> visit::vt<E> {
@@ -972,68 +977,84 @@ fn lint_unnecessary_allocations() -> visit::vt<@mut Context> {
     })
 }
 
-fn lint_missing_struct_doc() -> visit::vt<@mut Context> {
+fn lint_missing_doc() -> visit::vt<@mut Context> {
+    fn check_attrs(cx: @mut Context, attrs: &[ast::attribute],
+                   sp: span, msg: &str) {
+        if !attrs.any(|a| a.node.is_sugared_doc) {
+            cx.span_lint(missing_doc, sp, msg);
+        }
+    }
+
     visit::mk_vt(@visit::Visitor {
-        visit_struct_field: |field, cx: @mut Context, vt| {
-            let relevant = match field.node.kind {
-                ast::named_field(_, vis) => vis != ast::private,
-                ast::unnamed_field => false,
-            };
+        visit_struct_method: |m, cx, vt| {
+            if m.vis == ast::public {
+                check_attrs(cx, m.attrs, m.span,
+                            "missing documentation for a method");
+            }
+            visit::visit_struct_method(m, cx, vt);
+        },
+
+        visit_ty_method: |m, cx, vt| {
+            // All ty_method objects are linted about because they're part of a
+            // trait (no visibility)
+            check_attrs(cx, m.attrs, m.span,
+                        "missing documentation for a method");
+            visit::visit_ty_method(m, cx, vt);
+        },
 
-            if relevant {
-                let mut has_doc = false;
-                for field.node.attrs.each |attr| {
-                    if attr.node.is_sugared_doc {
-                        has_doc = true;
-                        break;
+        visit_fn: |fk, d, b, sp, id, cx, vt| {
+            // Only warn about explicitly public methods. Soon implicit
+            // public-ness will hopefully be going away.
+            match *fk {
+                visit::fk_method(_, _, m) if m.vis == ast::public => {
+                    // If we're in a trait implementation, no need to duplicate
+                    // documentation
+                    if !cx.in_trait_impl {
+                        check_attrs(cx, m.attrs, sp,
+                                    "missing documentation for a method");
                     }
                 }
-                if !has_doc {
-                    cx.span_lint(missing_struct_doc, field.span, "missing documentation \
-                                                                  for a field.");
-                }
-            }
 
-            visit::visit_struct_field(field, cx, vt);
+                _ => {}
+            }
+            visit::visit_fn(fk, d, b, sp, id, cx, vt);
         },
-        .. *visit::default_visitor()
-    })
-}
 
-fn lint_missing_trait_doc() -> visit::vt<@mut Context> {
-    visit::mk_vt(@visit::Visitor {
-        visit_trait_method: |method, cx: @mut Context, vt| {
-            let mut has_doc = false;
-            let span = match copy *method {
-                ast::required(m) => {
-                    for m.attrs.each |attr| {
-                        if attr.node.is_sugared_doc {
-                            has_doc = true;
-                            break;
-                        }
-                    }
-                    m.span
-                },
-                ast::provided(m) => {
-                    if m.vis == ast::private {
-                        has_doc = true;
-                    } else {
-                        for m.attrs.each |attr| {
-                            if attr.node.is_sugared_doc {
-                                has_doc = true;
-                                break;
+        visit_item: |it, cx, vt| {
+            match it.node {
+                // Go ahead and match the fields here instead of using
+                // visit_struct_field while we have access to the enclosing
+                // struct's visibility
+                ast::item_struct(sdef, _) if it.vis == ast::public => {
+                    check_attrs(cx, it.attrs, it.span,
+                                "missing documentation for a struct");
+                    for sdef.fields.each |field| {
+                        match field.node.kind {
+                            ast::named_field(_, vis) if vis != ast::private => {
+                                check_attrs(cx, field.node.attrs, field.span,
+                                            "missing documentation for a field");
                             }
+                            ast::unnamed_field | ast::named_field(*) => {}
                         }
                     }
-                    m.span
                 }
+
+                ast::item_trait(*) if it.vis == ast::public => {
+                    check_attrs(cx, it.attrs, it.span,
+                                "missing documentation for a trait");
+                }
+
+                ast::item_fn(*) if it.vis == ast::public => {
+                    check_attrs(cx, it.attrs, it.span,
+                                "missing documentation for a function");
+                }
+
+                _ => {}
             };
-            if !has_doc {
-                cx.span_lint(missing_trait_doc, span, "missing documentation \
-                                                       for a method.");
-            }
-            visit::visit_trait_method(method, cx, vt);
+
+            visit::visit_item(it, cx, vt);
         },
+
         .. *visit::default_visitor()
     })
 }
@@ -1045,6 +1066,7 @@ pub fn check_crate(tcx: ty::ctxt, crate: @ast::crate) {
         tcx: tcx,
         lint_stack: ~[],
         visitors: ~[],
+        in_trait_impl: false,
     };
 
     // Install defaults.
@@ -1066,8 +1088,7 @@ pub fn check_crate(tcx: ty::ctxt, crate: @ast::crate) {
     cx.add_lint(lint_unused_mut());
     cx.add_lint(lint_session());
     cx.add_lint(lint_unnecessary_allocations());
-    cx.add_lint(lint_missing_struct_doc());
-    cx.add_lint(lint_missing_trait_doc());
+    cx.add_lint(lint_missing_doc());
 
     // Actually perform the lint checks (iterating the ast)
     do cx.with_lint_attrs(crate.node.attrs) {
@@ -1076,13 +1097,20 @@ pub fn check_crate(tcx: ty::ctxt, crate: @ast::crate) {
         visit::visit_crate(crate, cx, visit::mk_vt(@visit::Visitor {
             visit_item: |it, cx: @mut Context, vt| {
                 do cx.with_lint_attrs(it.attrs) {
+                    match it.node {
+                        ast::item_impl(_, Some(*), _, _) => {
+                            cx.in_trait_impl = true;
+                        }
+                        _ => {}
+                    }
                     check_item_ctypes(cx, it);
                     check_item_non_camel_case_types(cx, it);
                     check_item_default_methods(cx, it);
                     check_item_heap(cx, it);
 
                     cx.process(Item(it));
                     visit::visit_item(it, cx, vt);
+                    cx.in_trait_impl = false;
                 }
             },
             visit_fn: |fk, decl, body, span, id, cx, vt| {

src/test/compile-fail/lint-missing-doc.rs

@@ -0,0 +1,72 @@
+// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// When denying at the crate level, be sure to not get random warnings from the
+// injected intrinsics by the compiler.
+#[deny(missing_doc)];
+
+struct Foo {
+    a: int,
+    priv b: int,
+    pub c: int, // doesn't matter, Foo is private
+}
+
+pub struct PubFoo { //~ ERROR: missing documentation
+    a: int,      //~ ERROR: missing documentation
+    priv b: int,
+    pub c: int,  //~ ERROR: missing documentation
+}
+
+#[allow(missing_doc)]
+pub struct PubFoo2 {
+    a: int,
+    pub c: int,
+}
+
+/// dox
+pub fn foo() {}
+pub fn foo2() {} //~ ERROR: missing documentation
+fn foo3() {}
+#[allow(missing_doc)] pub fn foo4() {}
+
+/// dox
+pub trait A {}
+trait B {}
+pub trait C {} //~ ERROR: missing documentation
+#[allow(missing_doc)] pub trait D {}
+
+trait Bar {
+    /// dox
+    pub fn foo();
+    fn foo2(); //~ ERROR: missing documentation
+    pub fn foo3(); //~ ERROR: missing documentation
+}
+
+impl Foo {
+    pub fn foo() {} //~ ERROR: missing documentation
+    /// dox
+    pub fn foo1() {}
+    fn foo2() {}
+    #[allow(missing_doc)] pub fn foo3() {}
+}
+
+#[allow(missing_doc)]
+trait F {
+    pub fn a();
+    fn b(&self);
+}
+
+// should need to redefine documentation for implementations of traits
+impl F for Foo {
+    pub fn a() {}
+    fn b(&self) {}
+}
+
+fn main() {}