Add intra-doc `field@` disambiguator #83546

camelid · 2021-03-27T02:47:54Z

This change required some refactoring because fields (1) don't get their
own Res (instead they seem to use the Res of their parent node) and
(2) DefKind::Field::ns() returns None, so rustdoc has to
special-case fields. For some reason, we treat fields as if they're in
the value namespace - I'm not sure why that is.

Fixes #80283.

rust-highfive · 2021-03-27T02:47:56Z

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2021-03-27T02:47:57Z

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

This change required some refactoring because fields (1) don't get their own `Res` (instead they seem to use the `Res` of their parent node) and (2) `DefKind::Field::ns()` returns `None`, so rustdoc has to special-case fields. For some reason, we treat fields as if they're in the value namespace - I'm not sure why that is.

jyn514

I'm actually kinda iffy on this, I prefer the current behavior and we get to avoid clashes with foo.com.
Yeah I'm just worried about the possibilities of ambiguity. It was listed as an optional extension in the rfc and iirc this was part of why.

@Manishearth are you happy with this now that there's no ambiguity?

jyn514 · 2021-03-27T13:42:23Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                if expect_field {
+                    self.resolve_variant_or_field(item_name, did, ty_res, extra_fragment)
+                } else {


This function is already super long with lots of indentation - could you change this to a separate match arm? Res::Def(...) if expect_field => { ... }

Alternatively you could turn this whole match into a separate function, which lets you do an early return.

jyn514 · 2021-03-27T13:43:46Z

src/librustdoc/passes/collect_intra_doc_links.rs

+        match self.cx.tcx.type_of(did).kind() {
+            ty::Adt(def, _) => {


nit: I would make this an early return so you don't have so much indentation.

Suggested change

match self.cx.tcx.type_of(did).kind() {

ty::Adt(def, _) => {

let def = match self.cx.tcx.type_of(did).kind() {

ty::Adt(def, _) => def,

_ => return None,

};

jyn514 · 2021-03-27T13:46:14Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                // Fields are resolved to their parent type's Res.
+                // FIXME: what about DefKind::Variant?
+                | (DefKind::Struct | DefKind::Enum, Some(Disambiguator::Kind(DefKind::Field)))


This looks suspicious ... I'd have to look into how exactly rustc_resolve behaves here, but I'd rather not allow people to write

/// [field@S] struct S;

and have it work. Maybe you could add this to yet another side channel? 😅

This looks suspicious ... I'd have to look into how exactly rustc_resolve behaves here, but I'd rather not allow people to write

/// [field@S] struct S;

and have it work.

Your suspicion is correct, because that code works fine....

I don't think it's rustc_resolve behavior; we seem to use rustc_resolve to resolve everything up until the field, and then resolve that manually ourselves somehow. It seems like this code could use some cleanup; I think it would be a good idea for us to have our own Res enum that includes fields. Maybe it would be a good idea to do that first before implementing this feature?

Or, is there some way that rustc_resolve can resolve fields for us?

Or, is there some way that rustc_resolve can resolve fields for us?

I wouldn't expect so, since that needs type information (put another way, it can only resolve things that have a namespace).

I think it would be a good idea for us to have our own Res enum that includes fields.

Ok, there's already a custom Res type you could modify:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 71 in 58de79f

enum Res {

. But I'm not sure why that needs a new variant, I would expect using a normal Res::Def(DefKind::Field) to work.

I think it would be a good idea for us to have our own Res enum that includes fields.

Ok, there's already a custom Res type you could modify:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 71 in 58de79f

enum Res {

. But I'm not sure why that needs a new variant, I would expect using a normal Res::Def(DefKind::Field) to work.

Sounds good! I said that because I didn't understand why we didn't just use Res::Def(DefKind::Field) for the field resolution. So I think a good cleanup to do before I add this feature is to use Res::Def(DefKind::Field) to resolve fields instead of resolving them to their parent struct/enum. I think I'll play around with that a bit to see if I can get it working.

So I think a good cleanup to do before I add this feature is to use Res::Def(DefKind::Field) to resolve fields instead of resolving them to their parent struct/enum. I think I'll play around with that a bit to see if I can get it working.

Buyer beware:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Lines 605 to 608 in d474075

// HACK(jynelson): `clean` expects the type, not the associated item

// but the disambiguator logic expects the associated item.

// Store the kind in a side channel so that only the disambiguator logic looks at it.

self.kind_side_channel.set(Some((kind.as_def_kind(), id)));

but if you get it working that would be amazing :)

Uh-oh... ;)

jyn514 · 2021-03-27T13:50:29Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                    // Fields technically don't have a namespace, but we treat
+                    // fields as if they belong to the value namespace.
+                    DefKind::Field => TypeNS,


Suggested change

// Fields technically don't have a namespace, but we treat

// fields as if they belong to the value namespace.

DefKind::Field => TypeNS,

// Fields technically don't have a namespace, but we treat

// fields as if they belong to the value namespace.

DefKind::Field => ValueNS,

Do we need to return a namespace here? Would it be better to return an Option, the same way DefKind::ns does?

Oops... I said they belong to the value namespace because I saw that elsewhere, but I think in my mind I thought type namespace fitted better. Need to figure out if it makes sense to update elsewhere to use TypeNS or to switch to using ValueNS here...

Manishearth · 2021-03-27T14:38:14Z

@jyn514 If this is allowing field@Foo.bar that's still ambiguous, @s have meaning in URLs. field@Foo::bar is fine by me, that was never what my opposition was to.

jyn514 · 2021-03-27T14:52:07Z

field@Foo::bar is fine by me, that was never what my opposition was to.

Ok great, that's what this is :)

camelid · 2021-03-28T20:23:37Z

field@Foo::bar is fine by me, that was never what my opposition was to.

Great! 👍

I had an idea on how to get this to be like Foo.bar but without the ambiguity: Instead of rendering [`field@Foo::bar`] (and [`Foo::bar`] too if there's no method named bar) as Foo::bar, we render it as Foo.bar. What do you think?

Manishearth · 2021-03-28T22:37:17Z

That would be the first time we clean up the displayed text in intra doc links, no? Or do we do that already?

camelid · 2021-03-28T23:34:29Z

That would be the first time we clean up the displayed text in intra doc links, no? Or do we do that already?

We remove disambiguators from the displayed text, so [`prim@str`] renders as str. This would be like an extension of that.

jyn514 · 2021-03-29T01:01:12Z

I would rather not change the display. Foo.bar is valid syntax and I don't think people will always want it to be changed (as opposed to disambiguators, which are invalid syntax unless we remove them).

Manishearth · 2021-03-29T05:45:53Z

Yeah I think I'm aligned with @jyn514 on this one for now

jyn514 · 2021-03-29T18:57:56Z

I had an idea on how to get this to be like Foo.bar but without the ambiguity: Instead of rendering [field@Foo::bar] (and [Foo::bar] too if there's no method named bar) as Foo::bar, we render it as Foo.bar. What do you think?

Actually this is a good point - I think we should only remove the disambiguator if it's not a field, since otherwise it's ambiguous with the method (and Foo::bar isn't valid syntax for a field). People can still add their own titles if they want.

camelid · 2021-04-01T17:30:49Z

Actually this is a good point - I think we should only remove the disambiguator if it's not a field, since otherwise it's ambiguous with the method (and Foo::bar isn't valid syntax for a field). People can still add their own titles if they want.

I actually meant without the ambiguity that Manish was concerned about re domain names.

I think we should still strip the disambiguator. It's inconsistent and surprising if we strip all disambiguators except just field@. I don't think the ambiguity is a real concern since in some cases the reader can tell from context, and in all cases they can just click the link to find out.

Manishearth · 2021-04-01T17:46:55Z

Yeah I'm happy to strip the disambiguator.

jyn514 · 2021-04-01T19:11:52Z

Ok, in that case this is just waiting on questions about the implementation.

camelid · 2021-04-01T20:15:05Z

I think I would like to talk about #83546 (comment) before I address the review comments, since I don't want to waste time working on things that would be fixed by doing the cleanup.

rust-log-analyzer · 2021-04-01T20:18:32Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 435 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/src/test/rustdoc/intra-doc/field-disambiguator.rs:10: line longer than 100 chars
tidy error: /checkout/src/test/rustdoc/intra-doc/field-disambiguator.rs:24: line longer than 100 chars
tidy error: /checkout/src/test/rustdoc/intra-doc/field-disambiguator.rs:25: line longer than 100 chars
some tidy checks failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:14

jyn514 · 2021-04-01T22:52:25Z

I think I would like to talk about #83546 (comment) before I address the review comments, since I don't want to waste time working on things that would be fixed by doing the cleanup.

This link doesn't go anywhere for me - can you summarize what it says so I can find it?

camelid · 2021-04-01T23:14:48Z

I think I would like to talk about #83546 (comment) before I address the review comments, since I don't want to waste time working on things that would be fixed by doing the cleanup.

This link doesn't go anywhere for me - can you summarize what it says so I can find it?

Hmm, the link doesn't work for me either. It's the comment that you just replied to. Here's a working link: #83546 (comment)

bors · 2021-04-06T02:33:27Z

☔ The latest upstream changes (presumably #83905) made this pull request unmergeable. Please resolve the merge conflicts.

jyn514 · 2021-07-02T08:44:26Z

@camelid are you planning to follow up on this?

camelid · 2021-07-11T18:21:16Z

I'm not sure if I'll have the time soon to do the necessary refactor to get this working. I'm trying to make progress on some smaller PRs that I've had open for a while. I might be able to get to this after those.

jyn514 · 2021-08-16T05:28:55Z

@camelid I'm going to close this so someone else can pick up the issue - feel free to come back to this if you have time :)

camelid added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 27, 2021

rust-highfive assigned jyn514 Mar 27, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2021

camelid force-pushed the field-disambiguator branch from 842fb22 to 58de79f Compare March 27, 2021 02:49

This comment has been minimized.

Sign in to view

camelid marked this pull request as draft March 27, 2021 03:07

jyn514 reviewed Mar 27, 2021

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2021

Add an enum test case

04d5a32

camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2021

jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2021

jyn514 closed this Aug 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add intra-doc `field@` disambiguator #83546

Add intra-doc `field@` disambiguator #83546

camelid commented Mar 27, 2021 •

edited by jyn514

Loading

rust-highfive commented Mar 27, 2021

rust-highfive commented Mar 27, 2021

This comment has been minimized.

jyn514 left a comment

jyn514 Mar 27, 2021

jyn514 Mar 27, 2021

jyn514 Mar 27, 2021

camelid Apr 1, 2021

jyn514 Apr 1, 2021

camelid Apr 1, 2021 •

edited

Loading

jyn514 Apr 1, 2021

camelid Apr 1, 2021

jyn514 Mar 27, 2021

camelid Apr 1, 2021

Manishearth commented Mar 27, 2021

jyn514 commented Mar 27, 2021

camelid commented Mar 28, 2021 •

edited

Loading

Manishearth commented Mar 28, 2021

camelid commented Mar 28, 2021

jyn514 commented Mar 29, 2021 •

edited

Loading

Manishearth commented Mar 29, 2021

jyn514 commented Mar 29, 2021 •

edited

Loading

camelid commented Apr 1, 2021

Manishearth commented Apr 1, 2021

jyn514 commented Apr 1, 2021

camelid commented Apr 1, 2021

rust-log-analyzer commented Apr 1, 2021

jyn514 commented Apr 1, 2021

camelid commented Apr 1, 2021

bors commented Apr 6, 2021

jyn514 commented Jul 2, 2021

camelid commented Jul 11, 2021

jyn514 commented Aug 16, 2021

-        match self.cx.tcx.type_of(did).kind() {
-            ty::Adt(def, _) => {
+        let def = match self.cx.tcx.type_of(did).kind() {
+            ty::Adt(def, _) => def,
+            _ => return None,
+        };

	// HACK(jynelson): `clean` expects the type, not the associated item
	// but the disambiguator logic expects the associated item.
	// Store the kind in a side channel so that only the disambiguator logic looks at it.
	self.kind_side_channel.set(Some((kind.as_def_kind(), id)));

Add intra-doc field@ disambiguator #83546

Add intra-doc field@ disambiguator #83546

Conversation

camelid commented Mar 27, 2021 • edited by jyn514 Loading

rust-highfive commented Mar 27, 2021

rust-highfive commented Mar 27, 2021

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Mar 27, 2021

jyn514 commented Mar 27, 2021

camelid commented Mar 28, 2021 • edited Loading

Manishearth commented Mar 28, 2021

camelid commented Mar 28, 2021

jyn514 commented Mar 29, 2021 • edited Loading

Manishearth commented Mar 29, 2021

jyn514 commented Mar 29, 2021 • edited Loading

camelid commented Apr 1, 2021

Manishearth commented Apr 1, 2021

jyn514 commented Apr 1, 2021

camelid commented Apr 1, 2021

rust-log-analyzer commented Apr 1, 2021

jyn514 commented Apr 1, 2021

camelid commented Apr 1, 2021

bors commented Apr 6, 2021

jyn514 commented Jul 2, 2021

camelid commented Jul 11, 2021

jyn514 commented Aug 16, 2021

Add intra-doc `field@` disambiguator #83546

Add intra-doc `field@` disambiguator #83546

camelid commented Mar 27, 2021 •

edited by jyn514

Loading

camelid Apr 1, 2021 •

edited

Loading

camelid commented Mar 28, 2021 •

edited

Loading

jyn514 commented Mar 29, 2021 •

edited

Loading

jyn514 commented Mar 29, 2021 •

edited

Loading