make missing_doc lint respect the visibility rules

Previously, the `exported_items` set created by the privacy pass was
incomplete. Specifically, it did not include items that had been defined
at a private path but then `pub use`d at a public path. This commit
finds all crate exports during the privacy pass. Consequently, some code
in the reachable pass and in rustdoc is no longer necessary. This commit
then removes the separate `MissingDocLintVisitor` lint pass, opting to
check missing_doc lint in the same pass as the other lint checkers using
the visibility result computed by the privacy pass.

Fixes #9777.

Author: dcrewi

src/librustc/driver/driver.rs

@@ -305,11 +305,10 @@ pub fn phase_3_run_analysis_passes(sess: Session,
 
     let reachable_map =
         time(time_passes, "reachability checking", (), |_|
-             reachable::find_reachable(ty_cx, method_map, exp_map2,
-                                       &exported_items));
+             reachable::find_reachable(ty_cx, method_map, &exported_items));
 
     time(time_passes, "lint checking", (), |_|
-         lint::check_crate(ty_cx, crate));
+         lint::check_crate(ty_cx, &exported_items, crate));
 
     CrateAnalysis {
         exp_map2: exp_map2,

src/librustc/middle/lint.rs

@@ -34,6 +34,7 @@
 //! Context itself, span_lint should be used instead of add_lint.
 
 use driver::session;
+use middle::privacy;
 use middle::trans::adt; // for `adt::is_ffi_safe`
 use middle::ty;
 use middle::pat_util;
@@ -317,21 +318,28 @@ pub fn get_lint_dict() -> LintDict {
     return map;
 }
 
-struct Context {
+struct Context<'self> {
     // All known lint modes (string versions)
     dict: @LintDict,
     // Current levels of each lint warning
     cur: SmallIntMap<(level, LintSource)>,
     // context we're checking in (used to access fields like sess)
     tcx: ty::ctxt,
+    // Items exported by the crate; used by the missing_doc lint.
+    exported_items: &'self privacy::ExportedItems,
+    // The id of the current `ast::struct_def` being walked.
+    cur_struct_def_id: ast::NodeId,
+    // Whether some ancestor of the current node was marked
+    // #[doc(hidden)].
+    is_doc_hidden: 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.
     lint_stack: ~[(lint, level, LintSource)],
 }
 
-impl Context {
+impl<'self> Context<'self> {
     fn get_level(&self, lint: lint) -> level {
         match self.cur.find(&(lint as uint)) {
           Some(&(lvl, _)) => lvl,
@@ -440,9 +448,16 @@ impl Context {
             true
         };
 
+        let old_is_doc_hidden = self.is_doc_hidden;
+        self.is_doc_hidden = self.is_doc_hidden ||
+            attrs.iter().any(|attr| ("doc" == attr.name() && match attr.meta_item_list()
+                                     { None => false,
+                                       Some(l) => attr::contains_name(l, "hidden") }));
+
         f(self);
 
         // rollback
+        self.is_doc_hidden = old_is_doc_hidden;
         do pushed.times {
             let (lint, lvl, src) = self.lint_stack.pop();
             self.set_level(lint, lvl, src);
@@ -870,125 +885,83 @@ fn check_unnecessary_allocation(cx: &Context, e: &ast::Expr) {
     }
 }
 
-struct MissingDocLintVisitor(ty::ctxt);
-
-impl MissingDocLintVisitor {
-    fn check_attrs(&self, attrs: &[ast::Attribute], id: ast::NodeId,
-                   sp: Span, msg: ~str) {
-        if !attrs.iter().any(|a| a.node.is_sugared_doc) {
-            self.sess.add_lint(missing_doc, id, sp, msg);
-        }
-    }
-
-    fn check_struct(&self, sdef: &ast::struct_def) {
-        for field in sdef.fields.iter() {
-            match field.node.kind {
-                ast::named_field(_, vis) if vis != ast::private => {
-                    self.check_attrs(field.node.attrs, field.node.id, field.span,
-                                     ~"missing documentation for a field");
-                }
-                ast::unnamed_field | ast::named_field(*) => {}
-            }
-        }
-    }
-
-    fn doc_hidden(&self, attrs: &[ast::Attribute]) -> bool {
-        do attrs.iter().any |attr| {
-            "doc" == attr.name() &&
-                match attr.meta_item_list() {
-                    Some(l) => attr::contains_name(l, "hidden"),
-                    None    => false // not of the form #[doc(...)]
-                }
-        }
+fn check_missing_doc_attrs(cx: &Context,
+                           id: ast::NodeId,
+                           attrs: &[ast::Attribute],
+                           sp: Span,
+                           desc: &'static str) {
+    // If we're building a test harness, then warning about
+    // documentation is probably not really relevant right now.
+    if cx.tcx.sess.opts.test { return }
+
+    // `#[doc(hidden)]` disables missing_doc check.
+    if cx.is_doc_hidden { return }
+
+    // Only check publicly-visible items, using the result from the
+    // privacy pass.
+    if !cx.exported_items.contains(&id) { return }
+
+    if !attrs.iter().any(|a| a.node.is_sugared_doc) {
+        cx.span_lint(missing_doc, sp,
+                     format!("missing documentation for {}", desc));
     }
 }
 
-impl Visitor<()> for MissingDocLintVisitor {
-    fn visit_ty_method(&mut self, m:&ast::TypeMethod, _: ()) {
-        if self.doc_hidden(m.attrs) { return }
-
-        // All ty_method objects are linted about because they're part of a
-        // trait (no visibility)
-        self.check_attrs(m.attrs, m.id, m.span,
-                         ~"missing documentation for a method");
-        visit::walk_ty_method(self, m, ());
-    }
+fn check_missing_doc_item(cx: &mut Context, it: &ast::item) { // XXX doesn't need to be mut
+    let desc = match it.node {
+        ast::item_fn(*) => "a function",
+        ast::item_mod(*) => "a module",
+        ast::item_enum(*) => "an enum",
+        ast::item_struct(*) => "a struct",
+        ast::item_trait(*) => "a trait",
+        _ => return
+    };
+    check_missing_doc_attrs(cx, it.id, it.attrs, it.span, desc);
+}
 
-    fn visit_fn(&mut self, fk: &visit::fn_kind, d: &ast::fn_decl,
-                b: &ast::Block, sp: Span, id: ast::NodeId, _: ()) {
-        // Only warn about explicitly public methods.
-        match *fk {
-            visit::fk_method(_, _, m) => {
-                if self.doc_hidden(m.attrs) {
-                    return;
-                }
-                // If we're in a trait implementation, no need to duplicate
-                // documentation
-                if m.vis == ast::public {
-                    self.check_attrs(m.attrs, id, sp,
-                                     ~"missing documentation for a method");
+fn check_missing_doc_method(cx: &Context, m: &ast::method) {
+    let did = ast::DefId {
+        crate: ast::LOCAL_CRATE,
+        node: m.id
+    };
+    match cx.tcx.methods.find(&did) {
+        None => cx.tcx.sess.span_bug(m.span, "missing method descriptor?!"),
+        Some(md) => {
+            match md.container {
+                // Always check default methods defined on traits.
+                ty::TraitContainer(*) => {}
+                // For methods defined on impls, it depends on whether
+                // it is an implementation for a trait or is a plain
+                // impl.
+                ty::ImplContainer(cid) => {
+                    match ty::impl_trait_ref(cx.tcx, cid) {
+                        Some(*) => return, // impl for trait: don't doc
+                        None => {} // plain impl: doc according to privacy
+                    }
                 }
             }
-            _ => {}
         }
-        visit::walk_fn(self, fk, d, b, sp, id, ());
     }
+    check_missing_doc_attrs(cx, m.id, m.attrs, m.span, "a method");
+}
 
-    fn visit_item(&mut self, it: @ast::item, _: ()) {
-        // If we're building a test harness, then warning about documentation is
-        // probably not really relevant right now
-        if self.sess.opts.test { return }
-        if self.doc_hidden(it.attrs) { return }
-
-        match it.node {
-            ast::item_struct(sdef, _) if it.vis == ast::public => {
-                self.check_attrs(it.attrs, it.id, it.span,
-                                 ~"missing documentation for a struct");
-                self.check_struct(sdef);
-            }
-
-            // Skip implementations because they inherit documentation from the
-            // trait (which was already linted)
-            ast::item_impl(_, Some(*), _, _) => return,
-
-            ast::item_trait(*) if it.vis != ast::public => return,
-            ast::item_trait(*) => self.check_attrs(it.attrs, it.id, it.span,
-                                                   ~"missing documentation for a trait"),
-
-            ast::item_fn(*) if it.vis == ast::public => {
-                self.check_attrs(it.attrs, it.id, it.span,
-                                 ~"missing documentation for a function");
-            }
-
-            ast::item_mod(*) if it.vis == ast::public => {
-                self.check_attrs(it.attrs, it.id, it.span,
-                                 ~"missing documentation for a module");
-            }
-
-            ast::item_enum(ref edef, _) if it.vis == ast::public => {
-                self.check_attrs(it.attrs, it.id, it.span,
-                                 ~"missing documentation for an enum");
-                for variant in edef.variants.iter() {
-                    if variant.node.vis == ast::private { continue; }
-
-                    self.check_attrs(variant.node.attrs, variant.node.id,
-                                     variant.span,
-                                     ~"missing documentation for a variant");
-                    match variant.node.kind {
-                        ast::struct_variant_kind(sdef) => {
-                            self.check_struct(sdef);
-                        }
-                        _ => ()
-                    }
-                }
-            }
+fn check_missing_doc_ty_method(cx: &Context, tm: &ast::TypeMethod) {
+    check_missing_doc_attrs(cx, tm.id, tm.attrs, tm.span, "a type method");
+}
 
-            _ => {}
-        }
-        visit::walk_item(self, it, ());
+fn check_missing_doc_struct_field(cx: &Context, sf: &ast::struct_field) {
+    match sf.node.kind {
+        ast::named_field(_, vis) if vis != ast::private =>
+            check_missing_doc_attrs(cx, cx.cur_struct_def_id, sf.node.attrs,
+                                    sf.span, "a struct field"),
+        _ => {}
     }
 }
 
+fn check_missing_doc_variant(cx: &Context, v: &ast::variant) {
+    check_missing_doc_attrs(cx, v.node.id, v.node.attrs, v.span, "a variant");
+}
+
 /// Checks for use of items with #[deprecated], #[experimental] and
 /// #[unstable] (or none of them) attributes.
 fn check_stability(cx: &Context, e: &ast::Expr) {
@@ -1062,13 +1035,14 @@ fn check_stability(cx: &Context, e: &ast::Expr) {
     cx.span_lint(lint, e.span, msg);
 }
 
-impl Visitor<()> for Context {
+impl<'self> Visitor<()> for Context<'self> {
     fn visit_item(&mut self, it: @ast::item, _: ()) {
         do self.with_lint_attrs(it.attrs) |cx| {
             check_item_ctypes(cx, it);
             check_item_non_camel_case_types(cx, it);
             check_item_non_uppercase_statics(cx, it);
             check_heap_item(cx, it);
+            check_missing_doc_item(cx, it);
 
             do cx.visit_ids |v| {
                 v.visit_item(it, ());
@@ -1111,6 +1085,8 @@ impl Visitor<()> for Context {
         match *fk {
             visit::fk_method(_, _, m) => {
                 do self.with_lint_attrs(m.attrs) |cx| {
+                    check_missing_doc_method(cx, m);
+
                     do cx.visit_ids |v| {
                         v.visit_fn(fk, decl, body, span, id, ());
                     }
@@ -1120,9 +1096,45 @@ impl Visitor<()> for Context {
             _ => recurse(self),
         }
     }
+
+    fn visit_ty_method(&mut self, t: &ast::TypeMethod, _: ()) {
+        do self.with_lint_attrs(t.attrs) |cx| {
+            check_missing_doc_ty_method(cx, t);
+
+            visit::walk_ty_method(cx, t, ());
+        }
+    }
+
+    fn visit_struct_def(&mut self,
+                        s: @ast::struct_def,
+                        i: ast::Ident,
+                        g: &ast::Generics,
+                        id: ast::NodeId,
+                        _: ()) {
+        let old_id = self.cur_struct_def_id;
+        self.cur_struct_def_id = id;
+        visit::walk_struct_def(self, s, i, g, id, ());
+        self.cur_struct_def_id = old_id;
+    }
+
+    fn visit_struct_field(&mut self, s: @ast::struct_field, _: ()) {
+        do self.with_lint_attrs(s.node.attrs) |cx| {
+            check_missing_doc_struct_field(cx, s);
+
+            visit::walk_struct_field(cx, s, ());
+        }
+    }
+
+    fn visit_variant(&mut self, v: &ast::variant, g: &ast::Generics, _: ()) {
+        do self.with_lint_attrs(v.node.attrs) |cx| {
+            check_missing_doc_variant(cx, v);
+
+            visit::walk_variant(cx, v, g, ());
+        }
+    }
 }
 
-impl ast_util::IdVisitingOperation for Context {
+impl<'self> ast_util::IdVisitingOperation for Context<'self> {
     fn visit_id(&self, id: ast::NodeId) {
         match self.tcx.sess.lints.pop(&id) {
             None => {}
@@ -1135,17 +1147,16 @@ impl ast_util::IdVisitingOperation for Context {
     }
 }
 
-pub fn check_crate(tcx: ty::ctxt, crate: &ast::Crate) {
-    // This visitor contains more state than is currently maintained in Context,
-    // and there's no reason for the Context to keep track of this information
-    // really
-    let mut dox = MissingDocLintVisitor(tcx);
-    visit::walk_crate(&mut dox, crate, ());
-
+pub fn check_crate(tcx: ty::ctxt,
+                   exported_items: &privacy::ExportedItems,
+                   crate: &ast::Crate) {
     let mut cx = Context {
         dict: @get_lint_dict(),
         cur: SmallIntMap::new(),
         tcx: tcx,
+        exported_items: exported_items,
+        cur_struct_def_id: -1,
+        is_doc_hidden: false,
         lint_stack: ~[],
     };