Skip to content

Commit 47b8a52

Browse files
jyn514jyn514
committed
Report an ambiguity if both modules and primitives are in scope
- Add a new `prim@` disambiguator, since both modules and primitives are in the same namespace - Refactor `report_ambiguity` into a closure Additionally, I noticed that rustdoc would previously allow `[struct@char]` if `char` resolved to a primitive (not if it had a DefId). I fixed that and added a test case.
1 parent 8fdce9b commit 47b8a52

File tree

3 files changed

+192
-43
lines changed

3 files changed

+192
-43
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs

Lines changed: 109 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
181181
fn resolve(
182182
&self,
183183
path_str: &str,
184-
disambiguator: Option<Disambiguator>,
185184
ns: Namespace,
186185
current_item: &Option<String>,
187186
parent_id: Option<DefId>,
@@ -218,18 +217,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
218217
return Ok((res, Some(path_str.to_owned())));
219218
}
220219
Res::Def(DefKind::Mod, _) => {
221-
// This resolved to a module, but we want primitive types to take precedence instead.
222-
if matches!(
223-
disambiguator,
224-
None | Some(Disambiguator::Namespace(Namespace::TypeNS))
225-
) {
226-
if let Some((path, prim)) = is_primitive(path_str, ns) {
227-
if extra_fragment.is_some() {
228-
return Err(ErrorKind::AnchorFailure(AnchorFailure::Primitive));
229-
}
230-
return Ok((prim, Some(path.to_owned())));
231-
}
232-
}
233220
return Ok((res, extra_fragment.clone()));
234221
}
235222
_ => {
@@ -713,7 +700,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
713700
let resolved_self;
714701
let mut path_str;
715702
let disambiguator;
716-
let (res, fragment) = {
703+
let (mut res, mut fragment) = {
717704
path_str = if let Ok((d, path)) = Disambiguator::from_str(&link) {
718705
disambiguator = Some(d);
719706
path
@@ -756,7 +743,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
756743
Some(ns @ (ValueNS | TypeNS)) => {
757744
match self.resolve(
758745
path_str,
759-
disambiguator,
760746
ns,
761747
&current_item,
762748
base_node,
@@ -784,7 +770,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
784770
.map(|res| (res, extra_fragment.clone())),
785771
type_ns: match self.resolve(
786772
path_str,
787-
disambiguator,
788773
TypeNS,
789774
&current_item,
790775
base_node,
@@ -802,7 +787,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
802787
},
803788
value_ns: match self.resolve(
804789
path_str,
805-
disambiguator,
806790
ValueNS,
807791
&current_item,
808792
base_node,
@@ -848,13 +832,18 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
848832
if is_derive_trait_collision(&candidates) {
849833
candidates.macro_ns = None;
850834
}
835+
let candidates =
836+
candidates.map(|candidate| candidate.map(|(res, _)| res));
837+
let candidates = [TypeNS, ValueNS, MacroNS]
838+
.iter()
839+
.filter_map(|&ns| candidates[ns].map(|res| (res, ns)));
851840
ambiguity_error(
852841
cx,
853842
&item,
854843
path_str,
855844
&dox,
856845
link_range,
857-
candidates.map(|candidate| candidate.map(|(res, _)| res)),
846+
candidates.collect(),
858847
);
859848
continue;
860849
}
@@ -870,13 +859,81 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
870859
}
871860
};
872861

862+
// Check for a primitive which might conflict with a module
863+
// Report the ambiguity and require that the user specify which one they meant.
864+
// FIXME: could there ever be a primitive not in the type namespace?
865+
if matches!(
866+
disambiguator,
867+
None | Some(Disambiguator::Namespace(Namespace::TypeNS) | Disambiguator::Primitive)
868+
) && !matches!(res, Res::PrimTy(_))
869+
{
870+
if let Some((path, prim)) = is_primitive(path_str, TypeNS) {
871+
// `prim@char`
872+
if matches!(disambiguator, Some(Disambiguator::Primitive)) {
873+
if fragment.is_some() {
874+
anchor_failure(
875+
cx,
876+
&item,
877+
path_str,
878+
&dox,
879+
link_range,
880+
AnchorFailure::Primitive,
881+
);
882+
continue;
883+
}
884+
res = prim;
885+
fragment = Some(path.to_owned());
886+
} else {
887+
// `[char]` when a `char` module is in scope
888+
let candidates = vec![(res, TypeNS), (prim, TypeNS)];
889+
ambiguity_error(cx, &item, path_str, &dox, link_range, candidates);
890+
continue;
891+
}
892+
}
893+
}
894+
895+
let report_mismatch = |specified: Disambiguator, resolved: Disambiguator| {
896+
// The resolved item did not match the disambiguator; give a better error than 'not found'
897+
let msg = format!("incompatible link kind for `{}`", path_str);
898+
report_diagnostic(cx, &msg, &item, &dox, link_range.clone(), |diag, sp| {
899+
let note = format!(
900+
"this link resolved to {} {}, which is not {} {}",
901+
resolved.article(),
902+
resolved.descr(),
903+
specified.article(),
904+
specified.descr()
905+
);
906+
let suggestion = resolved.display_for(path_str);
907+
let help_msg =
908+
format!("to link to the {}, use its disambiguator", resolved.descr());
909+
diag.note(&note);
910+
if let Some(sp) = sp {
911+
diag.span_suggestion(
912+
sp,
913+
&help_msg,
914+
suggestion,
915+
Applicability::MaybeIncorrect,
916+
);
917+
} else {
918+
diag.help(&format!("{}: {}", help_msg, suggestion));
919+
}
920+
});
921+
};
873922
if let Res::PrimTy(_) = res {
874-
item.attrs.links.push((ori_link, None, fragment));
923+
match disambiguator {
924+
Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {
925+
item.attrs.links.push((ori_link, None, fragment))
926+
}
927+
Some(other) => {
928+
report_mismatch(other, Disambiguator::Primitive);
929+
continue;
930+
}
931+
}
875932
} else {
876933
debug!("intra-doc link to {} resolved to {:?}", path_str, res);
877934

878935
// Disallow e.g. linking to enums with `struct@`
879-
if let Res::Def(kind, id) = res {
936+
if let Res::Def(kind, _) = res {
880937
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
881938
match (self.kind_side_channel.take().unwrap_or(kind), disambiguator) {
882939
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const)))
@@ -890,22 +947,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
890947
// All of these are valid, so do nothing
891948
=> {}
892949
(actual, Some(Disambiguator::Kind(expected))) if actual == expected => {}
893-
(_, Some(Disambiguator::Kind(expected))) => {
894-
// The resolved item did not match the disambiguator; give a better error than 'not found'
895-
let msg = format!("incompatible link kind for `{}`", path_str);
896-
report_diagnostic(cx, &msg, &item, &dox, link_range, |diag, sp| {
897-
// HACK(jynelson): by looking at the source I saw the DefId we pass
898-
// for `expected.descr()` doesn't matter, since it's not a crate
899-
let note = format!("this link resolved to {} {}, which is not {} {}", kind.article(), kind.descr(id), expected.article(), expected.descr(id));
900-
let suggestion = Disambiguator::display_for(kind, path_str);
901-
let help_msg = format!("to link to the {}, use its disambiguator", kind.descr(id));
902-
diag.note(&note);
903-
if let Some(sp) = sp {
904-
diag.span_suggestion(sp, &help_msg, suggestion, Applicability::MaybeIncorrect);
905-
} else {
906-
diag.help(&format!("{}: {}", help_msg, suggestion));
907-
}
908-
});
950+
(_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => {
951+
report_mismatch(specified, Disambiguator::Kind(kind));
909952
continue;
910953
}
911954
}
@@ -961,14 +1004,15 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
9611004

9621005
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
9631006
enum Disambiguator {
1007+
Primitive,
9641008
Kind(DefKind),
9651009
Namespace(Namespace),
9661010
}
9671011

9681012
impl Disambiguator {
9691013
/// (disambiguator, path_str)
9701014
fn from_str(link: &str) -> Result<(Self, &str), ()> {
971-
use Disambiguator::{Kind, Namespace as NS};
1015+
use Disambiguator::{Kind, Namespace as NS, Primitive};
9721016

9731017
let find_suffix = || {
9741018
let suffixes = [
@@ -999,6 +1043,7 @@ impl Disambiguator {
9991043
"type" => NS(Namespace::TypeNS),
10001044
"value" => NS(Namespace::ValueNS),
10011045
"macro" => NS(Namespace::MacroNS),
1046+
"prim" | "primitive" => Primitive,
10021047
_ => return find_suffix(),
10031048
};
10041049
Ok((d, &rest[1..]))
@@ -1007,7 +1052,12 @@ impl Disambiguator {
10071052
}
10081053
}
10091054

1010-
fn display_for(kind: DefKind, path_str: &str) -> String {
1055+
fn display_for(self, path_str: &str) -> String {
1056+
let kind = match self {
1057+
Disambiguator::Primitive => return format!("prim@{}", path_str),
1058+
Disambiguator::Kind(kind) => kind,
1059+
Disambiguator::Namespace(_) => panic!("display_for cannot be used on namespaces"),
1060+
};
10111061
if kind == DefKind::Macro(MacroKind::Bang) {
10121062
return format!("{}!", path_str);
10131063
} else if kind == DefKind::Fn || kind == DefKind::AssocFn {
@@ -1043,6 +1093,25 @@ impl Disambiguator {
10431093
Self::Kind(k) => {
10441094
k.ns().expect("only DefKinds with a valid namespace can be disambiguators")
10451095
}
1096+
Self::Primitive => TypeNS,
1097+
}
1098+
}
1099+
1100+
fn article(self) -> &'static str {
1101+
match self {
1102+
Self::Namespace(_) => panic!("article() doesn't make sense for namespaces"),
1103+
Self::Kind(k) => k.article(),
1104+
Self::Primitive => "a",
1105+
}
1106+
}
1107+
1108+
fn descr(self) -> &'static str {
1109+
match self {
1110+
Self::Namespace(n) => n.descr(),
1111+
// HACK(jynelson): by looking at the source I saw the DefId we pass
1112+
// for `expected.descr()` doesn't matter, since it's not a crate
1113+
Self::Kind(k) => k.descr(DefId::local(hir::def_id::DefIndex::from_usize(0))),
1114+
Self::Primitive => "builtin type",
10461115
}
10471116
}
10481117
}
@@ -1183,14 +1252,10 @@ fn ambiguity_error(
11831252
path_str: &str,
11841253
dox: &str,
11851254
link_range: Option<Range<usize>>,
1186-
candidates: PerNS<Option<Res>>,
1255+
candidates: Vec<(Res, Namespace)>,
11871256
) {
11881257
let mut msg = format!("`{}` is ", path_str);
11891258

1190-
let candidates = [TypeNS, ValueNS, MacroNS]
1191-
.iter()
1192-
.filter_map(|&ns| candidates[ns].map(|res| (res, ns)))
1193-
.collect::<Vec<_>>();
11941259
match candidates.as_slice() {
11951260
[(first_def, _), (second_def, _)] => {
11961261
msg += &format!(
@@ -1229,6 +1294,7 @@ fn ambiguity_error(
12291294
}
12301295
_ => {
12311296
let type_ = match (res, ns) {
1297+
(Res::PrimTy(_), _) => "prim",
12321298
(Res::Def(DefKind::Const, _), _) => "const",
12331299
(Res::Def(DefKind::Static, _), _) => "static",
12341300
(Res::Def(DefKind::Struct, _), _) => "struct",

‎src/test/rustdoc-ui/intra-link-prim-conflict.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
#![deny(broken_intra_doc_links)]
2+
//~^ NOTE lint level is defined
3+
4+
/// [char]
5+
//~^ ERROR both a module and a builtin type
6+
//~| NOTE ambiguous link
7+
//~| HELP to link to the module
8+
//~| HELP to link to the builtin type
9+
10+
/// [type@char]
11+
//~^ ERROR both a module and a builtin type
12+
//~| NOTE ambiguous link
13+
//~| HELP to link to the module
14+
//~| HELP to link to the builtin type
15+
16+
/// [mod@char] // ok
17+
/// [prim@char] // ok
18+
19+
/// [struct@char]
20+
//~^ ERROR incompatible link
21+
//~| HELP use its disambiguator
22+
//~| NOTE resolved to a module
23+
pub mod char {}
24+
25+
pub mod inner {
26+
//! [struct@char]
27+
//~^ ERROR incompatible link
28+
//~| HELP use its disambiguator
29+
//~| NOTE resolved to a builtin type
30+
}

‎src/test/rustdoc-ui/intra-link-prim-conflict.stderr

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
error: `char` is both a module and a builtin type
2+
--> $DIR/intra-link-prim-conflict.rs:4:6
3+
|
4+
LL | /// [char]
5+
| ^^^^ ambiguous link
6+
|
7+
note: the lint level is defined here
8+
--> $DIR/intra-link-prim-conflict.rs:1:9
9+
|
10+
LL | #![deny(broken_intra_doc_links)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^
12+
help: to link to the module, prefix with the item type
13+
|
14+
LL | /// [module@char]
15+
| ^^^^^^^^^^^
16+
help: to link to the builtin type, prefix with the item type
17+
|
18+
LL | /// [prim@char]
19+
| ^^^^^^^^^
20+
21+
error: `char` is both a module and a builtin type
22+
--> $DIR/intra-link-prim-conflict.rs:10:6
23+
|
24+
LL | /// [type@char]
25+
| ^^^^^^^^^ ambiguous link
26+
|
27+
help: to link to the module, prefix with the item type
28+
|
29+
LL | /// [module@char]
30+
| ^^^^^^^^^^^
31+
help: to link to the builtin type, prefix with the item type
32+
|
33+
LL | /// [prim@char]
34+
| ^^^^^^^^^
35+
36+
error: incompatible link kind for `char`
37+
--> $DIR/intra-link-prim-conflict.rs:19:6
38+
|
39+
LL | /// [struct@char]
40+
| ^^^^^^^^^^^ help: to link to the module, use its disambiguator: `mod@char`
41+
|
42+
= note: this link resolved to a module, which is not a struct
43+
44+
error: incompatible link kind for `char`
45+
--> $DIR/intra-link-prim-conflict.rs:26:10
46+
|
47+
LL | //! [struct@char]
48+
| ^^^^^^^^^^^ help: to link to the builtin type, use its disambiguator: `prim@char`
49+
|
50+
= note: this link resolved to a builtin type, which is not a struct
51+
52+
error: aborting due to 4 previous errors
53+

0 commit comments

Comments
 (0)