Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Small code improvements in collect_intra_doc_links.rs #112806

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Small code improvements in collect_intra_doc_links.rs #112806

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 39 additions & 64 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,13 @@ impl Res {
Res::Primitive(_) => return Suggestion::Prefix("prim"),
Res::Def(kind, _) => kind,
};
if kind == DefKind::Macro(MacroKind::Bang) {
return Suggestion::Macro;
} else if kind == DefKind::Fn || kind == DefKind::AssocFn {
return Suggestion::Function;
} else if kind == DefKind::Field {
return Suggestion::RemoveDisambiguator;
}

let prefix = match kind {
DefKind::Fn | DefKind::AssocFn => return Suggestion::Function,
DefKind::Field => return Suggestion::RemoveDisambiguator,
DefKind::Macro(MacroKind::Bang) => return Suggestion::Macro,
Comment on lines -111 to +115
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the if-else blocks into the match block right below


DefKind::Macro(MacroKind::Derive) => "derive",
DefKind::Struct => "struct",
DefKind::Enum => "enum",
DefKind::Trait => "trait",
Expand All @@ -126,7 +124,6 @@ impl Res {
"const"
}
DefKind::Static(_) => "static",
DefKind::Macro(MacroKind::Derive) => "derive",
// Now handle things that don't have a specific disambiguator
_ => match kind
.ns()
Expand Down Expand Up @@ -283,20 +280,15 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {

debug!("looking for enum variant {path_str}");
let mut split = path_str.rsplitn(3, "::");
let variant_field_name = split
.next()
.map(|f| Symbol::intern(f))
.expect("fold_item should ensure link is non-empty");
let variant_name =
// we're not sure this is a variant at all, so use the full string
// If there's no second component, the link looks like `[path]`.
// So there's no partial res and we should say the whole link failed to resolve.
split.next().map(|f| Symbol::intern(f)).ok_or_else(no_res)?;
let path = split
.next()
// If there's no third component, we saw `[a::b]` before and it failed to resolve.
// So there's no partial res.
.ok_or_else(no_res)?;
let variant_field_name = Symbol::intern(split.next().unwrap());
// We're not sure this is a variant at all, so use the full string.
// If there's no second component, the link looks like `[path]`.
// So there's no partial res and we should say the whole link failed to resolve.
let variant_name = Symbol::intern(split.next().ok_or_else(no_res)?);

// If there's no third component, we saw `[a::b]` before and it failed to resolve.
// So there's no partial res.
let path = split.next().ok_or_else(no_res)?;
Comment on lines -286 to +291
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the comments above the statement which they're describing (one reason that's better is because they previously prevented rustfmt from working for that statement) and remove the misleading .expect(…) string. Also get rid of a .map(…) call.

let ty_res = self.resolve_path(&path, TypeNS, item_id, module_id).ok_or_else(no_res)?;

match ty_res {
Expand Down Expand Up @@ -447,41 +439,29 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
}

// Try looking for methods and associated items.
let mut split = path_str.rsplitn(2, "::");
// NB: `split`'s first element is always defined, even if the delimiter was not present.
// NB: `item_str` could be empty when resolving in the root namespace (e.g. `::std`).
let item_str = split.next().unwrap();
let item_name = Symbol::intern(item_str);
let path_root = split
.next()
// NB: `path_root` could be empty when resolving in the root namespace (e.g. `::std`).
let (path_root, item_str) = path_str.rsplit_once("::").ok_or_else(|| {
// If there's no `::`, it's not an associated item.
// So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved.
.ok_or_else(|| {
debug!("found no `::`, assuming {item_name} was correctly not in scope");
UnresolvedPath {
item_id,
module_id,
partial_res: None,
unresolved: item_str.into(),
}
})?;
debug!("found no `::`, assuming {path_str} was correctly not in scope");
UnresolvedPath { item_id, module_id, partial_res: None, unresolved: path_str.into() }
})?;
let item_name = Symbol::intern(item_str);

// FIXME(#83862): this arbitrarily gives precedence to primitives over modules to support
// links to primitives when `#[rustc_doc_primitive]` is present. It should give an ambiguity
// error instead and special case *only* modules with `#[rustc_doc_primitive]`, not all
// primitives.
match resolve_primitive(&path_root, TypeNS)
.or_else(|| self.resolve_path(&path_root, TypeNS, item_id, module_id))
.and_then(|ty_res| {
let candidates = self
.resolve_associated_item(ty_res, item_name, ns, module_id)
match resolve_primitive(path_root, TypeNS)
.or_else(|| self.resolve_path(path_root, TypeNS, item_id, module_id))
.map(|ty_res| {
self.resolve_associated_item(ty_res, item_name, ns, module_id)
.into_iter()
.map(|(res, def_id)| (res, Some(def_id)))
.collect::<Vec<_>>();
if !candidates.is_empty() { Some(candidates) } else { None }
.collect::<Vec<_>>()
}) {
Some(r) => Ok(r),
None => {
Some(r) if !r.is_empty() => Ok(r),
_ => {
Comment on lines -473 to +464
Copy link
Contributor Author

@kadiwa4 kadiwa4 Jun 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line

if !candidates.is_empty() { Some(candidates) } else { None }

inside of and_then is just a .filter(…) condition with extra steps and can be moved into the match arm below.

if ns == Namespace::ValueNS {
self.variant_field(path_str, item_id, module_id)
.map(|(res, def_id)| vec![(res, Some(def_id))])
Expand Down Expand Up @@ -1262,16 +1242,18 @@ impl LinkCollector<'_, '_> {
self.report_rawptr_assoc_feature_gate(diag.dox, &diag.link_range, diag.item);
return None;
} else {
candidates = vec![candidates[0]];
candidates = vec![*candidate];
}
}

// If there are multiple items with the same "kind" (for example, both "associated types")
// and after removing duplicated kinds, only one remains, the `ambiguity_error` function
// won't emit an error. So at this point, we can just take the first candidate as it was
// the first retrieved and use it to generate the link.
if candidates.len() > 1 && !ambiguity_error(self.cx, &diag, &key.path_str, &candidates) {
candidates = vec![candidates[0]];
if let [candidate, _candidate2, ..] = *candidates
&& !ambiguity_error(self.cx, &diag, &key.path_str, &candidates)
{
candidates = vec![candidate];
Comment on lines -1273 to +1256
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pattern matching instead of checking .len() and then doing bounds-checked indexing.

}

if let &[(res, def_id)] = candidates.as_slice() {
Expand Down Expand Up @@ -1321,12 +1303,11 @@ impl LinkCollector<'_, '_> {
let mut err = ResolutionFailure::NotResolved(err);
for other_ns in [TypeNS, ValueNS, MacroNS] {
if other_ns != expected_ns {
if let Ok(res) =
self.resolve(path_str, other_ns, item_id, module_id)
&& !res.is_empty()
if let Ok(&[res, ..]) =
self.resolve(path_str, other_ns, item_id, module_id).as_deref()
{
err = ResolutionFailure::WrongNamespace {
res: full_res(self.cx.tcx, res[0]),
res: full_res(self.cx.tcx, res),
expected_ns,
};
break;
Expand Down Expand Up @@ -1747,7 +1728,6 @@ fn report_diagnostic(
lint.note(format!(
"the link appears in this line:\n\n{line}\n\
{indicator: <before$}{indicator:^<found$}",
line = line,
indicator = "",
before = md_range.start - last_new_line_offset,
found = md_range.len(),
Expand Down Expand Up @@ -1808,18 +1788,13 @@ fn resolution_failure(

let item_id = *item_id;
let module_id = *module_id;
// FIXME(jynelson): this might conflict with my `Self` fix in #76467
// FIXME: maybe use itertools `collect_tuple` instead?
fn split(path: &str) -> Option<(&str, &str)> {
let mut splitter = path.rsplitn(2, "::");
splitter.next().and_then(|right| splitter.next().map(|left| (left, right)))
}

// Check if _any_ parent of the path gets resolved.
// If so, report it and say the first which failed; if not, say the first path segment didn't resolve.
let mut name = path_str;
'outer: loop {
let Some((start, end)) = split(name) else {
// FIXME(jynelson): this might conflict with my `Self` fix in #76467
let Some((start, end)) = name.rsplit_once("::") else {
Comment on lines -1822 to +1797
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper function split already exists in the standard library: rsplit_once behaves the same way.

// avoid bug that marked [Quux::Z] as missing Z, not Quux
if partial_res.is_none() {
*unresolved = name.into();
Expand All @@ -1830,8 +1805,8 @@ fn resolution_failure(
for ns in [TypeNS, ValueNS, MacroNS] {
if let Ok(v_res) = collector.resolve(start, ns, item_id, module_id) {
debug!("found partial_res={v_res:?}");
if !v_res.is_empty() {
*partial_res = Some(full_res(tcx, v_res[0]));
if let Some(&res) = v_res.first() {
*partial_res = Some(full_res(tcx, res));
Comment on lines -1833 to +1809
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pattern matching instead of checking .is_empty() and then doing bounds-checked indexing.

*unresolved = end.into();
break 'outer;
}
Expand Down
Loading