From 3e33ef4c42c5e4c4400a8cd470ac851a4dff0789 Mon Sep 17 00:00:00 2001 From: mitaa Date: Thu, 24 Mar 2016 06:10:52 +0100 Subject: [PATCH 1/9] Correct anchor for links to associated trait items --- src/librustdoc/clean/inline.rs | 17 ++++-- src/librustdoc/clean/mod.rs | 63 +++++++++++++--------- src/librustdoc/html/render.rs | 92 +++++++++++++++++---------------- src/librustdoc/passes.rs | 13 ++--- src/test/rustdoc/issue-28478.rs | 39 ++++++++++++++ 5 files changed, 143 insertions(+), 81 deletions(-) create mode 100644 src/test/rustdoc/issue-28478.rs diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index e9c55c56a9d72..265b43ca16b38 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -26,7 +26,7 @@ use rustc::middle::const_eval; use core::DocContext; use doctree; -use clean::{self, Attributes}; +use clean::{self, Attributes, GetDefId}; use super::{Clean, ToSource}; @@ -414,15 +414,22 @@ pub fn build_impl(cx: &DocContext, clean::RegionBound(..) => unreachable!(), } }); - if let Some(clean::ResolvedPath { did, .. }) = trait_ { - if Some(did) == cx.deref_trait_did.get() { - super::build_deref_target_impls(cx, &trait_items, ret); - } + if trait_.def_id() == cx.deref_trait_did.get() { + super::build_deref_target_impls(cx, &trait_items, ret); } + + let provided = trait_.def_id().map(|did| { + cx.tcx().provided_trait_methods(did) + .into_iter() + .map(|meth| meth.name.to_string()) + .collect() + }).unwrap_or(HashSet::new()); + ret.push(clean::Item { inner: clean::ImplItem(clean::Impl { unsafety: hir::Unsafety::Normal, // FIXME: this should be decoded derived: clean::detect_derived(&attrs), + provided_trait_methods: provided, trait_: trait_, for_: ty.ty.clean(cx), generics: (&ty.generics, &predicates, subst::TypeSpace).clean(cx), diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index cca027ca17a01..bec3ae799cace 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -44,7 +44,7 @@ use rustc::middle::stability; use rustc_front::hir; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::rc::Rc; use std::u32; @@ -559,15 +559,9 @@ impl TyParamBound { fn is_sized_bound(&self, cx: &DocContext) -> bool { use rustc_front::hir::TraitBoundModifier as TBM; if let Some(tcx) = cx.tcx_opt() { - let sized_did = match tcx.lang_items.sized_trait() { - Some(did) => did, - None => return false - }; - if let TyParamBound::TraitBound(PolyTrait { - trait_: Type::ResolvedPath { did, .. }, .. - }, TBM::None) = *self { - if did == sized_did { - return true + if let TyParamBound::TraitBound(PolyTrait { ref trait_, .. }, TBM::None) = *self { + if trait_.def_id() == tcx.lang_items.sized_trait() { + return true; } } } @@ -724,15 +718,18 @@ impl<'tcx> Clean for ty::TraitRef<'tcx> { } } - TraitBound(PolyTrait { - trait_: ResolvedPath { - path: path, - typarams: None, - did: self.def_id, - is_generic: false, + TraitBound( + PolyTrait { + trait_: ResolvedPath { + path: path, + typarams: None, + did: self.def_id, + is_generic: false, + }, + lifetimes: late_bounds, }, - lifetimes: late_bounds - }, hir::TraitBoundModifier::None) + hir::TraitBoundModifier::None + ) } } @@ -932,7 +929,6 @@ impl<'a, 'tcx> Clean for (&'a ty::Generics<'tcx>, &'a ty::GenericPredicates<'tcx>, subst::ParamSpace) { fn clean(&self, cx: &DocContext) -> Generics { - use std::collections::HashSet; use self::WherePredicate as WP; let (gens, preds, space) = *self; @@ -1486,6 +1482,16 @@ pub enum TypeKind { TypeTypedef, } +pub trait GetDefId { + fn def_id(&self) -> Option; +} + +impl GetDefId for Option { + fn def_id(&self) -> Option { + self.as_ref().and_then(|d| d.def_id()) + } +} + impl Type { pub fn primitive_type(&self) -> Option { match *self { @@ -1499,7 +1505,9 @@ impl Type { _ => None, } } +} +impl GetDefId for Type { fn def_id(&self) -> Option { match *self { ResolvedPath { did, .. } => Some(did), @@ -2208,6 +2216,7 @@ impl Clean for hir::ImplPolarity { pub struct Impl { pub unsafety: hir::Unsafety, pub generics: Generics, + pub provided_trait_methods: HashSet, pub trait_: Option, pub for_: Type, pub items: Vec, @@ -2227,12 +2236,19 @@ impl Clean> for doctree::Impl { // If this impl block is an implementation of the Deref trait, then we // need to try inlining the target's inherent impl blocks as well. - if let Some(ResolvedPath { did, .. }) = trait_ { - if Some(did) == cx.deref_trait_did.get() { - build_deref_target_impls(cx, &items, &mut ret); - } + if trait_.def_id() == cx.deref_trait_did.get() { + build_deref_target_impls(cx, &items, &mut ret); } + let provided = trait_.def_id().and_then(|did| { + cx.tcx_opt().map(|tcx| { + tcx.provided_trait_methods(did) + .into_iter() + .map(|meth| meth.name.to_string()) + .collect() + }) + }).unwrap_or(HashSet::new()); + ret.push(Item { name: None, attrs: self.attrs.clean(cx), @@ -2244,6 +2260,7 @@ impl Clean> for doctree::Impl { inner: ImplItem(Impl { unsafety: self.unsafety, generics: self.generics.clean(cx), + provided_trait_methods: provided, trait_: trait_, for_: self.for_.clean(cx), items: items, diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 071e3dd6bd010..51e069c66681e 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -62,7 +62,7 @@ use rustc::middle::stability; use rustc::session::config::get_unstable_features_setting; use rustc_front::hir; -use clean::{self, SelfTy, Attributes}; +use clean::{self, SelfTy, Attributes, GetDefId}; use doctree; use fold::DocFolder; use html::escape::Escape; @@ -144,9 +144,7 @@ pub struct Impl { impl Impl { fn trait_did(&self) -> Option { - self.impl_.trait_.as_ref().and_then(|tr| { - if let clean::ResolvedPath { did, .. } = *tr {Some(did)} else {None} - }) + self.impl_.trait_.def_id() } } @@ -967,7 +965,7 @@ impl DocFolder for Cache { // Collect all the implementors of traits. if let clean::ImplItem(ref i) = item.inner { - if let Some(clean::ResolvedPath{ did, .. }) = i.trait_ { + if let Some(did) = i.trait_.def_id() { self.implementors.entry(did).or_insert(vec![]).push(Implementor { def_id: item.def_id, stability: item.stability.clone(), @@ -2066,10 +2064,11 @@ fn render_stability_since(w: &mut fmt::Formatter, render_stability_since_raw(w, item.stable_since(), containing_item.stable_since()) } -fn render_assoc_item(w: &mut fmt::Formatter, meth: &clean::Item, +fn render_assoc_item(w: &mut fmt::Formatter, + item: &clean::Item, link: AssocItemLink) -> fmt::Result { fn method(w: &mut fmt::Formatter, - it: &clean::Item, + meth: &clean::Item, unsafety: hir::Unsafety, constness: hir::Constness, abi: abi::Abi, @@ -2080,12 +2079,20 @@ fn render_assoc_item(w: &mut fmt::Formatter, meth: &clean::Item, -> fmt::Result { use syntax::abi::Abi; - let name = it.name.as_ref().unwrap(); - let anchor = format!("#{}.{}", shortty(it), name); + let name = meth.name.as_ref().unwrap(); + let anchor = format!("#{}.{}", shortty(meth), name); let href = match link { AssocItemLink::Anchor => anchor, - AssocItemLink::GotoSource(did) => { - href(did).map(|p| format!("{}{}", p.0, anchor)).unwrap_or(anchor) + AssocItemLink::GotoSource(did, provided_methods) => { + // We're creating a link from an impl-item to the corresponding + // trait-item and need to map the anchored type accordingly. + let ty = if provided_methods.contains(name) { + ItemType::Method + } else { + ItemType::TyMethod + }; + + href(did).map(|p| format!("{}#{}.{}", p.0, ty, name)).unwrap_or(anchor) } }; let vis_constness = match get_unstable_features_setting() { @@ -2106,21 +2113,21 @@ fn render_assoc_item(w: &mut fmt::Formatter, meth: &clean::Item, decl = Method(selfty, d), where_clause = WhereClause(g)) } - match meth.inner { + match item.inner { clean::TyMethodItem(ref m) => { - method(w, meth, m.unsafety, hir::Constness::NotConst, + method(w, item, m.unsafety, hir::Constness::NotConst, m.abi, &m.generics, &m.self_, &m.decl, link) } clean::MethodItem(ref m) => { - method(w, meth, m.unsafety, m.constness, + method(w, item, m.unsafety, m.constness, m.abi, &m.generics, &m.self_, &m.decl, link) } clean::AssociatedConstItem(ref ty, ref default) => { - assoc_const(w, meth, ty, default.as_ref()) + assoc_const(w, item, ty, default.as_ref()) } clean::AssociatedTypeItem(ref bounds, ref default) => { - assoc_type(w, meth, bounds, default) + assoc_type(w, item, bounds, default) } _ => panic!("render_assoc_item called on non-associated-item") } @@ -2338,9 +2345,9 @@ fn render_struct(w: &mut fmt::Formatter, it: &clean::Item, } #[derive(Copy, Clone)] -enum AssocItemLink { +enum AssocItemLink<'a> { Anchor, - GotoSource(DefId), + GotoSource(DefId, &'a HashSet), } enum AssocItemRender<'a> { @@ -2383,12 +2390,7 @@ fn render_assoc_items(w: &mut fmt::Formatter, } if !traits.is_empty() { let deref_impl = traits.iter().find(|t| { - match *t.impl_.trait_.as_ref().unwrap() { - clean::ResolvedPath { did, .. } => { - Some(did) == c.deref_trait_did - } - _ => false - } + t.impl_.trait_.def_id() == c.deref_trait_did }); if let Some(impl_) = deref_impl { render_deref_methods(w, cx, impl_, containing_item)?; @@ -2400,8 +2402,8 @@ fn render_assoc_items(w: &mut fmt::Formatter, }); for i in &manual { let did = i.trait_did().unwrap(); - render_impl(w, cx, i, AssocItemLink::GotoSource(did), true, - containing_item.stable_since())?; + let assoc_link = AssocItemLink::GotoSource(did, &i.impl_.provided_trait_methods); + render_impl(w, cx, i, assoc_link, true, containing_item.stable_since())?; } if !derived.is_empty() { write!(w, "

\ @@ -2409,8 +2411,8 @@ fn render_assoc_items(w: &mut fmt::Formatter,

")?; for i in &derived { let did = i.trait_did().unwrap(); - render_impl(w, cx, i, AssocItemLink::GotoSource(did), true, - containing_item.stable_since())?; + let assoc_link = AssocItemLink::GotoSource(did, &i.impl_.provided_trait_methods); + render_impl(w, cx, i, assoc_link, true, containing_item.stable_since())?; } } } @@ -2427,17 +2429,16 @@ fn render_deref_methods(w: &mut fmt::Formatter, cx: &Context, impl_: &Impl, } }).next().expect("Expected associated type binding"); let what = AssocItemRender::DerefFor { trait_: deref_type, type_: target }; - match *target { - clean::ResolvedPath { did, .. } => render_assoc_items(w, cx, container_item, did, what), - _ => { - if let Some(prim) = target.primitive_type() { - if let Some(c) = cache().primitive_locations.get(&prim) { - let did = DefId { krate: *c, index: prim.to_def_index() }; - render_assoc_items(w, cx, container_item, did, what)?; - } + if let Some(did) = target.def_id() { + render_assoc_items(w, cx, container_item, did, what) + } else { + if let Some(prim) = target.primitive_type() { + if let Some(c) = cache().primitive_locations.get(&prim) { + let did = DefId { krate: *c, index: prim.to_def_index() }; + render_assoc_items(w, cx, container_item, did, what)?; } - Ok(()) } + Ok(()) } } @@ -2521,18 +2522,19 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi fn render_default_items(w: &mut fmt::Formatter, cx: &Context, - did: DefId, t: &clean::Trait, - i: &clean::Impl, - render_static: bool, - outer_version: Option<&str>) -> fmt::Result { + i: &clean::Impl, + render_static: bool, + outer_version: Option<&str>) -> fmt::Result { for trait_item in &t.items { let n = trait_item.name.clone(); - if i.items.iter().find(|m| { m.name == n }).is_some() { + if i.items.iter().find(|m| m.name == n).is_some() { continue; } + let did = i.trait_.as_ref().unwrap().def_id().unwrap(); + let assoc_link = AssocItemLink::GotoSource(did, &i.provided_trait_methods); - doctraititem(w, cx, trait_item, AssocItemLink::GotoSource(did), render_static, + doctraititem(w, cx, trait_item, assoc_link, render_static, outer_version)?; } Ok(()) @@ -2542,9 +2544,9 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi // default methods which weren't overridden in the implementation block. // FIXME: this also needs to be done for associated types, whenever defaults // for them work. - if let Some(clean::ResolvedPath { did, .. }) = i.impl_.trait_ { + if let Some(did) = i.trait_did() { if let Some(t) = cache().traits.get(&did) { - render_default_items(w, cx, did, t, &i.impl_, render_header, outer_version)?; + render_default_items(w, cx, t, &i.impl_, render_header, outer_version)?; } } write!(w, "")?; diff --git a/src/librustdoc/passes.rs b/src/librustdoc/passes.rs index 154b812cdff3e..88cb20991d660 100644 --- a/src/librustdoc/passes.rs +++ b/src/librustdoc/passes.rs @@ -16,7 +16,7 @@ use std::string::String; use std::usize; use rustc_front::hir; -use clean::{self, Attributes}; +use clean::{self, Attributes, GetDefId}; use clean::Item; use plugins; use fold; @@ -74,7 +74,7 @@ pub fn strip_hidden(krate: clean::Crate) -> plugins::PluginResult { return None; } // Impls of stripped traits also don't need to exist - if let Some(clean::ResolvedPath { did, .. }) = *trait_ { + if let Some(did) = trait_.def_id() { if self.stripped.contains(&did) { return None; } @@ -223,13 +223,10 @@ struct ImplStripper<'a>(&'a DefIdSet); impl<'a> fold::DocFolder for ImplStripper<'a> { fn fold_item(&mut self, i: Item) -> Option { if let clean::ImplItem(ref imp) = i.inner { - match imp.trait_ { - Some(clean::ResolvedPath{ did, .. }) => { - if did.is_local() && !self.0.contains(&did) { - return None; - } + if let Some(did) = imp.trait_.def_id() { + if did.is_local() && !self.0.contains(&did) { + return None; } - Some(..) | None => {} } } self.fold_item_recur(i) diff --git a/src/test/rustdoc/issue-28478.rs b/src/test/rustdoc/issue-28478.rs new file mode 100644 index 0000000000000..9d3433fb3997d --- /dev/null +++ b/src/test/rustdoc/issue-28478.rs @@ -0,0 +1,39 @@ +// Copyright 2016 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(associated_type_defaults)] +#![feature(associated_consts)] + +// @has issue_28478/trait.Bar.html +pub trait Bar { + // @has - '//*[@id="associatedtype.Bar"]' 'type Bar = ()' + type Bar = (); + + // @has - '//*[@id="associatedconstant.Baz"]' 'const Baz: usize = 7' + const Baz: usize = 7; + // @has - '//*[@id="tymethod.bar"]' 'fn bar' + fn bar(); + // @has - '//*[@id="method.baz"]' 'fn baz' + fn baz() { } +} + +// @has issue_28478/struct.Foo.html +pub struct Foo; + +impl Foo { + // @has - '//*[@href="#method.foo"]' 'foo' + pub fn foo() {} +} + +impl Bar for Foo { + // @has - '//*[@href="../issue_28478/trait.Bar.html#tymethod.bar"]' 'bar' + fn bar() {} + // @has - '//*[@href="../issue_28478/trait.Bar.html#method.baz"]' 'baz' +} From 1bd8183c1581bffb688e07113bd2a7ee494bf2fb Mon Sep 17 00:00:00 2001 From: mitaa Date: Thu, 24 Mar 2016 06:16:23 +0100 Subject: [PATCH 2/9] Don't hardcode item-type anchor ids These should always correspond to the values in `ItemType::to_static_str` --- src/librustdoc/html/render.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 51e069c66681e..24b84212a893c 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -2160,8 +2160,9 @@ fn item_struct(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item, write!(w, "

Fields

\n")?; for field in fields { write!(w, " -
\ + \ {name}", + shortty = ItemType::StructField, stab = field.stability_class(), name = field.name.as_ref().unwrap())?; document(w, cx, field)?; @@ -2231,7 +2232,8 @@ fn item_enum(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item, if !e.variants.is_empty() { write!(w, "

Variants

\n")?; for variant in &e.variants { - write!(w, "
{name}", + write!(w, "
{name}", + shortty = ItemType::Variant, name = variant.name.as_ref().unwrap())?; document(w, cx, variant)?; @@ -2247,8 +2249,9 @@ fn item_enum(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item, ")?; for field in fields { write!(w, "
\ + id='{shortty}.{v}.field.{f}'>\ {f}", + shortty = ItemType::Variant, v = variant.name.as_ref().unwrap(), f = field.name.as_ref().unwrap())?; document(w, cx, field)?; @@ -2460,6 +2463,7 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi fn doctraititem(w: &mut fmt::Formatter, cx: &Context, item: &clean::Item, link: AssocItemLink, render_static: bool, outer_version: Option<&str>) -> fmt::Result { + let shortty = shortty(item); let name = item.name.as_ref().unwrap(); let is_static = match item.inner { @@ -2472,8 +2476,8 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi clean::MethodItem(..) | clean::TyMethodItem(..) => { // Only render when the method is not static or we allow static methods if !is_static || render_static { - let id = derive_id(format!("method.{}", name)); - write!(w, "

", id, shortty(item))?; + let id = derive_id(format!("{}.{}", shortty, name)); + write!(w, "

", id, shortty)?; render_stability_since_raw(w, item.stable_since(), outer_version)?; write!(w, "")?; render_assoc_item(w, item, link)?; @@ -2481,26 +2485,26 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi } } clean::TypedefItem(ref tydef, _) => { - let id = derive_id(format!("associatedtype.{}", name)); - write!(w, "

", id, shortty(item))?; + let id = derive_id(format!("{}.{}", ItemType::AssociatedType, name)); + write!(w, "

", id, shortty)?; write!(w, "type {} = {}", name, tydef.type_)?; write!(w, "

\n")?; } clean::AssociatedConstItem(ref ty, ref default) => { - let id = derive_id(format!("associatedconstant.{}", name)); - write!(w, "

", id, shortty(item))?; + let id = derive_id(format!("{}.{}", shortty, name)); + write!(w, "

", id, shortty)?; assoc_const(w, item, ty, default.as_ref())?; write!(w, "

\n")?; } clean::ConstantItem(ref c) => { - let id = derive_id(format!("associatedconstant.{}", name)); - write!(w, "

", id, shortty(item))?; + let id = derive_id(format!("{}.{}", shortty, name)); + write!(w, "

", id, shortty)?; assoc_const(w, item, &c.type_, Some(&c.expr))?; write!(w, "

\n")?; } clean::AssociatedTypeItem(ref bounds, ref default) => { - let id = derive_id(format!("associatedtype.{}", name)); - write!(w, "

", id, shortty(item))?; + let id = derive_id(format!("{}.{}", shortty, name)); + write!(w, "

", id, shortty)?; assoc_type(w, item, bounds, default)?; write!(w, "

\n")?; } From 0e3a2c01679f1b86df01c739ce8299c687e5c7b6 Mon Sep 17 00:00:00 2001 From: mitaa Date: Fri, 25 Mar 2016 00:10:15 +0100 Subject: [PATCH 3/9] Linkify associated types and constants --- src/librustdoc/html/render.rs | 58 +++++++++++++++++++++++---------- src/test/rustdoc/issue-28478.rs | 5 ++- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 24b84212a893c..462cef2e0e3c6 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -2021,10 +2021,33 @@ fn item_trait(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item, Ok(()) } -fn assoc_const(w: &mut fmt::Formatter, it: &clean::Item, - ty: &clean::Type, default: Option<&String>) - -> fmt::Result { - write!(w, "const {}", it.name.as_ref().unwrap())?; +fn naive_assoc_href(it: &clean::Item, link: AssocItemLink) -> String { + use html::item_type::ItemType::*; + + let name = it.name.as_ref().unwrap(); + let ty = match shortty(it) { + Typedef | AssociatedType => AssociatedType, + s@_ => s, + }; + + let anchor = format!("#{}.{}", ty, name); + match link { + AssocItemLink::Anchor => anchor, + AssocItemLink::GotoSource(did, _) => { + href(did).map(|p| format!("{}{}", p.0, anchor)).unwrap_or(anchor) + } + } +} + +fn assoc_const(w: &mut fmt::Formatter, + it: &clean::Item, + ty: &clean::Type, + default: Option<&String>, + link: AssocItemLink) -> fmt::Result { + write!(w, "const {}", + naive_assoc_href(it, link), + it.name.as_ref().unwrap())?; + write!(w, ": {}", ty)?; if let Some(default) = default { write!(w, " = {}", default)?; @@ -2034,13 +2057,15 @@ fn assoc_const(w: &mut fmt::Formatter, it: &clean::Item, fn assoc_type(w: &mut fmt::Formatter, it: &clean::Item, bounds: &Vec, - default: &Option) - -> fmt::Result { - write!(w, "type {}", it.name.as_ref().unwrap())?; + default: Option<&clean::Type>, + link: AssocItemLink) -> fmt::Result { + write!(w, "type {}", + naive_assoc_href(it, link), + it.name.as_ref().unwrap())?; if !bounds.is_empty() { write!(w, ": {}", TyParamBounds(bounds))? } - if let Some(ref default) = *default { + if let Some(default) = default { write!(w, " = {}", default)?; } Ok(()) @@ -2095,6 +2120,7 @@ fn render_assoc_item(w: &mut fmt::Formatter, href(did).map(|p| format!("{}#{}.{}", p.0, ty, name)).unwrap_or(anchor) } }; + // FIXME(#24111): remove when `const_fn` is stabilized let vis_constness = match get_unstable_features_setting() { UnstableFeatures::Allow => constness, _ => hir::Constness::NotConst @@ -2124,10 +2150,10 @@ fn render_assoc_item(w: &mut fmt::Formatter, link) } clean::AssociatedConstItem(ref ty, ref default) => { - assoc_const(w, item, ty, default.as_ref()) + assoc_const(w, item, ty, default.as_ref(), link) } clean::AssociatedTypeItem(ref bounds, ref default) => { - assoc_type(w, item, bounds, default) + assoc_type(w, item, bounds, default.as_ref(), link) } _ => panic!("render_assoc_item called on non-associated-item") } @@ -2487,25 +2513,25 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi clean::TypedefItem(ref tydef, _) => { let id = derive_id(format!("{}.{}", ItemType::AssociatedType, name)); write!(w, "

", id, shortty)?; - write!(w, "type {} = {}", name, tydef.type_)?; + assoc_type(w, item, &Vec::new(), Some(&tydef.type_), link)?; write!(w, "

\n")?; } clean::AssociatedConstItem(ref ty, ref default) => { let id = derive_id(format!("{}.{}", shortty, name)); write!(w, "

", id, shortty)?; - assoc_const(w, item, ty, default.as_ref())?; + assoc_const(w, item, ty, default.as_ref(), link)?; write!(w, "

\n")?; } clean::ConstantItem(ref c) => { let id = derive_id(format!("{}.{}", shortty, name)); write!(w, "

", id, shortty)?; - assoc_const(w, item, &c.type_, Some(&c.expr))?; + assoc_const(w, item, &c.type_, Some(&c.expr), link)?; write!(w, "

\n")?; } clean::AssociatedTypeItem(ref bounds, ref default) => { let id = derive_id(format!("{}.{}", shortty, name)); write!(w, "

", id, shortty)?; - assoc_type(w, item, bounds, default)?; + assoc_type(w, item, bounds, default.as_ref(), link)?; write!(w, "

\n")?; } _ => panic!("can't make docs for trait item with name {:?}", item.name) @@ -2545,9 +2571,7 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi } // If we've implemented a trait, then also emit documentation for all - // default methods which weren't overridden in the implementation block. - // FIXME: this also needs to be done for associated types, whenever defaults - // for them work. + // default items which weren't overridden in the implementation block. if let Some(did) = i.trait_did() { if let Some(t) = cache().traits.get(&did) { render_default_items(w, cx, t, &i.impl_, render_header, outer_version)?; diff --git a/src/test/rustdoc/issue-28478.rs b/src/test/rustdoc/issue-28478.rs index 9d3433fb3997d..0db92a491ed18 100644 --- a/src/test/rustdoc/issue-28478.rs +++ b/src/test/rustdoc/issue-28478.rs @@ -14,9 +14,10 @@ // @has issue_28478/trait.Bar.html pub trait Bar { // @has - '//*[@id="associatedtype.Bar"]' 'type Bar = ()' + // @has - '//*[@href="#associatedtype.Bar"]' 'Bar' type Bar = (); - // @has - '//*[@id="associatedconstant.Baz"]' 'const Baz: usize = 7' + // @has - '//*[@href="#associatedconstant.Baz"]' 'Baz' const Baz: usize = 7; // @has - '//*[@id="tymethod.bar"]' 'fn bar' fn bar(); @@ -33,6 +34,8 @@ impl Foo { } impl Bar for Foo { + // @has - '//*[@href="../issue_28478/trait.Bar.html#associatedtype.Bar"]' 'Bar' + // @has - '//*[@href="../issue_28478/trait.Bar.html#associatedconstant.Baz"]' 'Baz' // @has - '//*[@href="../issue_28478/trait.Bar.html#tymethod.bar"]' 'bar' fn bar() {} // @has - '//*[@href="../issue_28478/trait.Bar.html#method.baz"]' 'baz' From d0f74b605942849b647b0e262fbda1969bd73c2a Mon Sep 17 00:00:00 2001 From: mitaa Date: Fri, 25 Mar 2016 02:18:57 +0100 Subject: [PATCH 4/9] Load struct-variant data correctly from metadata --- src/librustdoc/clean/mod.rs | 17 +++++------------ src/test/rustdoc/issue-32395.rs | 2 ++ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index bec3ae799cace..4143cdaf80cd1 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -1892,18 +1892,11 @@ impl<'tcx> Clean for ty::VariantDefData<'tcx, 'static> { Item { source: Span::empty(), name: Some(field.name.clean(cx)), - attrs: Vec::new(), + attrs: cx.tcx().get_attrs(field.did).clean(cx), visibility: Some(field.vis), - // FIXME: this is not accurate, we need an id for - // the specific field but we're using the id - // for the whole variant. Thus we read the - // stability from the whole variant as well. - // Struct variants are experimental and need - // more infrastructure work before we can get - // at the needed information here. - def_id: self.did, - stability: get_stability(cx, self.did), - deprecation: get_deprecation(cx, self.did), + def_id: field.did, + stability: get_stability(cx, field.did), + deprecation: get_deprecation(cx, field.did), inner: StructFieldItem( TypedStructField(field.unsubst_ty().clean(cx)) ) @@ -1916,7 +1909,7 @@ impl<'tcx> Clean for ty::VariantDefData<'tcx, 'static> { name: Some(self.name.clean(cx)), attrs: inline::load_attrs(cx, cx.tcx(), self.did), source: Span::empty(), - visibility: Some(hir::Public), + visibility: Some(hir::Inherited), def_id: self.did, inner: VariantItem(Variant { kind: kind }), stability: get_stability(cx, self.did), diff --git a/src/test/rustdoc/issue-32395.rs b/src/test/rustdoc/issue-32395.rs index fba2a6ad4877c..672c8757049d9 100644 --- a/src/test/rustdoc/issue-32395.rs +++ b/src/test/rustdoc/issue-32395.rs @@ -14,8 +14,10 @@ // @has variant_struct/enum.Foo.html // @!has - 'pub qux' +// @!has - 'pub Bar' extern crate variant_struct; // @has issue_32395/enum.Foo.html // @!has - 'pub qux' +// @!has - 'pub Bar' pub use variant_struct::Foo; From 6a76872d7165f901e3ec127a25be1a6303d137b3 Mon Sep 17 00:00:00 2001 From: mitaa Date: Sat, 26 Mar 2016 11:59:30 +0100 Subject: [PATCH 5/9] Extend linkchecker with anchor checking This adds checks to ensure that: * link anchors refer to existing id's on the target page * id's are unique within an html document * page redirects are valid --- src/doc/style/features/traits/generics.md | 5 +- src/libcore/iter.rs | 4 +- src/libstd/io/mod.rs | 2 +- src/libstd/lib.rs | 2 +- src/libstd/net/tcp.rs | 10 +- src/libstd/net/udp.rs | 16 +- src/tools/linkchecker/main.rs | 238 ++++++++++++++++++---- 7 files changed, 215 insertions(+), 62 deletions(-) diff --git a/src/doc/style/features/traits/generics.md b/src/doc/style/features/traits/generics.md index 26ffda50ac53d..a09640c3055c2 100644 --- a/src/doc/style/features/traits/generics.md +++ b/src/doc/style/features/traits/generics.md @@ -27,8 +27,7 @@ explicitly implement to be used by this generic function. * _Inference_. Since the type parameters to generic functions can usually be inferred, generic functions can help cut down on verbosity in code where explicit conversions or other method calls would usually be necessary. See the - [overloading/implicits use case](#use-case-limited-overloading-andor-implicit-conversions) - below. + overloading/implicits use case below. * _Precise types_. Because generics give a _name_ to the specific type implementing a trait, it is possible to be precise about places where that exact type is required or produced. For example, a function @@ -51,7 +50,7 @@ explicitly implement to be used by this generic function. a `Vec` contains elements of a single concrete type (and, indeed, the vector representation is specialized to lay these out in line). Sometimes heterogeneous collections are useful; see - [trait objects](#use-case-trait-objects) below. + trait objects below. * _Signature verbosity_. Heavy use of generics can bloat function signatures. **[Ed. note]** This problem may be mitigated by some language improvements; stay tuned. diff --git a/src/libcore/iter.rs b/src/libcore/iter.rs index 72421e94a4355..4dbcf7ab4e320 100644 --- a/src/libcore/iter.rs +++ b/src/libcore/iter.rs @@ -434,7 +434,7 @@ pub trait Iterator { /// `None`. Once `None` is encountered, `count()` returns the number of /// times it called [`next()`]. /// - /// [`next()`]: #method.next + /// [`next()`]: #tymethod.next /// /// # Overflow Behavior /// @@ -497,7 +497,7 @@ pub trait Iterator { /// This method will evaluate the iterator `n` times, discarding those elements. /// After it does so, it will call [`next()`] and return its value. /// - /// [`next()`]: #method.next + /// [`next()`]: #tymethod.next /// /// Like most indexing operations, the count starts from zero, so `nth(0)` /// returns the first value, `nth(1)` the second, and so on. diff --git a/src/libstd/io/mod.rs b/src/libstd/io/mod.rs index 28492b30e0f88..5362f7086bd8b 100644 --- a/src/libstd/io/mod.rs +++ b/src/libstd/io/mod.rs @@ -1254,7 +1254,7 @@ pub trait BufRead: Read { /// longer be returned. As such, this function may do odd things if /// `fill_buf` isn't called before calling it. /// - /// [fillbuf]: #tymethod.fill_buff + /// [fillbuf]: #tymethod.fill_buf /// /// The `amt` must be `<=` the number of bytes in the buffer returned by /// `fill_buf`. diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index 5608bb646a4b4..f7b1042592d85 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -190,7 +190,7 @@ //! [`thread`]: thread/index.html //! [`use std::env`]: env/index.html //! [`use`]: ../book/crates-and-modules.html#importing-modules-with-use -//! [crate root]: ../book/crates-and-modules.html#basic-terminology:-crates-and-modules +//! [crate root]: ../book/crates-and-modules.html#basic-terminology-crates-and-modules //! [crates.io]: https://crates.io //! [deref coercions]: ../book/deref-coercions.html //! [files]: fs/struct.File.html diff --git a/src/libstd/net/tcp.rs b/src/libstd/net/tcp.rs index 414696413f494..38da74b89039b 100644 --- a/src/libstd/net/tcp.rs +++ b/src/libstd/net/tcp.rs @@ -196,7 +196,7 @@ impl TcpStream { /// /// For more information about this option, see [`set_nodelay`][link]. /// - /// [link]: #tymethod.set_nodelay + /// [link]: #method.set_nodelay #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn nodelay(&self) -> io::Result { self.0.nodelay() @@ -215,7 +215,7 @@ impl TcpStream { /// /// For more information about this option, see [`set_ttl`][link]. /// - /// [link]: #tymethod.set_ttl + /// [link]: #method.set_ttl #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn ttl(&self) -> io::Result { self.0.ttl() @@ -238,7 +238,7 @@ impl TcpStream { /// /// For more information about this option, see [`set_only_v6`][link]. /// - /// [link]: #tymethod.set_only_v6 + /// [link]: #method.set_only_v6 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn only_v6(&self) -> io::Result { self.0.only_v6() @@ -374,7 +374,7 @@ impl TcpListener { /// /// For more information about this option, see [`set_ttl`][link]. /// - /// [link]: #tymethod.set_ttl + /// [link]: #method.set_ttl #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn ttl(&self) -> io::Result { self.0.ttl() @@ -397,7 +397,7 @@ impl TcpListener { /// /// For more information about this option, see [`set_only_v6`][link]. /// - /// [link]: #tymethod.set_only_v6 + /// [link]: #method.set_only_v6 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn only_v6(&self) -> io::Result { self.0.only_v6() diff --git a/src/libstd/net/udp.rs b/src/libstd/net/udp.rs index da1cf115e8d54..0be9f13e81761 100644 --- a/src/libstd/net/udp.rs +++ b/src/libstd/net/udp.rs @@ -155,7 +155,7 @@ impl UdpSocket { /// For more information about this option, see /// [`set_broadcast`][link]. /// - /// [link]: #tymethod.set_broadcast + /// [link]: #method.set_broadcast #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn broadcast(&self) -> io::Result { self.0.broadcast() @@ -175,7 +175,7 @@ impl UdpSocket { /// For more information about this option, see /// [`set_multicast_loop_v4`][link]. /// - /// [link]: #tymethod.set_multicast_loop_v4 + /// [link]: #method.set_multicast_loop_v4 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn multicast_loop_v4(&self) -> io::Result { self.0.multicast_loop_v4() @@ -198,7 +198,7 @@ impl UdpSocket { /// For more information about this option, see /// [`set_multicast_ttl_v4`][link]. /// - /// [link]: #tymethod.set_multicast_ttl_v4 + /// [link]: #method.set_multicast_ttl_v4 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn multicast_ttl_v4(&self) -> io::Result { self.0.multicast_ttl_v4() @@ -218,7 +218,7 @@ impl UdpSocket { /// For more information about this option, see /// [`set_multicast_loop_v6`][link]. /// - /// [link]: #tymethod.set_multicast_loop_v6 + /// [link]: #method.set_multicast_loop_v6 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn multicast_loop_v6(&self) -> io::Result { self.0.multicast_loop_v6() @@ -237,7 +237,7 @@ impl UdpSocket { /// /// For more information about this option, see [`set_ttl`][link]. /// - /// [link]: #tymethod.set_ttl + /// [link]: #method.set_ttl #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn ttl(&self) -> io::Result { self.0.ttl() @@ -260,7 +260,7 @@ impl UdpSocket { /// /// For more information about this option, see [`set_only_v6`][link]. /// - /// [link]: #tymethod.set_only_v6 + /// [link]: #method.set_only_v6 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn only_v6(&self) -> io::Result { self.0.only_v6() @@ -293,7 +293,7 @@ impl UdpSocket { /// For more information about this option, see /// [`join_multicast_v4`][link]. /// - /// [link]: #tymethod.join_multicast_v4 + /// [link]: #method.join_multicast_v4 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn leave_multicast_v4(&self, multiaddr: &Ipv4Addr, interface: &Ipv4Addr) -> io::Result<()> { self.0.leave_multicast_v4(multiaddr, interface) @@ -304,7 +304,7 @@ impl UdpSocket { /// For more information about this option, see /// [`join_multicast_v6`][link]. /// - /// [link]: #tymethod.join_multicast_v6 + /// [link]: #method.join_multicast_v6 #[stable(feature = "net2_mutators", since = "1.9.0")] pub fn leave_multicast_v6(&self, multiaddr: &Ipv6Addr, interface: u32) -> io::Result<()> { self.0.leave_multicast_v6(multiaddr, interface) diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index 19037a2c4d7f2..db87bc1fce4be 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -17,9 +17,9 @@ //! actually point to a valid place. //! //! Currently this doesn't actually do any HTML parsing or anything fancy like -//! that, it just has a simple "regex" to search for `href` tags. These values -//! are then translated to file URLs if possible and then the destination is -//! asserted to exist. +//! that, it just has a simple "regex" to search for `href` and `id` tags. +//! These values are then translated to file URLs if possible and then the +//! destination is asserted to exist. //! //! A few whitelisted exceptions are allowed as there's known bugs in rustdoc, //! but this should catch the majority of "broken link" cases. @@ -29,14 +29,16 @@ extern crate url; use std::env; use std::fs::File; use std::io::prelude::*; -use std::path::Path; +use std::path::{Path, PathBuf}; +use std::collections::{HashMap, HashSet}; +use std::collections::hash_map::Entry; use url::{Url, UrlParser}; macro_rules! t { ($e:expr) => (match $e { Ok(e) => e, - Err(e) => panic!("{} failed with {}", stringify!($e), e), + Err(e) => panic!("{} failed with {:?}", stringify!($e), e), }) } @@ -45,44 +47,68 @@ fn main() { let docs = env::current_dir().unwrap().join(docs); let mut url = Url::from_file_path(&docs).unwrap(); let mut errors = false; - walk(&docs, &docs, &mut url, &mut errors); + walk(&mut HashMap::new(), &docs, &docs, &mut url, &mut errors); if errors { panic!("found some broken links"); } } -fn walk(root: &Path, dir: &Path, url: &mut Url, errors: &mut bool) { +#[derive(Debug)] +pub enum LoadError { + IOError(std::io::Error), + BrokenRedirect(PathBuf, std::io::Error), +} + +struct FileEntry { + source: String, + ids: HashSet, +} + +type Cache = HashMap; + +fn walk(cache: &mut Cache, + root: &Path, + dir: &Path, + url: &mut Url, + errors: &mut bool) +{ for entry in t!(dir.read_dir()).map(|e| t!(e)) { let path = entry.path(); let kind = t!(entry.file_type()); url.path_mut().unwrap().push(entry.file_name().into_string().unwrap()); if kind.is_dir() { - walk(root, &path, url, errors); + walk(cache, root, &path, url, errors); } else { - check(root, &path, url, errors); + check(cache, root, &path, url, errors); } url.path_mut().unwrap().pop(); } } -fn check(root: &Path, file: &Path, base: &Url, errors: &mut bool) { +fn check(cache: &mut Cache, + root: &Path, + file: &Path, + base: &Url, + errors: &mut bool) +{ // ignore js files as they are not prone to errors as the rest of the // documentation is and they otherwise bring up false positives. if file.extension().and_then(|s| s.to_str()) == Some("js") { return } - let pretty_file = file.strip_prefix(root).unwrap_or(file); - // Unfortunately we're not 100% full of valid links today to we need a few // whitelists to get this past `make check` today. // FIXME(#32129) - if file.ends_with("std/string/struct.String.html") { + if file.ends_with("std/string/struct.String.html") || + file.ends_with("collections/string/struct.String.html") { return } // FIXME(#32130) if file.ends_with("btree_set/struct.BTreeSet.html") || - file.ends_with("collections/struct.BTreeSet.html") { + file.ends_with("collections/struct.BTreeSet.html") || + file.ends_with("collections/btree_map/struct.BTreeMap.html") || + file.ends_with("collections/hash_map/struct.HashMap.html") { return } @@ -104,15 +130,159 @@ fn check(root: &Path, file: &Path, base: &Url, errors: &mut bool) { let mut parser = UrlParser::new(); parser.base_url(base); + + let res = load_file(cache, root, PathBuf::from(file), false, false); + let (pretty_file, contents) = match res { + Ok(res) => res, + Err(_) => return, + }; + + // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' + with_attrs_in_source(&contents, " href", |url, i| { + // Once we've plucked out the URL, parse it using our base url and + // then try to extract a file path. If either of these fail then we + // just keep going. + let (parsed_url, path) = match url_to_file_path(&parser, url) { + Some((url, path)) => (url, PathBuf::from(path)), + None => return, + }; + + // Alright, if we've found a file name then this file had better + // exist! If it doesn't then we register and print an error. + if path.exists() { + if path.is_dir() { + return; + } + let res = load_file(cache, root, path.clone(), true, false); + let (pretty_path, contents) = match res { + Ok(res) => res, + Err(LoadError::IOError(err)) => panic!(format!("{}", err)), + Err(LoadError::BrokenRedirect(target, _)) => { + print!("{}:{}: broken redirect to {}", + pretty_file.display(), i + 1, target.display()); + return; + } + }; + + if let Some(ref fragment) = parsed_url.fragment { + // Fragments like `#1-6` are most likely line numbers to be + // interpreted by javascript, so we're ignoring these + if fragment.splitn(2, '-') + .all(|f| f.chars().all(|c| c.is_numeric())) { + return; + } + + let ids = &mut cache.get_mut(&pretty_path).unwrap().ids; + if ids.is_empty() { + // Search for anything that's the regex 'id[ ]*=[ ]*".*?"' + with_attrs_in_source(&contents, " id", |fragment, i| { + let frag = fragment.trim_left_matches("#").to_owned(); + if !ids.insert(frag) { + *errors = true; + println!("{}:{}: id is not unique: `{}`", + pretty_file.display(), i, fragment); + } + }); + } + if !ids.contains(fragment) { + *errors = true; + print!("{}:{}: broken link fragment ", + pretty_file.display(), i + 1); + println!("`#{}` pointing to `{}`", + fragment, pretty_path.display()); + }; + } + } else { + *errors = true; + print!("{}:{}: broken link - ", pretty_file.display(), i + 1); + let pretty_path = path.strip_prefix(root).unwrap_or(&path); + println!("{}", pretty_path.display()); + } + }); +} + +fn load_file(cache: &mut Cache, + root: &Path, + file: PathBuf, + follow_redirects: bool, + is_redirect: bool) -> Result<(PathBuf, String), LoadError> { + let mut contents = String::new(); - if t!(File::open(file)).read_to_string(&mut contents).is_err() { - return + let pretty_file = PathBuf::from(file.strip_prefix(root).unwrap_or(&file)); + + let maybe_redirect = match cache.entry(pretty_file.clone()) { + Entry::Occupied(entry) => { + contents = entry.get().source.clone(); + None + }, + Entry::Vacant(entry) => { + let mut fp = try!(File::open(file.clone()).map_err(|err| { + if is_redirect { + LoadError::BrokenRedirect(file.clone(), err) + } else { + LoadError::IOError(err) + } + })); + try!(fp.read_to_string(&mut contents) + .map_err(|err| LoadError::IOError(err))); + + let maybe = if follow_redirects { + maybe_redirect(&contents) + } else { + None + }; + if maybe.is_none() { + entry.insert(FileEntry { + source: contents.clone(), + ids: HashSet::new(), + }); + } + maybe + }, + }; + let base = Url::from_file_path(&file).unwrap(); + let mut parser = UrlParser::new(); + parser.base_url(&base); + + match maybe_redirect.and_then(|url| url_to_file_path(&parser, &url)) { + Some((_, redirect_file)) => { + assert!(follow_redirects); + let path = PathBuf::from(redirect_file); + load_file(cache, root, path, follow_redirects, true) + } + None => Ok((pretty_file, contents)) } +} + +fn maybe_redirect(source: &str) -> Option { + const REDIRECT: &'static str = "

Redirecting to Option<(Url, PathBuf)> { + parser.parse(url).ok().and_then(|parsed_url| { + parsed_url.to_file_path().ok().map(|f| (parsed_url, f)) + }) +} + +fn with_attrs_in_source(contents: &str, + attr: &str, + mut f: F) +{ for (i, mut line) in contents.lines().enumerate() { - // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' - while let Some(j) = line.find(" href") { - let rest = &line[j + 5..]; + while let Some(j) = line.find(attr) { + let rest = &line[j + attr.len() ..]; line = rest; let pos_equals = match rest.find("=") { Some(i) => i, @@ -121,40 +291,24 @@ fn check(root: &Path, file: &Path, base: &Url, errors: &mut bool) { if rest[..pos_equals].trim_left_matches(" ") != "" { continue } + let rest = &rest[pos_equals + 1..]; - let pos_quote = match rest.find("\"").or_else(|| rest.find("'")) { + + let pos_quote = match rest.find(&['"', '\''][..]) { Some(i) => i, None => continue, }; + let quote_delim = rest.as_bytes()[pos_quote] as char; + if rest[..pos_quote].trim_left_matches(" ") != "" { continue } let rest = &rest[pos_quote + 1..]; - let url = match rest.find("\"").or_else(|| rest.find("'")) { + let url = match rest.find(quote_delim) { Some(i) => &rest[..i], None => continue, }; - - // Once we've plucked out the URL, parse it using our base url and - // then try to extract a file path. If either if these fail then we - // just keep going. - let parsed_url = match parser.parse(url) { - Ok(url) => url, - Err(..) => continue, - }; - let path = match parsed_url.to_file_path() { - Ok(path) => path, - Err(..) => continue, - }; - - // Alright, if we've found a file name then this file had better - // exist! If it doesn't then we register and print an error. - if !path.exists() { - *errors = true; - print!("{}:{}: broken link - ", pretty_file.display(), i + 1); - let pretty_path = path.strip_prefix(root).unwrap_or(&path); - println!("{}", pretty_path.display()); - } + f(url, i) } } } From 8779e7baa495b8dc46008c8f7d2badcb0ea3b1ab Mon Sep 17 00:00:00 2001 From: mitaa Date: Sat, 26 Mar 2016 16:42:38 +0100 Subject: [PATCH 6/9] Don't initialize id-map when rendering md files Adding these "known" values to the table of used ids is only required when embedding markdown into a rustdoc html page and may yield unexpected results when rendering a standalone `*.md` file. --- src/librustdoc/html/markdown.rs | 6 +++--- src/librustdoc/html/render.rs | 14 ++++++++++---- src/librustdoc/markdown.rs | 2 +- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 9fd476bfbab55..e7304b6951083 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -607,7 +607,7 @@ mod tests { fn issue_17736() { let markdown = "# title"; format!("{}", Markdown(markdown)); - reset_ids(); + reset_ids(true); } #[test] @@ -615,7 +615,7 @@ mod tests { fn t(input: &str, expect: &str) { let output = format!("{}", Markdown(input)); assert_eq!(output, expect); - reset_ids(); + reset_ids(true); } t("# Foo bar", "\n

\ @@ -654,7 +654,7 @@ mod tests { Panics

"); }; test(); - reset_ids(); + reset_ids(true); test(); } diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 462cef2e0e3c6..0f436efd70b2e 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -378,8 +378,14 @@ fn init_ids() -> HashMap { /// This method resets the local table of used ID attributes. This is typically /// used at the beginning of rendering an entire HTML page to reset from the /// previous state (if any). -pub fn reset_ids() { - USED_ID_MAP.with(|s| *s.borrow_mut() = init_ids()); +pub fn reset_ids(embedded: bool) { + USED_ID_MAP.with(|s| { + *s.borrow_mut() = if embedded { + init_ids() + } else { + HashMap::new() + }; + }); } pub fn derive_id(candidate: String) -> String { @@ -1280,7 +1286,7 @@ impl Context { keywords: &keywords, }; - reset_ids(); + reset_ids(true); // We have a huge number of calls to write, so try to alleviate some // of the pain by using a buffered writer instead of invoking the @@ -2748,6 +2754,6 @@ fn test_unique_id() { assert_eq!(&actual[..], expected); }; test(); - reset_ids(); + reset_ids(true); test(); } diff --git a/src/librustdoc/markdown.rs b/src/librustdoc/markdown.rs index 03d2c1a1b4d0d..d21726dd40f08 100644 --- a/src/librustdoc/markdown.rs +++ b/src/librustdoc/markdown.rs @@ -83,7 +83,7 @@ pub fn render(input: &str, mut output: PathBuf, matches: &getopts::Matches, } let title = metadata[0]; - reset_ids(); + reset_ids(false); let rendered = if include_toc { format!("{}", MarkdownWithToc(text)) From 41916d860179f4715fd0f588c1b2342a223a4186 Mon Sep 17 00:00:00 2001 From: mitaa Date: Sat, 26 Mar 2016 21:11:15 +0100 Subject: [PATCH 7/9] Drop cached sources to reduce memory usage --- src/tools/linkchecker/main.rs | 66 +++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index db87bc1fce4be..aed710e2b8edc 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -66,6 +66,25 @@ struct FileEntry { type Cache = HashMap; +impl FileEntry { + fn parse_ids(&mut self, + file: &Path, + contents: &str, + errors: &mut bool) +{ + if self.ids.is_empty() { + with_attrs_in_source(contents, " id", |fragment, i| { + let frag = fragment.trim_left_matches("#").to_owned(); + if !self.ids.insert(frag) { + *errors = true; + println!("{}:{}: id is not unique: `{}`", + file.display(), i, fragment); + } + }); + } + } +} + fn walk(cache: &mut Cache, root: &Path, dir: &Path, @@ -79,7 +98,13 @@ fn walk(cache: &mut Cache, if kind.is_dir() { walk(cache, root, &path, url, errors); } else { - check(cache, root, &path, url, errors); + let pretty_path = check(cache, root, &path, url, errors); + if let Some(pretty_path) = pretty_path { + let entry = cache.get_mut(&pretty_path).unwrap(); + // we don't need the source anymore, + // so drop to to reduce memory-usage + entry.source = String::new(); + } } url.path_mut().unwrap().pop(); } @@ -89,12 +114,12 @@ fn check(cache: &mut Cache, root: &Path, file: &Path, base: &Url, - errors: &mut bool) + errors: &mut bool) -> Option { // ignore js files as they are not prone to errors as the rest of the // documentation is and they otherwise bring up false positives. if file.extension().and_then(|s| s.to_str()) == Some("js") { - return + return None; } // Unfortunately we're not 100% full of valid links today to we need a few @@ -102,29 +127,29 @@ fn check(cache: &mut Cache, // FIXME(#32129) if file.ends_with("std/string/struct.String.html") || file.ends_with("collections/string/struct.String.html") { - return + return None; } // FIXME(#32130) if file.ends_with("btree_set/struct.BTreeSet.html") || file.ends_with("collections/struct.BTreeSet.html") || file.ends_with("collections/btree_map/struct.BTreeMap.html") || file.ends_with("collections/hash_map/struct.HashMap.html") { - return + return None; } if file.ends_with("std/sys/ext/index.html") { - return + return None; } if let Some(file) = file.to_str() { // FIXME(#31948) if file.contains("ParseFloatError") { - return + return None; } // weird reexports, but this module is on its way out, so chalk it up to // "rustdoc weirdness" and move on from there if file.contains("scoped_tls") { - return + return None; } } @@ -134,8 +159,12 @@ fn check(cache: &mut Cache, let res = load_file(cache, root, PathBuf::from(file), false, false); let (pretty_file, contents) = match res { Ok(res) => res, - Err(_) => return, + Err(_) => return None, }; + { + cache.get_mut(&pretty_file).unwrap() + .parse_ids(&pretty_file, &contents, errors); + } // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' with_attrs_in_source(&contents, " href", |url, i| { @@ -172,19 +201,10 @@ fn check(cache: &mut Cache, return; } - let ids = &mut cache.get_mut(&pretty_path).unwrap().ids; - if ids.is_empty() { - // Search for anything that's the regex 'id[ ]*=[ ]*".*?"' - with_attrs_in_source(&contents, " id", |fragment, i| { - let frag = fragment.trim_left_matches("#").to_owned(); - if !ids.insert(frag) { - *errors = true; - println!("{}:{}: id is not unique: `{}`", - pretty_file.display(), i, fragment); - } - }); - } - if !ids.contains(fragment) { + let entry = &mut cache.get_mut(&pretty_path).unwrap(); + entry.parse_ids(&pretty_path, &contents, errors); + + if !entry.ids.contains(fragment) { *errors = true; print!("{}:{}: broken link fragment ", pretty_file.display(), i + 1); @@ -199,6 +219,7 @@ fn check(cache: &mut Cache, println!("{}", pretty_path.display()); } }); + Some(pretty_file) } fn load_file(cache: &mut Cache, @@ -206,7 +227,6 @@ fn load_file(cache: &mut Cache, file: PathBuf, follow_redirects: bool, is_redirect: bool) -> Result<(PathBuf, String), LoadError> { - let mut contents = String::new(); let pretty_file = PathBuf::from(file.strip_prefix(root).unwrap_or(&file)); From b01da7282dc5b71cfe78d4cd54c664bcbb13b393 Mon Sep 17 00:00:00 2001 From: mitaa Date: Mon, 28 Mar 2016 14:31:57 +0200 Subject: [PATCH 8/9] Don't check(=cache) redirect pages Checking a redirect page during tree traversal before trying to actually follow the redirect leads to retrieval of the redirect pages source instead. --- src/tools/linkchecker/main.rs | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index aed710e2b8edc..7116cb01d6bb4 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -35,6 +35,8 @@ use std::collections::hash_map::Entry; use url::{Url, UrlParser}; +use Redirect::*; + macro_rules! t { ($e:expr) => (match $e { Ok(e) => e, @@ -57,6 +59,12 @@ fn main() { pub enum LoadError { IOError(std::io::Error), BrokenRedirect(PathBuf, std::io::Error), + IsRedirect, +} + +enum Redirect { + SkipRedirect, + FromRedirect(bool), } struct FileEntry { @@ -156,7 +164,7 @@ fn check(cache: &mut Cache, let mut parser = UrlParser::new(); parser.base_url(base); - let res = load_file(cache, root, PathBuf::from(file), false, false); + let res = load_file(cache, root, PathBuf::from(file), SkipRedirect); let (pretty_file, contents) = match res { Ok(res) => res, Err(_) => return None, @@ -182,7 +190,7 @@ fn check(cache: &mut Cache, if path.is_dir() { return; } - let res = load_file(cache, root, path.clone(), true, false); + let res = load_file(cache, root, path.clone(), FromRedirect(false)); let (pretty_path, contents) = match res { Ok(res) => res, Err(LoadError::IOError(err)) => panic!(format!("{}", err)), @@ -191,6 +199,7 @@ fn check(cache: &mut Cache, pretty_file.display(), i + 1, target.display()); return; } + Err(LoadError::IsRedirect) => unreachable!(), }; if let Some(ref fragment) = parsed_url.fragment { @@ -225,8 +234,7 @@ fn check(cache: &mut Cache, fn load_file(cache: &mut Cache, root: &Path, file: PathBuf, - follow_redirects: bool, - is_redirect: bool) -> Result<(PathBuf, String), LoadError> { + redirect: Redirect) -> Result<(PathBuf, String), LoadError> { let mut contents = String::new(); let pretty_file = PathBuf::from(file.strip_prefix(root).unwrap_or(&file)); @@ -237,7 +245,7 @@ fn load_file(cache: &mut Cache, }, Entry::Vacant(entry) => { let mut fp = try!(File::open(file.clone()).map_err(|err| { - if is_redirect { + if let FromRedirect(true) = redirect { LoadError::BrokenRedirect(file.clone(), err) } else { LoadError::IOError(err) @@ -246,12 +254,12 @@ fn load_file(cache: &mut Cache, try!(fp.read_to_string(&mut contents) .map_err(|err| LoadError::IOError(err))); - let maybe = if follow_redirects { - maybe_redirect(&contents) + let maybe = maybe_redirect(&contents); + if maybe.is_some() { + if let SkipRedirect = redirect { + return Err(LoadError::IsRedirect); + } } else { - None - }; - if maybe.is_none() { entry.insert(FileEntry { source: contents.clone(), ids: HashSet::new(), @@ -266,9 +274,8 @@ fn load_file(cache: &mut Cache, match maybe_redirect.and_then(|url| url_to_file_path(&parser, &url)) { Some((_, redirect_file)) => { - assert!(follow_redirects); let path = PathBuf::from(redirect_file); - load_file(cache, root, path, follow_redirects, true) + load_file(cache, root, path, FromRedirect(true)) } None => Ok((pretty_file, contents)) } From 4e0abdb021a704842b3c2702b533643fafa166df Mon Sep 17 00:00:00 2001 From: mitaa Date: Mon, 28 Mar 2016 15:41:22 +0200 Subject: [PATCH 9/9] Add FIXME for linkchecker whitlists --- src/tools/linkchecker/main.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index 7116cb01d6bb4..12419d4f7e5f7 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -133,8 +133,11 @@ fn check(cache: &mut Cache, // Unfortunately we're not 100% full of valid links today to we need a few // whitelists to get this past `make check` today. // FIXME(#32129) - if file.ends_with("std/string/struct.String.html") || - file.ends_with("collections/string/struct.String.html") { + if file.ends_with("std/string/struct.String.html") { + return None; + } + // FIXME(#32553) + if file.ends_with("collections/string/struct.String.html") { return None; } // FIXME(#32130)