Skip to content

Commit cc31b99

Browse files
bugadanibugadani
committed
Review suggestions
1 parent 850437b commit cc31b99

File tree

1 file changed

+87
-79
lines changed

1 file changed

+87
-79
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs

+87-79
Original file line numberDiff line numberDiff line change
@@ -169,22 +169,18 @@ enum AnchorFailure {
169169
}
170170

171171
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
172-
struct CacheKey {
172+
struct ResolutionInfo {
173173
module_id: DefId,
174174
dis: Option<Disambiguator>,
175175
path_str: String,
176176
extra_fragment: Option<String>,
177177
}
178178

179-
impl CacheKey {
180-
fn new(
181-
module_id: DefId,
182-
dis: Option<Disambiguator>,
183-
path_str: String,
184-
extra_fragment: Option<String>,
185-
) -> Self {
186-
Self { module_id, dis, path_str, extra_fragment }
187-
}
179+
struct DiagnosticInfo<'a> {
180+
item: &'a Item,
181+
dox: &'a str,
182+
ori_link: &'a str,
183+
link_range: Option<Range<usize>>,
188184
}
189185

190186
#[derive(Clone, Debug, Hash)]
@@ -205,7 +201,7 @@ struct LinkCollector<'a, 'tcx> {
205201
/// See the code for associated items on inherent impls for details.
206202
kind_side_channel: Cell<Option<(DefKind, DefId)>>,
207203
/// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link
208-
visited_links: FxHashMap<CacheKey, CachedLink>,
204+
visited_links: FxHashMap<ResolutionInfo, CachedLink>,
209205
}
210206

211207
impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
@@ -1108,9 +1104,15 @@ impl LinkCollector<'_, '_> {
11081104
return None;
11091105
}
11101106

1111-
let key = CacheKey::new(module_id, disambiguator, path_str.to_owned(), extra_fragment);
1112-
let (mut res, mut fragment) =
1113-
self.resolve_with_disambiguator_cached(key, item, dox, &ori_link, link_range.clone())?;
1107+
let key = ResolutionInfo {
1108+
module_id,
1109+
dis: disambiguator,
1110+
path_str: path_str.to_owned(),
1111+
extra_fragment,
1112+
};
1113+
let diag =
1114+
DiagnosticInfo { item, dox, ori_link: &ori_link, link_range: link_range.clone() };
1115+
let (mut res, mut fragment) = self.resolve_with_disambiguator_cached(key, diag)?;
11141116

11151117
// Check for a primitive which might conflict with a module
11161118
// Report the ambiguity and require that the user specify which one they meant.
@@ -1220,59 +1222,47 @@ impl LinkCollector<'_, '_> {
12201222

12211223
fn resolve_with_disambiguator_cached(
12221224
&mut self,
1223-
key: CacheKey,
1224-
item: &Item,
1225-
dox: &str,
1226-
ori_link: &str,
1227-
link_range: Option<Range<usize>>,
1225+
key: ResolutionInfo,
1226+
diag: DiagnosticInfo<'_>,
12281227
) -> Option<(Res, Option<String>)> {
12291228
// Try to look up both the result and the corresponding side channel value
12301229
if let Some(ref cached) = self.visited_links.get(&key) {
12311230
self.kind_side_channel.set(cached.side_channel.clone());
1232-
Some(cached.res.clone())
1233-
} else {
1234-
match self.resolve_with_disambiguator(
1235-
key.dis,
1236-
item,
1237-
dox,
1238-
&key.path_str,
1239-
key.module_id,
1240-
key.extra_fragment.clone(),
1241-
ori_link,
1242-
link_range,
1243-
) {
1244-
Some(res) => {
1245-
// Store result for the actual namespace
1246-
self.visited_links.insert(
1247-
key,
1248-
CachedLink {
1249-
res: res.clone(),
1250-
side_channel: self.kind_side_channel.clone().into_inner(),
1251-
},
1252-
);
1253-
Some(res)
1254-
}
1255-
_ => None,
1256-
}
1231+
return Some(cached.res.clone());
12571232
}
1233+
1234+
let res = self.resolve_with_disambiguator(&key, diag);
1235+
1236+
// Cache only if resolved successfully - don't silence duplicate errors
1237+
if let Some(res) = &res {
1238+
// Store result for the actual namespace
1239+
self.visited_links.insert(
1240+
key,
1241+
CachedLink {
1242+
res: res.clone(),
1243+
side_channel: self.kind_side_channel.clone().into_inner(),
1244+
},
1245+
);
1246+
}
1247+
1248+
res
12581249
}
12591250

12601251
/// After parsing the disambiguator, resolve the main part of the link.
12611252
// FIXME(jynelson): wow this is just so much
12621253
fn resolve_with_disambiguator(
12631254
&self,
1264-
disambiguator: Option<Disambiguator>,
1265-
item: &Item,
1266-
dox: &str,
1267-
path_str: &str,
1268-
base_node: DefId,
1269-
extra_fragment: Option<String>,
1270-
ori_link: &str,
1271-
link_range: Option<Range<usize>>,
1255+
key: &ResolutionInfo,
1256+
diag: DiagnosticInfo<'_>,
12721257
) -> Option<(Res, Option<String>)> {
1258+
let disambiguator = key.dis;
1259+
let path_str = &key.path_str;
1260+
let base_node = key.module_id;
1261+
let extra_fragment = &key.extra_fragment;
1262+
12731263
match disambiguator.map(Disambiguator::ns) {
12741264
Some(ns @ (ValueNS | TypeNS)) => {
1275-
match self.resolve(path_str, ns, base_node, &extra_fragment) {
1265+
match self.resolve(path_str, ns, base_node, extra_fragment) {
12761266
Ok(res) => Some(res),
12771267
Err(ErrorKind::Resolve(box mut kind)) => {
12781268
// We only looked in one namespace. Try to give a better error if possible.
@@ -1281,24 +1271,21 @@ impl LinkCollector<'_, '_> {
12811271
// FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator`
12821272
// See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach
12831273
for &new_ns in &[other_ns, MacroNS] {
1284-
if let Some(res) = self.check_full_res(
1285-
new_ns,
1286-
path_str,
1287-
base_node,
1288-
&extra_fragment,
1289-
) {
1274+
if let Some(res) =
1275+
self.check_full_res(new_ns, path_str, base_node, extra_fragment)
1276+
{
12901277
kind = ResolutionFailure::WrongNamespace(res, ns);
12911278
break;
12921279
}
12931280
}
12941281
}
12951282
resolution_failure(
12961283
self,
1297-
&item,
1284+
diag.item,
12981285
path_str,
12991286
disambiguator,
1300-
dox,
1301-
link_range,
1287+
diag.dox,
1288+
diag.link_range,
13021289
smallvec![kind],
13031290
);
13041291
// This could just be a normal link or a broken link
@@ -1307,7 +1294,14 @@ impl LinkCollector<'_, '_> {
13071294
return None;
13081295
}
13091296
Err(ErrorKind::AnchorFailure(msg)) => {
1310-
anchor_failure(self.cx, &item, &ori_link, dox, link_range, msg);
1297+
anchor_failure(
1298+
self.cx,
1299+
diag.item,
1300+
diag.ori_link,
1301+
diag.dox,
1302+
diag.link_range,
1303+
msg,
1304+
);
13111305
return None;
13121306
}
13131307
}
@@ -1318,21 +1312,35 @@ impl LinkCollector<'_, '_> {
13181312
macro_ns: self
13191313
.resolve_macro(path_str, base_node)
13201314
.map(|res| (res, extra_fragment.clone())),
1321-
type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
1315+
type_ns: match self.resolve(path_str, TypeNS, base_node, extra_fragment) {
13221316
Ok(res) => {
13231317
debug!("got res in TypeNS: {:?}", res);
13241318
Ok(res)
13251319
}
13261320
Err(ErrorKind::AnchorFailure(msg)) => {
1327-
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
1321+
anchor_failure(
1322+
self.cx,
1323+
diag.item,
1324+
diag.ori_link,
1325+
diag.dox,
1326+
diag.link_range,
1327+
msg,
1328+
);
13281329
return None;
13291330
}
13301331
Err(ErrorKind::Resolve(box kind)) => Err(kind),
13311332
},
1332-
value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
1333+
value_ns: match self.resolve(path_str, ValueNS, base_node, extra_fragment) {
13331334
Ok(res) => Ok(res),
13341335
Err(ErrorKind::AnchorFailure(msg)) => {
1335-
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
1336+
anchor_failure(
1337+
self.cx,
1338+
diag.item,
1339+
diag.ori_link,
1340+
diag.dox,
1341+
diag.link_range,
1342+
msg,
1343+
);
13361344
return None;
13371345
}
13381346
Err(ErrorKind::Resolve(box kind)) => Err(kind),
@@ -1343,7 +1351,7 @@ impl LinkCollector<'_, '_> {
13431351
Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) => {
13441352
Err(ResolutionFailure::WrongNamespace(res, TypeNS))
13451353
}
1346-
_ => match (fragment, extra_fragment) {
1354+
_ => match (fragment, extra_fragment.clone()) {
13471355
(Some(fragment), Some(_)) => {
13481356
// Shouldn't happen but who knows?
13491357
Ok((res, Some(fragment)))
@@ -1359,11 +1367,11 @@ impl LinkCollector<'_, '_> {
13591367
if len == 0 {
13601368
resolution_failure(
13611369
self,
1362-
&item,
1370+
diag.item,
13631371
path_str,
13641372
disambiguator,
1365-
dox,
1366-
link_range,
1373+
diag.dox,
1374+
diag.link_range,
13671375
candidates.into_iter().filter_map(|res| res.err()).collect(),
13681376
);
13691377
// this could just be a normal link
@@ -1382,35 +1390,35 @@ impl LinkCollector<'_, '_> {
13821390
let candidates = candidates.map(|candidate| candidate.ok().map(|(res, _)| res));
13831391
ambiguity_error(
13841392
self.cx,
1385-
&item,
1393+
diag.item,
13861394
path_str,
1387-
dox,
1388-
link_range,
1395+
diag.dox,
1396+
diag.link_range,
13891397
candidates.present_items().collect(),
13901398
);
13911399
return None;
13921400
}
13931401
}
13941402
Some(MacroNS) => {
13951403
match self.resolve_macro(path_str, base_node) {
1396-
Ok(res) => Some((res, extra_fragment)),
1404+
Ok(res) => Some((res, extra_fragment.clone())),
13971405
Err(mut kind) => {
13981406
// `resolve_macro` only looks in the macro namespace. Try to give a better error if possible.
13991407
for &ns in &[TypeNS, ValueNS] {
14001408
if let Some(res) =
1401-
self.check_full_res(ns, path_str, base_node, &extra_fragment)
1409+
self.check_full_res(ns, path_str, base_node, extra_fragment)
14021410
{
14031411
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
14041412
break;
14051413
}
14061414
}
14071415
resolution_failure(
14081416
self,
1409-
&item,
1417+
diag.item,
14101418
path_str,
14111419
disambiguator,
1412-
dox,
1413-
link_range,
1420+
diag.dox,
1421+
diag.link_range,
14141422
smallvec![kind],
14151423
);
14161424
return None;

0 commit comments

Comments
 (0)