Skip to content

Commit 895fa9c

Browse files
committed
Extract functions for two closures
These closures were quite complex and part of a quite complex function. The fact that they are closures makes mistakes likely when refactoring. For example, earlier, I meant to use `resolved`, an argument of the closure, but I instead typed `res`, which captured a local variable and caused a subtle bug that led to a confusing test failure. Extracting them as functions makes the code easier to understand and refactor.
1 parent a5f09f7 commit 895fa9c

File tree

1 file changed

+107
-73
lines changed

1 file changed

+107
-73
lines changed

src/librustdoc/passes/collect_intra_doc_links.rs

Lines changed: 107 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,79 +1305,9 @@ impl LinkCollector<'_, '_> {
13051305
}
13061306
}
13071307

1308-
let report_mismatch = |specified: Disambiguator, resolved: Res| {
1309-
// The resolved item did not match the disambiguator; give a better error than 'not found'
1310-
let msg = format!("incompatible link kind for `{}`", path_str);
1311-
let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option<rustc_span::Span>| {
1312-
let note = format!(
1313-
"this link resolved to {} {}, which is not {} {}",
1314-
resolved.article(),
1315-
resolved.descr(),
1316-
specified.article(),
1317-
specified.descr(),
1318-
);
1319-
if let Some(sp) = sp {
1320-
diag.span_label(sp, &note);
1321-
} else {
1322-
diag.note(&note);
1323-
}
1324-
suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp);
1325-
};
1326-
report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback);
1327-
};
1328-
1329-
let verify = |kind: DefKind, id: DefId| {
1330-
let (kind, id) = if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment {
1331-
(self.cx.tcx.def_kind(id), id)
1332-
} else {
1333-
(kind, id)
1334-
};
1335-
debug!("intra-doc link to {} resolved to {:?} (id: {:?})", path_str, res, id);
1336-
1337-
// Disallow e.g. linking to enums with `struct@`
1338-
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
1339-
match (kind, disambiguator) {
1340-
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const)))
1341-
// NOTE: this allows 'method' to mean both normal functions and associated functions
1342-
// This can't cause ambiguity because both are in the same namespace.
1343-
| (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn)))
1344-
// These are namespaces; allow anything in the namespace to match
1345-
| (_, Some(Disambiguator::Namespace(_)))
1346-
// If no disambiguator given, allow anything
1347-
| (_, None)
1348-
// All of these are valid, so do nothing
1349-
=> {}
1350-
(actual, Some(Disambiguator::Kind(expected))) if actual == expected => {}
1351-
(_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => {
1352-
report_mismatch(specified, Res::Def(kind, id));
1353-
return None;
1354-
}
1355-
}
1356-
1357-
// item can be non-local e.g. when using #[doc(primitive = "pointer")]
1358-
if let Some((src_id, dst_id)) = id
1359-
.as_local()
1360-
// The `expect_def_id()` should be okay because `local_def_id_to_hir_id`
1361-
// would presumably panic if a fake `DefIndex` were passed.
1362-
.and_then(|dst_id| {
1363-
item.def_id.expect_def_id().as_local().map(|src_id| (src_id, dst_id))
1364-
})
1365-
{
1366-
if self.cx.tcx.privacy_access_levels(()).is_exported(src_id)
1367-
&& !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id)
1368-
{
1369-
privacy_error(self.cx, &diag_info, path_str);
1370-
}
1371-
}
1372-
1373-
Some(())
1374-
};
1375-
13761308
match res {
13771309
Res::Primitive(prim) => {
13781310
if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment {
1379-
let kind = self.cx.tcx.def_kind(id);
1380-
13811311
// We're actually resolving an associated item of a primitive, so we need to
13821312
// verify the disambiguator (if any) matches the type of the associated item.
13831313
// This case should really follow the same flow as the `Res::Def` branch below,
@@ -1386,7 +1316,16 @@ impl LinkCollector<'_, '_> {
13861316
// doesn't allow statements like `use str::trim;`, making this a (hopefully)
13871317
// valid omission. See https://github.com/rust-lang/rust/pull/80660#discussion_r551585677
13881318
// for discussion on the matter.
1389-
verify(kind, id)?;
1319+
let kind = self.cx.tcx.def_kind(id);
1320+
self.verify_disambiguator(
1321+
path_str,
1322+
&ori_link,
1323+
kind,
1324+
id,
1325+
disambiguator,
1326+
item,
1327+
&diag_info,
1328+
)?;
13901329

13911330
// FIXME: it would be nice to check that the feature gate was enabled in the original crate, not just ignore it altogether.
13921331
// However I'm not sure how to check that across crates.
@@ -1400,7 +1339,9 @@ impl LinkCollector<'_, '_> {
14001339
match disambiguator {
14011340
Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {}
14021341
Some(other) => {
1403-
report_mismatch(other, res);
1342+
self.report_disambiguator_mismatch(
1343+
path_str, &ori_link, other, res, &diag_info,
1344+
);
14041345
return None;
14051346
}
14061347
}
@@ -1414,13 +1355,106 @@ impl LinkCollector<'_, '_> {
14141355
})
14151356
}
14161357
Res::Def(kind, id) => {
1417-
verify(kind, id)?;
1358+
let (kind_for_dis, id_for_dis) =
1359+
if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment {
1360+
(self.cx.tcx.def_kind(id), id)
1361+
} else {
1362+
(kind, id)
1363+
};
1364+
self.verify_disambiguator(
1365+
path_str,
1366+
&ori_link,
1367+
kind_for_dis,
1368+
id_for_dis,
1369+
disambiguator,
1370+
item,
1371+
&diag_info,
1372+
)?;
14181373
let id = clean::register_res(self.cx, rustc_hir::def::Res::Def(kind, id));
14191374
Some(ItemLink { link: ori_link.link, link_text, did: id, fragment })
14201375
}
14211376
}
14221377
}
14231378

1379+
fn verify_disambiguator(
1380+
&self,
1381+
path_str: &str,
1382+
ori_link: &MarkdownLink,
1383+
kind: DefKind,
1384+
id: DefId,
1385+
disambiguator: Option<Disambiguator>,
1386+
item: &Item,
1387+
diag_info: &DiagnosticInfo<'_>,
1388+
) -> Option<()> {
1389+
debug!("intra-doc link to {} resolved to {:?}", path_str, (kind, id));
1390+
1391+
// Disallow e.g. linking to enums with `struct@`
1392+
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
1393+
match (kind, disambiguator) {
1394+
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const)))
1395+
// NOTE: this allows 'method' to mean both normal functions and associated functions
1396+
// This can't cause ambiguity because both are in the same namespace.
1397+
| (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn)))
1398+
// These are namespaces; allow anything in the namespace to match
1399+
| (_, Some(Disambiguator::Namespace(_)))
1400+
// If no disambiguator given, allow anything
1401+
| (_, None)
1402+
// All of these are valid, so do nothing
1403+
=> {}
1404+
(actual, Some(Disambiguator::Kind(expected))) if actual == expected => {}
1405+
(_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => {
1406+
self.report_disambiguator_mismatch(path_str,ori_link,specified, Res::Def(kind, id),diag_info);
1407+
return None;
1408+
}
1409+
}
1410+
1411+
// item can be non-local e.g. when using #[doc(primitive = "pointer")]
1412+
if let Some((src_id, dst_id)) = id
1413+
.as_local()
1414+
// The `expect_def_id()` should be okay because `local_def_id_to_hir_id`
1415+
// would presumably panic if a fake `DefIndex` were passed.
1416+
.and_then(|dst_id| {
1417+
item.def_id.expect_def_id().as_local().map(|src_id| (src_id, dst_id))
1418+
})
1419+
{
1420+
if self.cx.tcx.privacy_access_levels(()).is_exported(src_id)
1421+
&& !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id)
1422+
{
1423+
privacy_error(self.cx, diag_info, path_str);
1424+
}
1425+
}
1426+
1427+
Some(())
1428+
}
1429+
1430+
fn report_disambiguator_mismatch(
1431+
&self,
1432+
path_str: &str,
1433+
ori_link: &MarkdownLink,
1434+
specified: Disambiguator,
1435+
resolved: Res,
1436+
diag_info: &DiagnosticInfo<'_>,
1437+
) {
1438+
// The resolved item did not match the disambiguator; give a better error than 'not found'
1439+
let msg = format!("incompatible link kind for `{}`", path_str);
1440+
let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option<rustc_span::Span>| {
1441+
let note = format!(
1442+
"this link resolved to {} {}, which is not {} {}",
1443+
resolved.article(),
1444+
resolved.descr(),
1445+
specified.article(),
1446+
specified.descr(),
1447+
);
1448+
if let Some(sp) = sp {
1449+
diag.span_label(sp, &note);
1450+
} else {
1451+
diag.note(&note);
1452+
}
1453+
suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp);
1454+
};
1455+
report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback);
1456+
}
1457+
14241458
fn report_rawptr_assoc_feature_gate(&self, dox: &str, ori_link: &MarkdownLink, item: &Item) {
14251459
let span =
14261460
super::source_span_for_markdown_range(self.cx.tcx, dox, &ori_link.range, &item.attrs)

0 commit comments

Comments
 (0)