intra-doc: Use an enum to represent URL fragments #92088

camelid · 2021-12-18T21:46:42Z

This is a step in the direction of computing the links more lazily,
which I think will simplify the implementation of intra-doc links.
This will also make it easier to eventually use the actual Res for
associated items, enum variants, and fields, rather than their HTML
page's Res.

r? @jyn514

rust-highfive · 2021-12-18T21:46:44Z

Some changes occurred in intra-doc-links.

cc @jyn514

Some changes occurred in clean/types.rs.

cc @camelid

camelid · 2021-12-18T21:49:46Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -732,7 +764,7 @@ fn resolve_associated_trait_item(
    item_name: Symbol,
    ns: Namespace,
    cx: &mut DocContext<'_>,
-) -> Option<(ty::AssocKind, DefId)> {
+) -> Option<ty::AssocItem> {


This change is not strictly necessary, but it seems like a good refactoring. The reason I made the change is that I was going to make UrlFragment::from_assoc_item take a ty::AssocItem and use that to get the defaultness, but that caused some tests to fail.

camelid · 2021-12-18T22:06:11Z

This may affect perf, so @bors try @rust-timer queue

rust-timer · 2021-12-18T22:06:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-18T22:06:19Z

⌛ Trying commit a074157f6b4975ebd2d80fac9c588969aab366d5 with merge b097a2fe84360de4b3bc3e854766482bf11c4acf...

bors · 2021-12-18T23:39:18Z

☀️ Try build successful - checks-actions
Build commit: b097a2fe84360de4b3bc3e854766482bf11c4acf (b097a2fe84360de4b3bc3e854766482bf11c4acf)

rust-timer · 2021-12-18T23:39:20Z

Queued b097a2fe84360de4b3bc3e854766482bf11c4acf with parent 91a0600, future comparison URL.

camelid · 2021-12-19T01:01:58Z

src/librustdoc/passes/collect_intra_doc_links.rs

+/// Render the fragment, including the leading `#`.
+impl std::fmt::Display for UrlFragment {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {


I thought adding a Display impl would be the best way to go, but I could be convinced of changing this to a method that takes &mut String or similar because

it would then have an explanatory method name (e.g., make it clear that it includes the #)

it might be more efficient since it avoids the Result and using Formatter

camelid · 2021-12-19T01:02:53Z

src/librustdoc/passes/collect_intra_doc_links.rs

-                    Some((kind, field.did)),
-                ))
+                let fragment = if def.is_enum() {
+                    // FIXME: how can the field be a variant?


This should be fixed after #92075 is merged.

camelid · 2021-12-19T01:11:38Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -457,8 +505,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
        path_str: &'path str,
        ns: Namespace,
        module_id: DefId,
-        extra_fragment: &Option<String>,
-    ) -> Result<(Res, Option<String>), ErrorKind<'path>> {
+        extra_fragment: &Option<UrlFragment>,


I believe this and other instances of extra_fragment are always user-written fragments.

rust-timer · 2021-12-19T04:32:02Z

Finished benchmarking commit (b097a2fe84360de4b3bc3e854766482bf11c4acf): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

Moderate regression in instruction counts (up to 3.2% on incr-patched: println builds of regression-31157)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

camelid · 2021-12-19T18:50:36Z

The rustc changes (including in the summary comment) are all spurious. Not sure what to do about those other than cc @rylev (let me know if I should stop pinging you for these).

The rustdoc changes show small improvements across many benchmarks.

camelid · 2021-12-19T18:53:34Z

max-rss shows small, low-significance regressions, but the biggest regressions are for benchmarks in which I don't think there are even intra-doc links. So probably mostly spurious.

camelid · 2021-12-27T20:02:55Z

Since Joshua is on vacation, r? @Manishearth

Manishearth

lgtm! r=me with the merge conflicts addressed

This is a step in the direction of computing the links more lazily, which I think will simplify the implementation of intra-doc links. This will also make it easier to eventually use the actual `Res` for associated items, enum variants, and fields, rather than their HTML page's `Res`.

camelid · 2021-12-28T02:21:28Z

Thanks for the review!

@bors r=Manishearth rollup=never

bors · 2021-12-28T02:21:29Z

📌 Commit ae2bc69 has been approved by Manishearth

bors · 2021-12-28T04:19:04Z

⌛ Testing commit ae2bc69 with merge cc65bf3...

bors · 2021-12-28T07:32:38Z

☀️ Test successful - checks-actions
Approved by: Manishearth
Pushing cc65bf3 to master...

rust-timer · 2021-12-28T09:01:07Z

Finished benchmarking commit (cc65bf3): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -0.8% on incr-unchanged builds of externs)
Moderate regression in instruction counts (up to 1.1% on incr-unchanged builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

…anishearth rustdoc: Remove the intra-doc links side channel The side channel made the code much more complex and harder to understand. It was added as a temporary workaround in 0c99d80, but it's no longer necessary. The addition of `UrlFragment` in rust-lang#92088 was the key to getting rid of the side channel. The semantic information (rather than the strings that used to be used for fragments) that is now captured by `UrlFragment` is enough to obviate the side channel. An additional change had to be made to `UrlFragment` in this PR to make this possible: it now records `DefId`s rather than item names. This PR also consolidates the checks for anchor conflicts into one place. r? `@Manishearth`

…ishearth rustdoc: Remove the intra-doc links side channel The side channel made the code much more complex and harder to understand. It was added as a temporary workaround in 0c99d80, but it's no longer necessary. The addition of `UrlFragment` in rust-lang#92088 was the key to getting rid of the side channel. The semantic information (rather than the strings that used to be used for fragments) that is now captured by `UrlFragment` is enough to obviate the side channel. An additional change had to be made to `UrlFragment` in this PR to make this possible: it now records `DefId`s rather than item names. This PR also consolidates the checks for anchor conflicts into one place. r? `@Manishearth`

camelid added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Dec 18, 2021

rust-highfive assigned jyn514 Dec 18, 2021

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 18, 2021

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

camelid force-pushed the intra-doc-cleanup branch from 36f36bb to a074157 Compare December 18, 2021 21:48

camelid commented Dec 18, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2021

camelid commented Dec 19, 2021

View reviewed changes

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 19, 2021

rust-highfive assigned Manishearth and unassigned jyn514 Dec 27, 2021

Manishearth approved these changes Dec 27, 2021

View reviewed changes

camelid added 2 commits December 27, 2021 17:52

Remove needless return

4472d97

camelid force-pushed the intra-doc-cleanup branch from a074157 to ae2bc69 Compare December 28, 2021 02:19

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 28, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 28, 2021

bors merged commit cc65bf3 into rust-lang:master Dec 28, 2021

rustbot added this to the 1.59.0 milestone Dec 28, 2021

This was referenced Dec 28, 2021

rustdoc: Only special case struct fields for intra-doc links, not enum variants #92075

Merged

Replace def_id_no_primitives with def_id #92342

Closed

camelid deleted the intra-doc-cleanup branch December 28, 2021 19:12

camelid mentioned this pull request Jan 6, 2022

rustdoc: Remove the intra-doc links side channel #92601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intra-doc: Use an enum to represent URL fragments #92088

intra-doc: Use an enum to represent URL fragments #92088

camelid commented Dec 18, 2021

rust-highfive commented Dec 18, 2021

camelid Dec 18, 2021

camelid commented Dec 18, 2021

rust-timer commented Dec 18, 2021

bors commented Dec 18, 2021

bors commented Dec 18, 2021

rust-timer commented Dec 18, 2021

camelid Dec 19, 2021

camelid Dec 19, 2021

camelid Dec 19, 2021

rust-timer commented Dec 19, 2021

camelid commented Dec 19, 2021

camelid commented Dec 19, 2021

camelid commented Dec 27, 2021

Manishearth left a comment

camelid commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

rust-timer commented Dec 28, 2021

intra-doc: Use an enum to represent URL fragments #92088

intra-doc: Use an enum to represent URL fragments #92088

Conversation

camelid commented Dec 18, 2021

rust-highfive commented Dec 18, 2021

camelid Dec 18, 2021

Choose a reason for hiding this comment

camelid commented Dec 18, 2021

rust-timer commented Dec 18, 2021

bors commented Dec 18, 2021

bors commented Dec 18, 2021

rust-timer commented Dec 18, 2021

camelid Dec 19, 2021

Choose a reason for hiding this comment

camelid Dec 19, 2021

Choose a reason for hiding this comment

camelid Dec 19, 2021

Choose a reason for hiding this comment

rust-timer commented Dec 19, 2021

camelid commented Dec 19, 2021

camelid commented Dec 19, 2021

camelid commented Dec 27, 2021

Manishearth left a comment

Choose a reason for hiding this comment

camelid commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

rust-timer commented Dec 28, 2021