Rustdoc: Cache resolved links #77700

bugadani · 2020-10-08T11:51:29Z

A step towards #77681

rust-highfive · 2020-10-08T11:51:32Z

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2020-10-08T11:51:34Z

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2020-10-08T11:54:37Z

Looks good to me but I'll let @jyn514 handles it.

r? @jyn514

bugadani · 2020-10-08T12:26:58Z

So right now, 2 tests (should) have issues, because some [error] links are expected to fire errors multiple times. I plan on changing those tests so the links in them are unique and emit the expected errors.

src/test/rustdoc-ui/link-res-error-reported-once.rs

src/librustdoc/passes/collect_intra_doc_links.rs

jyn514 · 2020-10-08T15:28:09Z

src/test/rustdoc-ui/intra-links-warning.rs

-      //! , [Uniooon::X] and [Qux::Z].
-//~^ WARNING `Uniooon::X`
-//~| WARNING `Qux::Z`
+      //! , [Uniooon::Y] and [Qux::W].
+//~^ WARNING `Uniooon::Y`
+//~| WARNING `Qux::W`


@Manishearth do you know what this is meant to test? Is it that you get a warning on different items?

If it's just testing the warning is emitted at all I'd rather use the cases in intra-link-errors.rs I think.

src/librustdoc/passes/collect_intra_doc_links.rs

src/test/rustdoc-ui/intra-links-ambiguity.stderr

src/librustdoc/passes/collect_intra_doc_links.rs

jyn514 · 2020-10-08T18:27:16Z

src/librustdoc/passes/collect_intra_doc_links.rs

                return;
            }
-            (parts[0], Some(parts[1].to_owned()))
+            Some(parts[1].to_owned())


Doesn't need to be fixed here, but I wonder if this copy is actually necessary ...

src/librustdoc/passes/collect_intra_doc_links.rs

bugadani · 2020-10-08T18:58:30Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -563,14 +559,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
        current_item: &Option<String>,
        extra_fragment: &Option<String>,
    ) -> Option<Res> {
-        let check_full_res_inner = |this: &Self, result: Result<Res, ErrorKind<'_>>| {
+        let check_full_res_inner = |this: &Self, result: Result<Res, ErrorKind>| {


This function could be reworked to not have any closures and not be longer as it is currently. I wanted to do it, but for the sake of minimalism, this PR is a mess right now anyway and doesn't need any more.

Sure, seems like a good follow-up but I agree it should be separate.

jyn514 · 2020-10-08T18:59:27Z

Cache resolve errors

Why did you make this change? Performance only matters if there are no errors, and it made the code a lot more complicated

src/librustdoc/passes/collect_intra_doc_links.rs

bugadani · 2020-10-08T19:01:52Z

Cache resolve errors

Why did you make this change? Performance only matters if there are no errors, and it made the code a lot more complicated

I think I've been doing this for too long today and misunderstood you somewhere :)

jyn514 · 2020-10-08T19:02:21Z

Feel free to take a break :) I have homework I should be doing too 😆

bugadani · 2020-10-08T19:35:54Z

I didn't undo my lifetime-related changes, but I can if the Cow -> String is inconvenient.

jyn514 · 2020-10-08T19:55:01Z

I didn't undo my lifetime-related changes, but I can if the Cow -> String is inconvenient.

More that it causes unnecessary copies; it's probably not necessary but I'd rather not remove it since it's already there.

jyn514 · 2020-12-14T15:46:26Z

...
122	   |      ^^^^^^^^^
123	   |      |
124	   |      this link resolves to the associated function `g`, which is not in the type namespace
-	   |      help: to link to the associated function, add parentheses: `T::g()`
+	   |      help: to link to the associated function, add parentheses: `type@T::g()`
126	
-	error: unresolved link to `T::h`
+	error: unresolved link to `T::h!`
128	  --> $DIR/errors.rs:91:6
129	   |
130	LL | /// [T::h!]

-	   |      ^^^^^ the trait `T` has no macro named `h`
+	   |      ^^^^^ the trait `T` has no macro named `h!`
132	
-	error: unresolved link to `m`
+	error: unresolved link to `m()`
134	  --> $DIR/errors.rs:98:6
135	   |
136	LL | /// [m()]

137	   |      ^^^
138	   |      |
139	   |      this link resolves to the macro `m`, which is not in the value namespace
-	   |      help: to link to the macro, add an exclamation mark: `m!`
+	   |      help: to link to the macro, add an exclamation mark: `m()!`
141	
142	error: aborting due to 20 previous errors

Looks like this is now adding suggestions based on the original text instead of a normalized version, which isn't correct: type@T::g() isn't valid syntax and IMO error: unresolved link to `T::h!` looks like a bug where it interpreted ! as part of the link name.

bugadani · 2020-12-14T15:51:36Z

Yup, I know, maybe I broke it during rebase

bugadani · 2020-12-14T23:05:17Z

@jyn514 I undid the last two commits - the ones that spread the DiagInfo love around. I made at least two mistakes somewhere (the two commits had two different sets of test failures) and I don't have the brainpower to debug that right now. Also, the patch that depends on this one will conflict with the current split of diagnostic/resolution data. So right now this PR is a bit undercooked, but at least tests pass (locally).

jyn514 · 2020-12-14T23:38:25Z

I undid the last two commits - the ones that spread the DiagInfo love around.

Sounds good to me, those probably belonged in different PRs anyway.

I am not sure when rust-lang/rustc-perf#802 will be merged :( if it isn't by Friday I think we should just merge this anyway. Delegating to you in case I forget.

@bors delegate=bugadani

bors · 2020-12-14T23:38:27Z

✌️ @bugadani can now approve this pull request

jyn514 · 2020-12-15T00:26:37Z

@bors r+

See rust-lang/rustc-perf#802 (comment)

bors · 2020-12-15T00:26:38Z

📌 Commit fa64c27 has been approved by jyn514

bors · 2020-12-15T00:40:00Z

⌛ Testing commit fa64c27 with merge a1321f94ae00930fe0e091defe47f8d0dce31df1...

bors · 2020-12-15T01:56:10Z

💔 Test failed - checks-actions

jyn514 · 2020-12-15T02:00:30Z

 = note: LINK : fatal error LNK1201: error writing to program database 'D:\a\rust\rust\build\i686-pc-windows-msvc\test\run-make-fulldeps\issue64319\issue64319\bar.pdb'; check for insufficient disk space, invalid path, or insufficient privilege
          

error: aborting due to previous error

make[1]: *** [Makefile:17: all] Error 1

@bors retry

I feel like I've seen this spurious error before, @rust-lang/infra do you recognize it?

bors · 2020-12-15T04:06:57Z

⌛ Testing commit fa64c27 with merge e1cce06...

bors · 2020-12-15T06:31:42Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing e1cce06 to master...

pickfire · 2021-01-22T11:08:05Z

src/librustdoc/passes/collect_intra_doc_links.rs

                                    kind = ResolutionFailure::WrongNamespace(res, ns);
                                    break;
                                }
                            }
                        }
                        resolution_failure(
                            self,
-                            &item,
+                            diag.item,


Why not just change these 4 arguments to take diag?

Either it was forgotten, or resolution_failure is called in a lot of places where creating DiagInfo would have been a chore. I don't recall exactly.

Yes, a few other places but they could be made to use the struct.

rust-highfive assigned GuillaumeGomez Oct 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020

rust-highfive assigned jyn514 and unassigned GuillaumeGomez Oct 8, 2020

bugadani force-pushed the rustdoc-link-cache branch from 3a4f812 to ff4f5c6 Compare October 8, 2020 12:09

jyn514 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 Oct 8, 2020

jyn514 reviewed Oct 8, 2020

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 Oct 8, 2020

jyn514 reviewed Oct 8, 2020

View reviewed changes

jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2020

bugadani commented Oct 8, 2020

View reviewed changes

src/test/rustdoc-ui/intra-links-ambiguity.stderr Outdated Show resolved Hide resolved

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compiletime Issue: Problems and improvements with respect to compile times. labels Oct 8, 2020

bugadani force-pushed the rustdoc-link-cache branch from a2318a6 to ce944bb Compare October 8, 2020 18:19

jyn514 reviewed Oct 8, 2020

View reviewed changes

bugadani commented Oct 8, 2020

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

bugadani force-pushed the rustdoc-link-cache branch from 46a2338 to cca98a2 Compare October 8, 2020 19:43

Add test case for Self:: links

fa64c27

bugadani force-pushed the rustdoc-link-cache branch 2 times, most recently from c4b165d to 696cb27 Compare December 14, 2020 10:13

bugadani force-pushed the rustdoc-link-cache branch from 696cb27 to fa64c27 Compare December 14, 2020 23:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 15, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 15, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 15, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2020

bors merged commit e1cce06 into rust-lang:master Dec 15, 2020

rustbot added this to the 1.50.0 milestone Dec 15, 2020

bugadani deleted the rustdoc-link-cache branch December 19, 2020 16:55

jyn514 changed the title ~~Rustdoc: Cache resolved links in current module~~ Rustdoc: Cache resolved links Jan 15, 2021

bugadani mentioned this pull request Jan 15, 2021

Add rustdoc performance blog post rust-lang/blog.rust-lang.org#760

Merged

pickfire reviewed Jan 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustdoc: Cache resolved links #77700

Rustdoc: Cache resolved links #77700

bugadani commented Oct 8, 2020 •

edited

Loading

rust-highfive commented Oct 8, 2020

rust-highfive commented Oct 8, 2020

GuillaumeGomez commented Oct 8, 2020

bugadani commented Oct 8, 2020

jyn514 Oct 8, 2020

jyn514 Oct 8, 2020

bugadani Oct 8, 2020

jyn514 Oct 8, 2020

jyn514 commented Oct 8, 2020 •

edited

Loading

bugadani commented Oct 8, 2020

jyn514 commented Oct 8, 2020

bugadani commented Oct 8, 2020

jyn514 commented Oct 8, 2020

jyn514 commented Dec 14, 2020

bugadani commented Dec 14, 2020

bugadani commented Dec 14, 2020

jyn514 commented Dec 14, 2020

bors commented Dec 14, 2020

jyn514 commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

jyn514 commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

pickfire Jan 22, 2021

bugadani Jan 22, 2021

pickfire Jan 25, 2021

Rustdoc: Cache resolved links #77700

Rustdoc: Cache resolved links #77700

Conversation

bugadani commented Oct 8, 2020 • edited Loading

rust-highfive commented Oct 8, 2020

rust-highfive commented Oct 8, 2020

GuillaumeGomez commented Oct 8, 2020

bugadani commented Oct 8, 2020

jyn514 Oct 8, 2020

Choose a reason for hiding this comment

jyn514 Oct 8, 2020

Choose a reason for hiding this comment

bugadani Oct 8, 2020

Choose a reason for hiding this comment

jyn514 Oct 8, 2020

Choose a reason for hiding this comment

jyn514 commented Oct 8, 2020 • edited Loading

bugadani commented Oct 8, 2020

jyn514 commented Oct 8, 2020

bugadani commented Oct 8, 2020

jyn514 commented Oct 8, 2020

jyn514 commented Dec 14, 2020

bugadani commented Dec 14, 2020

bugadani commented Dec 14, 2020

jyn514 commented Dec 14, 2020

bors commented Dec 14, 2020

jyn514 commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

jyn514 commented Dec 15, 2020

bors commented Dec 15, 2020

bors commented Dec 15, 2020

pickfire Jan 22, 2021

Choose a reason for hiding this comment

bugadani Jan 22, 2021

Choose a reason for hiding this comment

pickfire Jan 25, 2021

Choose a reason for hiding this comment

bugadani commented Oct 8, 2020 •

edited

Loading

jyn514 commented Oct 8, 2020 •

edited

Loading